Skip to content

Commit

Permalink
inspector: support extra contexts
Browse files Browse the repository at this point in the history
This enables inspector support for contexts created using the vm module.

PR-URL: #14465
Reviewed-By: Jan Krems <[email protected]>
Reviewed-By: Timothy Gu <[email protected]>
Reviewed-By: Aleksey Kozyatinskiy <[email protected]>
  • Loading branch information
eugeneo authored and Eugene Ostroukhov committed Aug 11, 2017
1 parent 73c59bb commit 63fc89a
Show file tree
Hide file tree
Showing 4 changed files with 80 additions and 2 deletions.
3 changes: 3 additions & 0 deletions src/env-inl.h
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,9 @@ inline void Environment::TickInfo::set_index(uint32_t value) {

inline void Environment::AssignToContext(v8::Local<v8::Context> context) {
context->SetAlignedPointerInEmbedderData(kContextEmbedderDataIndex, this);
#if HAVE_INSPECTOR
inspector_agent()->ContextCreated(context);
#endif // HAVE_INSPECTOR
}

inline Environment* Environment::GetCurrent(v8::Isolate* isolate) {
Expand Down
14 changes: 12 additions & 2 deletions src/inspector_agent.cc
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "libplatform/libplatform.h"

#include <string.h>
#include <sstream>
#include <unordered_map>
#include <vector>

Expand Down Expand Up @@ -500,6 +501,7 @@ class NodeInspectorClient : public V8InspectorClient {
terminated_(false),
running_nested_loop_(false) {
client_ = V8Inspector::create(env->isolate(), this);
contextCreated(env->context(), "Node.js Main Context");
}

void runMessageLoopOnPause(int context_group_id) override {
Expand Down Expand Up @@ -627,7 +629,8 @@ class NodeInspectorClient : public 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.
Expand All @@ -641,7 +644,6 @@ bool Agent::Start(v8::Platform* platform, const char* path,
client_ =
std::unique_ptr<NodeInspectorClient>(
new NodeInspectorClient(parent_env_, platform));
client_->contextCreated(parent_env_->context(), "Node.js Main Context");
platform_ = platform;
CHECK_EQ(0, uv_async_init(uv_default_loop(),
&start_io_thread_async,
Expand Down Expand Up @@ -841,6 +843,14 @@ void Agent::RequestIoThreadStart() {
uv_async_send(&start_io_thread_async);
}

void Agent::ContextCreated(Local<Context> context) {
if (client_ == nullptr) // This happens for a main context
return;
std::ostringstream name;
name << "VM Context " << next_context_number_++;
client_->contextCreated(context, name.str());
}

} // namespace inspector
} // namespace node

Expand Down
2 changes: 2 additions & 0 deletions src/inspector_agent.h
Original file line number Diff line number Diff line change
Expand Up @@ -96,6 +96,7 @@ class Agent {
void RequestIoThreadStart();

DebugOptions& options() { return debug_options_; }
void ContextCreated(v8::Local<v8::Context> context);

private:
node::Environment* parent_env_;
Expand All @@ -105,6 +106,7 @@ class Agent {
bool enabled_;
std::string path_;
DebugOptions debug_options_;
int next_context_number_;
};

} // namespace inspector
Expand Down
63 changes: 63 additions & 0 deletions test/inspector/test-contexts.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
'use strict';

// Flags: --expose-gc

const common = require('../common');
common.skipIfInspectorDisabled();

const { strictEqual } = require('assert');
const { runInNewContext } = require('vm');
const { Session } = require('inspector');

const session = new Session();
session.connect();

function notificationPromise(method) {
return new Promise((resolve) => session.once(method, resolve));
}

async function testContextCreatedAndDestroyed() {
console.log('Testing context created/destroyed notifications');
const mainContextPromise =
notificationPromise('Runtime.executionContextCreated');

session.post('Runtime.enable');
let contextCreated = await mainContextPromise;
strictEqual('Node.js Main Context',
contextCreated.params.context.name,
JSON.stringify(contextCreated));

const secondContextCreatedPromise =
notificationPromise('Runtime.executionContextCreated');

let contextDestroyed = null;
session.once('Runtime.executionContextDestroyed',
(notification) => contextDestroyed = notification);

runInNewContext('1 + 1', {});

contextCreated = await secondContextCreatedPromise;
strictEqual('VM Context 1',
contextCreated.params.context.name,
JSON.stringify(contextCreated));

// GC is unpredictable...
while (!contextDestroyed)
global.gc();

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Nov 20, 2017

Member

@eugeneo I have a hard time understanding this. Does the inspector call into JS land from the garbage collector? (I guess it has to, otherwise I don't see contextDestroyed can change - and change it does, that I checked.) Isn't that mightily unsafe?

This comment has been minimized.

Copy link
@eugeneo

eugeneo Nov 20, 2017

Author Contributor

This tests that Inspector does not prevent the contexts from being GCed.

Node keeps a weak reference to the context here - https://github.com/nodejs/node/blob/master/src/node_contextify.cc#L112

This comment has been minimized.

Copy link
@eugeneo

eugeneo Nov 20, 2017

Author Contributor

And this is what happens in the Inspector:

callbackData->m_inspector->contextCollected(callbackData->m_groupId,

I am not entirely sure what is considered "JS land" (e.g. Inspector at times exists between V8 loop iterations - not quite the same as "user code"). The notification is sent entirely from native code, nothing is triggered in a user code.

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Nov 20, 2017

Member

Right, what I mean is, this is a synchronous loop; contextDestroyed == null upon entering it. It changes because of the callback on line 34 where the inspector invokes the 'Runtime.executionContextDestroyed' event listener.

It sounds like the call chain is GC -> inspector -> JS callback, but calling into JS land when the GC is active is unsafe (or so it was in the past, at least.)

This comment has been minimized.

Copy link
@eugeneo

eugeneo Nov 20, 2017

Author Contributor

It is called on a "Second pass" which should be safe.


strictEqual(contextCreated.params.context.id,
contextDestroyed.params.executionContextId,
JSON.stringify(contextDestroyed));
}

async function testBreakpointHit() {
console.log('Testing breakpoint is hit in a new context');
session.post('Debugger.enable');

const pausedPromise = notificationPromise('Debugger.paused');
runInNewContext('debugger', {});
await pausedPromise;
}

common.crashOnUnhandledRejection();
testContextCreatedAndDestroyed().then(testBreakpointHit);

0 comments on commit 63fc89a

Please sign in to comment.