vm: allow modifying context name in inspector #17720

TimothyGu · 2017-12-17T23:10:13Z

The auxData field is not exposed to JavaScript, as DevTools uses it for its isDefault parameter, which is implemented faithfully, contributing to the nice indentation in the context selection panel. Without the indentation, when Target domain gets implemented (along with a single Inspector for cluster) in #16627, subprocesses and VM contexts will be mixed up, causing confusion.

Refs: #14231 (comment)

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
tests and/or benchmarks are included
documentation is changed or added
commit message follows commit guidelines

Affected core subsystem(s)

vm, inspector, src

devsnek · 2017-12-17T23:24:56Z

doc/api/vm.md

+    **Default:** `'VM Context i'`, where `i` is an ascending numerical index of
+    the created context.
+  * `contextOrigin` {string} [Origin][origin] corresponding to the newly
+    created context for display purposes. **Default:** the empty string.


the empty string -> an empty string or just empty string

Used '' instead.

devsnek · 2017-12-17T23:25:36Z

doc/api/vm.md

+    **Default:** `'VM Context i'`, where `i` is an ascending numerical index of
+    the created context.
+  * `contextOrigin` {string} [Origin][origin] corresponding to the newly
+    created context for display purposes. **Default:** the empty string.


vsemozhetbyt · 2017-12-18T00:05:56Z

doc/api/vm.md

@@ -242,12 +247,18 @@ console.log(globalVar);
 // 1000
 ```

-## vm.createContext([sandbox])
+## vm.createContext([sandbox[, options]])


It seems this bottom reference (line 505) needs to be updated now:

[`vm.createContext()`]: #vm_vm_createcontext_sandbox

into the

[`vm.createContext()`]: #vm_vm_createcontext_sandbox_options

maclover7

One small stylistic comment

cc @jasnell, should/has a formal guide been written up for internal errors JS/C++ interactions?

maclover7 · 2017-12-18T00:51:00Z

lib/vm.js

@@ -73,18 +75,57 @@ Script.prototype.runInContext = function(contextifiedSandbox, options) {
 };

 Script.prototype.runInNewContext = function(sandbox, options) {
-  var context = createContext(sandbox);
+  const contextOptions = options ? {


Not a super big deal, but should this code section be extracted into a separate function, since it's also used below?

bnoordhuis · 2017-12-18T11:32:29Z

src/node_contextify.cc

-    env->AssignToContext(ctx);
+    Local<Value> name =
+        options_obj->Get(env->context(),
+                         FIXED_ONE_BYTE_STRING(env->isolate(), "name"))


env->name_string()

bnoordhuis · 2017-12-18T11:35:34Z

doc/api/vm.md

+    **Default:** `'VM Context i'`, where `i` is an ascending numerical index of
+    the created context.
+  * `contextOrigin` {string} [Origin][origin] corresponding to the newly
+    created context for display purposes. **Default:** `''`.


Consider documenting that it's a (file?) URL? The linked page doesn't do a great job at describing what it is and mentions things that aren't relevant (e.g. CORS.)

bnoordhuis · 2017-12-18T11:36:53Z

src/inspector_agent.cc

 std::unique_ptr<StringBuffer> ToProtocolString(Isolate* isolate,
-                                               Local<Value> value) {
+                                               Local<T> value) {


Not necessary, as far as I can tell. A Local<String> is a Local<Value>.

bnoordhuis · 2017-12-18T11:40:55Z

src/inspector_agent.cc

+    ContextInfo info(
+        String::NewFromUtf8(env->isolate(),
+                            GetHumanReadableProcessName().c_str(),
+                            NewStringType::kNormal).ToLocalChecked());


I would prefer if ContextInfo used std::string because then you don't have to worry whether it's safe to call into V8 from here and what the lifetime of the Local<String> is (and also because it's a bit awkward to create a JS string here, only to unpack it almost immediately again.)

TimothyGu · 2017-12-20T03:37:52Z

@vsemozhetbyt @maclover7 @bnoordhuis Comments addressed; PTAL.

bnoordhuis · 2017-12-20T10:26:43Z

src/env.h

-  }
-  v8::Local<v8::String> name;
-  v8::Local<v8::String> origin;
+  explicit ContextInfo(std::string name) : name(name) {}


const std::string& since you're making a copy.

bnoordhuis · 2017-12-20T10:27:33Z

src/env.h

-  v8::Local<v8::String> name;
-  v8::Local<v8::String> origin;
+  explicit ContextInfo(std::string name) : name(name) {}
+  std::string name;


I'd make this const.

TimothyGu · 2017-12-20T14:58:27Z

@bnoordhuis Done and done.

maclover7

LGTM as far as docs and JS code goes. Can't really to speak to C++ side

TimothyGu · 2017-12-20T15:39:41Z

CI: https://ci.nodejs.org/job/node-test-pull-request/12232/

TimothyGu · 2017-12-23T06:20:53Z

Landed in 2cb2145.

The `auxData` field is not exposed to JavaScript, as DevTools uses it for its `isDefault` parameter, which is implemented faithfully, contributing to the nice indentation in the context selection panel. Without the indentation, when `Target` domain gets implemented (along with a single Inspector for cluster) in #16627, subprocesses and VM contexts will be mixed up, causing confusion. PR-URL: #17720 Refs: #14231 (comment) Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Jon Moss <me@jonathanmoss.me> Reviewed-By: James M Snell <jasnell@gmail.com>

gibfahn · 2018-01-24T01:28:32Z

The test for this fails when cherry-picking to v8.x:

▶▶▶ tools/test.py test/sequential/test-inspector-contexts.js                                                                                                       ~/wrk/com/DANGER/node (⇡v8.x-staging✦)
=== release test-inspector-contexts ===                    
Path: sequential/test-inspector-contexts
Testing context created/destroyed notifications
/Users/gib/wrk/com/DANGER/node/test/common/index.js:822
             (err) => process.nextTick(() => { throw err; }));
                                               ^

TypeError: Cannot read property 'isDefault' of undefined
    at testContextCreatedAndDestroyed (/Users/gib/wrk/com/DANGER/node/test/sequential/test-inspector-contexts.js:39:25)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)
    at Function.Module.runMain (module.js:686:11)
    at startup (bootstrap_node.js:188:16)
    at bootstrap_node.js:609:3
Command: out/Release/node --expose-gc /Users/gib/wrk/com/DANGER/node/test/sequential/test-inspector-contexts.js
[00:00|% 100|+   0|-   1]: Done

Could you either backport or change the label to dont-land (if it shouldn't land)?

TimothyGu requested review from bnoordhuis and eugeneo December 17, 2017 23:10

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. lib / src Issues and PRs related to general changes in the lib or src directory. labels Dec 17, 2017

TimothyGu added inspector Issues and PRs related to the V8 inspector protocol vm Issues and PRs related to the vm subsystem. labels Dec 17, 2017

TimothyGu requested a review from addaleax December 17, 2017 23:14

devsnek reviewed Dec 17, 2017

View reviewed changes

vsemozhetbyt reviewed Dec 18, 2017

View reviewed changes

maclover7 reviewed Dec 18, 2017

View reviewed changes

bnoordhuis reviewed Dec 18, 2017

View reviewed changes

bnoordhuis approved these changes Dec 20, 2017

View reviewed changes

TimothyGu added 4 commits December 20, 2017 06:36

vm: allow modifying context name in inspector

9e708d6

doc fixup

574b6f7

more comments

e6a939d

More comments

596fa5a

TimothyGu force-pushed the vm-meta branch from 350d2ac to 596fa5a Compare December 20, 2017 14:58

maclover7 approved these changes Dec 20, 2017

View reviewed changes

jasnell approved these changes Dec 20, 2017

View reviewed changes

TimothyGu closed this Dec 23, 2017

TimothyGu deleted the vm-meta branch December 23, 2017 06:24

MylesBorins mentioned this pull request Jan 10, 2018

v9.4.0 proposal #18069

Merged

gibfahn added dont-land-on-v6.x labels Jan 24, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

vm: allow modifying context name in inspector #17720

vm: allow modifying context name in inspector #17720

TimothyGu commented Dec 17, 2017 •

edited

Loading

devsnek Dec 17, 2017 •

edited

Loading

TimothyGu Dec 18, 2017

devsnek Dec 17, 2017

vsemozhetbyt Dec 18, 2017

maclover7 left a comment

maclover7 Dec 18, 2017

bnoordhuis Dec 18, 2017

bnoordhuis Dec 18, 2017

bnoordhuis Dec 18, 2017

bnoordhuis Dec 18, 2017

TimothyGu commented Dec 20, 2017

bnoordhuis Dec 20, 2017

bnoordhuis Dec 20, 2017

TimothyGu commented Dec 20, 2017

maclover7 left a comment

TimothyGu commented Dec 20, 2017

TimothyGu commented Dec 23, 2017

gibfahn commented Jan 24, 2018

vm: allow modifying context name in inspector #17720

vm: allow modifying context name in inspector #17720

Conversation

TimothyGu commented Dec 17, 2017 • edited Loading

Checklist

Affected core subsystem(s)

devsnek Dec 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maclover7 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Dec 20, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TimothyGu commented Dec 20, 2017

maclover7 left a comment

Choose a reason for hiding this comment

TimothyGu commented Dec 20, 2017

TimothyGu commented Dec 23, 2017

gibfahn commented Jan 24, 2018

TimothyGu commented Dec 17, 2017 •

edited

Loading

devsnek Dec 17, 2017 •

edited

Loading