From a4d47153c96d3b4b1fd6fd9efdcc1ef3a394c7e6 Mon Sep 17 00:00:00 2001 From: Eugene Ostroukhov Date: Mon, 24 Apr 2017 10:06:17 -0700 Subject: [PATCH 1/2] inspector: support extra contexts This enables inspector support for contexts created using the vm module. Fixes: https://github.com/nodejs/node/issues/7593 Fixes: https://github.com/nodejs/node/issues/12096 Refs: https://github.com/nodejs/node/pull/9272 --- src/env-inl.h | 9 +++++++ src/env.h | 1 + src/inspector_agent.cc | 17 +++++++++++- src/inspector_agent.h | 4 +++ src/node_contextify.cc | 2 ++ test/inspector/test-contexts.js | 47 +++++++++++++++++++++++++++++++++ 6 files changed, 79 insertions(+), 1 deletion(-) create mode 100644 test/inspector/test-contexts.js diff --git a/src/env-inl.h b/src/env-inl.h index cf7304c98dba76..5cf676724a266d 100644 --- a/src/env-inl.h +++ b/src/env-inl.h @@ -259,6 +259,15 @@ inline void Environment::TickInfo::set_index(uint32_t value) { inline void Environment::AssignToContext(v8::Local context) { context->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex, this); +#if HAVE_INSPECTOR + inspector_agent()->ContextCreated(context); +#endif // HAVE_INSPECTOR +} + +inline void Environment::UnassignFromContext(v8::Local context) { +#if HAVE_INSPECTOR + inspector_agent()->ContextDestroyed(context); +#endif // HAVE_INSPECTOR } inline Environment* Environment::GetCurrent(v8::Isolate* isolate) { diff --git a/src/env.h b/src/env.h index a0f4c30e8dd4b9..9857fdf2e4a216 100644 --- a/src/env.h +++ b/src/env.h @@ -530,6 +530,7 @@ class Environment { const char* const* exec_argv, bool start_profiler_idle_notifier); void AssignToContext(v8::Local context); + void UnassignFromContext(v8::Local context); void CleanupHandles(); void StartProfilerIdleNotifier(); diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index bfa2b082b4ef35..1b7ea0d814d16a 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -12,6 +12,7 @@ #include "libplatform/libplatform.h" #include +#include #include #ifdef __POSIX__ @@ -539,7 +540,8 @@ class NodeInspectorClient : public v8_inspector::V8InspectorClient { Agent::Agent(Environment* env) : parent_env_(env), client_(nullptr), platform_(nullptr), - enabled_(false) {} + enabled_(false), + next_context_number_(1) {} // Destructor needs to be defined here in implementation file as the header // does not have full definition of some classes. @@ -669,6 +671,19 @@ void Agent::PauseOnNextJavascriptStatement(const std::string& reason) { channel->schedulePauseOnNextStatement(reason); } +void Agent::ContextCreated(Local context) { + if (!IsStarted()) // This happens for a main context + return; + std::ostringstream name; + name << "VM Context " << next_context_number_++; + client_->contextCreated(context, name.str()); +} + +void Agent::ContextDestroyed(Local context) { + CHECK_NE(client_, nullptr); + client_->contextDestroyed(context); +} + void Open(const FunctionCallbackInfo& args) { Environment* env = Environment::GetCurrent(args); inspector::Agent* agent = env->inspector_agent(); diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 80967212cd7aef..378a60942c5ede 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -79,6 +79,9 @@ class Agent { bool enabled() { return enabled_; } void PauseOnNextJavascriptStatement(const std::string& reason); + void ContextCreated(v8::Local context); + void ContextDestroyed(v8::Local context); + // Initialize 'inspector' module bindings static void InitInspector(v8::Local target, v8::Local unused, @@ -105,6 +108,7 @@ class Agent { bool enabled_; std::string path_; DebugOptions debug_options_; + int next_context_number_; }; } // namespace inspector diff --git a/src/node_contextify.cc b/src/node_contextify.cc index 792b1c4f8009f9..59e76fe082c559 100644 --- a/src/node_contextify.cc +++ b/src/node_contextify.cc @@ -357,6 +357,8 @@ class ContextifyContext { static void WeakCallback(const WeakCallbackInfo& data) { ContextifyContext* context = data.GetParameter(); + context->env()->UnassignFromContext(context->context()); + delete context; } diff --git a/test/inspector/test-contexts.js b/test/inspector/test-contexts.js new file mode 100644 index 00000000000000..e1a7cfb1c09d76 --- /dev/null +++ b/test/inspector/test-contexts.js @@ -0,0 +1,47 @@ +'use strict'; +const common = require('../common'); +common.skipIfInspectorDisabled(); +const assert = require('assert'); +const helper = require('./inspector-helper.js'); + +function setupExpectBreakInContext() { + return function(message) { + if ('Debugger.paused' === message['method']) { + const callFrame = message['params']['callFrames'][0]; + assert.strictEqual(callFrame['functionName'], 'testfn'); + return true; + } + }; +} + +function testBreakpointInContext(session) { + console.log('[test]', + 'Verifying debugger stops on start (--inspect-brk option)'); + const commands = [ + { 'method': 'Runtime.enable' }, + { 'method': 'Debugger.enable' }, + { 'method': 'Runtime.runIfWaitingForDebugger' } + ]; + session + .sendInspectorCommands(commands) + .expectMessages(setupExpectBreakInContext()); +} + +function resume(session) { + session + .sendInspectorCommands({ 'method': 'Debugger.resume'}) + .disconnect(true); +} + +function runTests(harness) { + harness + .runFrontendSession([ + testBreakpointInContext, + resume, + ]).expectShutDown(0); +} + +const script = 'require("vm").runInNewContext(' + + '"function testfn() {debugger};testfn();")'; + +helper.startNodeForInspectorTest(runTests, '--inspect-brk', script); From 3c28258036fa99b366549eea7dc74dec624a5ad2 Mon Sep 17 00:00:00 2001 From: John Gozde Date: Tue, 20 Jun 2017 14:55:21 -0600 Subject: [PATCH 2/2] inspector: only call contextCreated if connected In contextCreated, V8Inspector wraps contexts with InspectedContext, which adds "global" and "console" to each context. This breaks the vm-test-basic tests and may lead to unexpected behaviour outside of inspector sessions. Calling contextCreated only if the inspector is actually connected ensures that this behaviour is limited to active inspector sessions. --- src/inspector_agent.cc | 25 ++++++++++++++++++++----- src/inspector_agent.h | 2 ++ 2 files changed, 22 insertions(+), 5 deletions(-) diff --git a/src/inspector_agent.cc b/src/inspector_agent.cc index 1b7ea0d814d16a..3217427f43e7ab 100644 --- a/src/inspector_agent.cc +++ b/src/inspector_agent.cc @@ -11,6 +11,7 @@ #include "libplatform/libplatform.h" +#include #include #include #include @@ -608,6 +609,13 @@ bool Agent::StartIoThread(bool wait_for_connect) { MakeCallback(parent_env_->isolate(), process_object, emit_fn.As(), arraysize(argv), argv, 0, 0); + // Send contexts to inspector client that were created before connecting + for (auto const& context : contexts_) { + std::ostringstream name; + name << "VM Context " << next_context_number_++; + client_->contextCreated(context, name.str()); + } + return true; } @@ -672,16 +680,23 @@ void Agent::PauseOnNextJavascriptStatement(const std::string& reason) { } void Agent::ContextCreated(Local context) { - if (!IsStarted()) // This happens for a main context + if (!IsStarted()) // This happens for a main context return; - std::ostringstream name; - name << "VM Context " << next_context_number_++; - client_->contextCreated(context, name.str()); + if (IsConnected()) { + std::ostringstream name; + name << "VM Context " << next_context_number_++; + client_->contextCreated(context, name.str()); + } + contexts_.push_back(context); } void Agent::ContextDestroyed(Local context) { CHECK_NE(client_, nullptr); - client_->contextDestroyed(context); + auto it = std::find(contexts_.begin(), contexts_.end(), context); + if (it != contexts_.end()) { + client_->contextDestroyed(context); + contexts_.erase(it); + } } void Open(const FunctionCallbackInfo& args) { diff --git a/src/inspector_agent.h b/src/inspector_agent.h index 378a60942c5ede..31ebb57a4ddc44 100644 --- a/src/inspector_agent.h +++ b/src/inspector_agent.h @@ -2,6 +2,7 @@ #define SRC_INSPECTOR_AGENT_H_ #include +#include #include @@ -108,6 +109,7 @@ class Agent { bool enabled_; std::string path_; DebugOptions debug_options_; + std::vector> contexts_; int next_context_number_; };