Skip to content

Commit

Permalink
console: refactor inspector console extension installation
Browse files Browse the repository at this point in the history
- Instead of creating the console extensions eagerly during bootstrap
  and storing them on an object, wrap the code into a function to be
  called during `installAdditionalCommandLineAPI` only when the
  extensions are actually needed, which may not even happen if the
  user do not use the console in an inspector session, and does not
  need to happen during bootstrap unconditionally.
- Simplify the console methods wrapping and move the `consoleFromVM`
  storage to `internal/util/inspector.js`

PR-URL: #25450
Reviewed-By: John-David Dalton <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
  • Loading branch information
joyeecheung authored and addaleax committed Jan 23, 2019
1 parent 647a37f commit 3c661f0
Show file tree
Hide file tree
Showing 9 changed files with 67 additions and 90 deletions.
2 changes: 1 addition & 1 deletion lib/inspector.js
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,6 @@ module.exports = {
url: url,
// This is dynamically added during bootstrap,
// where the console from the VM is still available
console: require('internal/console/inspector').consoleFromVM,
console: require('internal/util/inspector').consoleFromVM,
Session
};
12 changes: 8 additions & 4 deletions lib/internal/bootstrap/node.js
Original file line number Diff line number Diff line change
Expand Up @@ -705,13 +705,17 @@ function setupGlobalConsole() {
value: consoleFromNode,
writable: true
});
// TODO(joyeecheung): can we skip this if inspector is not active?

if (config.hasInspector) {
const inspector =
NativeModule.require('internal/console/inspector');
inspector.addInspectorApis(consoleFromNode, consoleFromVM);
const inspector = NativeModule.require('internal/util/inspector');
// This will be exposed by `require('inspector').console` later.
inspector.consoleFromVM = consoleFromVM;
// TODO(joyeecheung): postpone this until the first time inspector
// is activated.
inspector.wrapConsole(consoleFromNode, consoleFromVM);
const { setConsoleExtensionInstaller } = internalBinding('inspector');
// Setup inspector command line API.
setConsoleExtensionInstaller(inspector.installConsoleExtensions);
}
}

Expand Down
52 changes: 0 additions & 52 deletions lib/internal/console/inspector.js

This file was deleted.

51 changes: 46 additions & 5 deletions lib/internal/util/inspector.js
Original file line number Diff line number Diff line change
@@ -1,11 +1,9 @@
'use strict';

const { hasInspector } = internalBinding('config');
const inspector = hasInspector ? require('inspector') : undefined;

let session;

function sendInspectorCommand(cb, onError) {
const { hasInspector } = internalBinding('config');
const inspector = hasInspector ? require('inspector') : undefined;
if (!hasInspector) return onError();
if (session === undefined) session = new inspector.Session();
try {
Expand All @@ -20,6 +18,49 @@ function sendInspectorCommand(cb, onError) {
}
}

// Create a special require function for the inspector command line API
function installConsoleExtensions(commandLineApi) {
if (commandLineApi.require) { return; }
const { tryGetCwd } = require('internal/process/execution');
const CJSModule = require('internal/modules/cjs/loader');
const { makeRequireFunction } = require('internal/modules/cjs/helpers');
const consoleAPIModule = new CJSModule('<inspector console>');
const cwd = tryGetCwd();
consoleAPIModule.paths =
CJSModule._nodeModulePaths(cwd).concat(CJSModule.globalPaths);
commandLineApi.require = makeRequireFunction(consoleAPIModule);
}

// Wrap a console implemented by Node.js with features from the VM inspector
function wrapConsole(consoleFromNode, consoleFromVM) {
const config = {};
const { consoleCall } = internalBinding('inspector');
for (const key of Object.keys(consoleFromVM)) {
// If global console has the same method as inspector console,
// then wrap these two methods into one. Native wrapper will preserve
// the original stack.
if (consoleFromNode.hasOwnProperty(key)) {
consoleFromNode[key] = consoleCall.bind(consoleFromNode,
consoleFromVM[key],
consoleFromNode[key],
config);
} else {
// Add additional console APIs from the inspector
consoleFromNode[key] = consoleFromVM[key];
}
}
}

// Stores the console from VM, should be set during bootstrap.
let consoleFromVM;
module.exports = {
sendInspectorCommand
installConsoleExtensions,
sendInspectorCommand,
wrapConsole,
get consoleFromVM() {
return consoleFromVM;
},
set consoleFromVM(val) {
consoleFromVM = val;
}
};
1 change: 0 additions & 1 deletion node.gyp
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,6 @@
'lib/internal/cluster/worker.js',
'lib/internal/console/constructor.js',
'lib/internal/console/global.js',
'lib/internal/console/inspector.js',
'lib/internal/coverage-gen/with_profiler.js',
'lib/internal/coverage-gen/with_instrumentation.js',
'lib/internal/crypto/certificate.js',
Expand Down
7 changes: 0 additions & 7 deletions src/env.cc
Original file line number Diff line number Diff line change
Expand Up @@ -352,13 +352,6 @@ void Environment::Start(const std::vector<std::string>& args,
static uv_once_t init_once = UV_ONCE_INIT;
uv_once(&init_once, InitThreadLocalOnce);
uv_key_set(&thread_local_env, this);

#if HAVE_INSPECTOR
// This needs to be set before we start the inspector
Local<Object> obj = Object::New(isolate());
CHECK(obj->SetPrototype(context(), Null(isolate())).FromJust());
set_inspector_console_api_object(obj);
#endif // HAVE_INSPECTOR
}

void Environment::RegisterHandleCleanups() {
Expand Down
2 changes: 1 addition & 1 deletion src/env.h
Original file line number Diff line number Diff line change
Expand Up @@ -354,7 +354,7 @@ constexpr size_t kFsStatsBufferLength = kFsStatsFieldsNumber * 2;
V(http2settings_constructor_template, v8::ObjectTemplate) \
V(http2stream_constructor_template, v8::ObjectTemplate) \
V(immediate_callback_function, v8::Function) \
V(inspector_console_api_object, v8::Object) \
V(inspector_console_extension_installer, v8::Function) \
V(libuv_stream_wrap_ctor_template, v8::FunctionTemplate) \
V(message_port, v8::Object) \
V(message_port_constructor_template, v8::FunctionTemplate) \
Expand Down
16 changes: 5 additions & 11 deletions src/inspector_agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ namespace {

using node::FatalError;

using v8::Array;
using v8::Context;
using v8::Function;
using v8::HandleScope;
Expand Down Expand Up @@ -493,16 +492,11 @@ class NodeInspectorClient : public V8InspectorClient {

void installAdditionalCommandLineAPI(Local<Context> context,
Local<Object> target) override {
Local<Object> console_api = env_->inspector_console_api_object();
CHECK(!console_api.IsEmpty());

Local<Array> properties =
console_api->GetOwnPropertyNames(context).ToLocalChecked();
for (uint32_t i = 0; i < properties->Length(); ++i) {
Local<Value> key = properties->Get(context, i).ToLocalChecked();
target->Set(context,
key,
console_api->Get(context, key).ToLocalChecked()).FromJust();
Local<Function> installer = env_->inspector_console_extension_installer();
if (!installer.IsEmpty()) {
Local<Value> argv[] = {target};
// If there is an exception, proceed in JS land
USE(installer->Call(context, target, arraysize(argv), argv));
}
}

Expand Down
14 changes: 6 additions & 8 deletions src/inspector_js_api.cc
Original file line number Diff line number Diff line change
Expand Up @@ -122,16 +122,13 @@ static bool InspectorEnabled(Environment* env) {
return agent->IsActive();
}

void AddCommandLineAPI(const FunctionCallbackInfo<Value>& info) {
void SetConsoleExtensionInstaller(const FunctionCallbackInfo<Value>& info) {
auto env = Environment::GetCurrent(info);
Local<Context> context = env->context();

// inspector.addCommandLineAPI takes 2 arguments: a string and a value.
CHECK_EQ(info.Length(), 2);
CHECK(info[0]->IsString());
CHECK_EQ(info.Length(), 1);
CHECK(info[0]->IsFunction());

Local<Object> console_api = env->inspector_console_api_object();
console_api->Set(context, info[0], info[1]).FromJust();
env->set_inspector_console_extension_installer(info[0].As<Function>());
}

void CallAndPauseOnStart(const FunctionCallbackInfo<v8::Value>& args) {
Expand Down Expand Up @@ -279,7 +276,8 @@ void Initialize(Local<Object> target, Local<Value> unused,

Agent* agent = env->inspector_agent();
env->SetMethod(target, "consoleCall", InspectorConsoleCall);
env->SetMethod(target, "addCommandLineAPI", AddCommandLineAPI);
env->SetMethod(
target, "setConsoleExtensionInstaller", SetConsoleExtensionInstaller);
if (agent->WillWaitForConnect())
env->SetMethod(target, "callAndPauseOnStart", CallAndPauseOnStart);
env->SetMethod(target, "open", Open);
Expand Down

0 comments on commit 3c661f0

Please sign in to comment.