From 3c661f0aa6593b70747a0e2143faeeb7f2529758 Mon Sep 17 00:00:00 2001 From: Joyee Cheung Date: Tue, 15 Jan 2019 21:57:29 +0800 Subject: [PATCH] console: refactor inspector console extension installation - 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: https://github.com/nodejs/node/pull/25450 Reviewed-By: John-David Dalton Reviewed-By: James M Snell Reviewed-By: Ruben Bridgewater --- lib/inspector.js | 2 +- lib/internal/bootstrap/node.js | 12 ++++--- lib/internal/console/inspector.js | 52 ------------------------------- lib/internal/util/inspector.js | 51 +++++++++++++++++++++++++++--- node.gyp | 1 - src/env.cc | 7 ----- src/env.h | 2 +- src/inspector_agent.cc | 16 +++------- src/inspector_js_api.cc | 14 ++++----- 9 files changed, 67 insertions(+), 90 deletions(-) delete mode 100644 lib/internal/console/inspector.js diff --git a/lib/inspector.js b/lib/inspector.js index 565b3aa9727644..cc8f86ee46c7c3 100644 --- a/lib/inspector.js +++ b/lib/inspector.js @@ -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 }; diff --git a/lib/internal/bootstrap/node.js b/lib/internal/bootstrap/node.js index 50f5001aa4b0fd..c06370f0bb9b58 100644 --- a/lib/internal/bootstrap/node.js +++ b/lib/internal/bootstrap/node.js @@ -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); } } diff --git a/lib/internal/console/inspector.js b/lib/internal/console/inspector.js deleted file mode 100644 index 3dbc68a193db53..00000000000000 --- a/lib/internal/console/inspector.js +++ /dev/null @@ -1,52 +0,0 @@ -'use strict'; - -const CJSModule = require('internal/modules/cjs/loader'); -const { makeRequireFunction } = require('internal/modules/cjs/helpers'); -const { tryGetCwd } = require('internal/process/execution'); -const { addCommandLineAPI, consoleCall } = internalBinding('inspector'); - -// Wrap a console implemented by Node.js with features from the VM inspector -function addInspectorApis(consoleFromNode, consoleFromVM) { - // Setup inspector command line API. - const cwd = tryGetCwd(); - const consoleAPIModule = new CJSModule(''); - consoleAPIModule.paths = - CJSModule._nodeModulePaths(cwd).concat(CJSModule.globalPaths); - addCommandLineAPI('require', makeRequireFunction(consoleAPIModule)); - const config = {}; - - // If global console has the same method as inspector console, - // then wrap these two methods into one. Native wrapper will preserve - // the original stack. - for (const key of Object.keys(consoleFromNode)) { - if (!consoleFromVM.hasOwnProperty(key)) - continue; - consoleFromNode[key] = consoleCall.bind(consoleFromNode, - consoleFromVM[key], - consoleFromNode[key], - config); - } - - // Add additional console APIs from the inspector - for (const key of Object.keys(consoleFromVM)) { - if (consoleFromNode.hasOwnProperty(key)) - continue; - consoleFromNode[key] = consoleFromVM[key]; - } -} - -module.exports = { - addInspectorApis -}; - -// Stores the console from VM, should be set during bootstrap. -let consoleFromVM; - -Object.defineProperty(module.exports, 'consoleFromVM', { - get() { - return consoleFromVM; - }, - set(val) { - consoleFromVM = val; - } -}); diff --git a/lib/internal/util/inspector.js b/lib/internal/util/inspector.js index face748ab0df69..68c20108bd317b 100644 --- a/lib/internal/util/inspector.js +++ b/lib/internal/util/inspector.js @@ -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 { @@ -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(''); + 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; + } }; diff --git a/node.gyp b/node.gyp index 03be5a05ea7813..8ca1ad3165bce4 100644 --- a/node.gyp +++ b/node.gyp @@ -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', diff --git a/src/env.cc b/src/env.cc index f45386dce4a058..97cfc72a9dd58e 100644 --- a/src/env.cc +++ b/src/env.cc @@ -352,13 +352,6 @@ void Environment::Start(const std::vector& 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 obj = Object::New(isolate()); - CHECK(obj->SetPrototype(context(), Null(isolate())).FromJust()); - set_inspector_console_api_object(obj); -#endif // HAVE_INSPECTOR } void Environment::RegisterHandleCleanups() { diff --git a/src/env.h b/src/env.h index ec1a893c206f4f..2209f907de8d33 100644 --- a/src/env.h +++ b/src/env.h @@ -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) \ diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index a53c58337e900b..dcc256b7f70434 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -32,7 +32,6 @@ namespace { using node::FatalError; -using v8::Array; using v8::Context; using v8::Function; using v8::HandleScope; @@ -493,16 +492,11 @@ class NodeInspectorClient : public V8InspectorClient { void installAdditionalCommandLineAPI(Local context, Local target) override { - Local console_api = env_->inspector_console_api_object(); - CHECK(!console_api.IsEmpty()); - - Local properties = - console_api->GetOwnPropertyNames(context).ToLocalChecked(); - for (uint32_t i = 0; i < properties->Length(); ++i) { - Local key = properties->Get(context, i).ToLocalChecked(); - target->Set(context, - key, - console_api->Get(context, key).ToLocalChecked()).FromJust(); + Local installer = env_->inspector_console_extension_installer(); + if (!installer.IsEmpty()) { + Local argv[] = {target}; + // If there is an exception, proceed in JS land + USE(installer->Call(context, target, arraysize(argv), argv)); } } diff --git a/src/inspector_js_api.cc b/src/inspector_js_api.cc index 3ab0e5e9fccf3c..3547e1022fb615 100644 --- a/src/inspector_js_api.cc +++ b/src/inspector_js_api.cc @@ -122,16 +122,13 @@ static bool InspectorEnabled(Environment* env) { return agent->IsActive(); } -void AddCommandLineAPI(const FunctionCallbackInfo& info) { +void SetConsoleExtensionInstaller(const FunctionCallbackInfo& info) { auto env = Environment::GetCurrent(info); - Local 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 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()); } void CallAndPauseOnStart(const FunctionCallbackInfo& args) { @@ -279,7 +276,8 @@ void Initialize(Local target, Local 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);