vm: allow modifying context name in inspector by TimothyGu · Pull Request #17720 · nodejs/node

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

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

Used '' instead.

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

doc/api/vm.md

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

doc/api/vm.md

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

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->name_string()

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

doc/api/vm.md

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

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

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

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

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

src/env.h

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

doc/api/vm.md Outdated

Copy link

Member

devsnek Dec 17, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here

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

Uh oh!

Conversation

TimothyGu commented Dec 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Uh oh!

devsnek Dec 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maclover7 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Dec 20, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Dec 20, 2017

Uh oh!

maclover7 left a comment

Choose a reason for hiding this comment

Uh oh!

TimothyGu commented Dec 20, 2017

Uh oh!

TimothyGu commented Dec 23, 2017

Uh oh!

gibfahn commented Jan 24, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

TimothyGu commented Dec 17, 2017 •

edited

Loading

devsnek Dec 17, 2017 •

edited

Loading