New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

inspector: support non-main contexts #14465

Merged
merged 0 commits into from Aug 11, 2017

Conversation

Projects
None yet
@eugeneo
Contributor

eugeneo commented Jul 25, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • commit message follows commit guidelines
Affected core subsystem(s)

inspector: non-main contexts are now reported to an inspector.

This is a suggested implementation of non-main contexts support for the inspector. It is based on a recently landed V8 patch that is included into this pull request.

This solution has a number of shortcomings that need to be fixed in V8:

  1. Inspector console is accessible from the new contexts. This is not the V8 console, e.g. using it will not print anything to stdout.
  2. Contexts are retained by the inspector for the duration of the session. This only happens if some event is reported that concerns inspector - e.g. console output or breakpoint hit.

This pull request is a suggested alternative to #14231 - note that 14231 also adds the inspector console.

CC: @ak239

refack added: link to V8 patch's Gerrit

@refack

This comment has been minimized.

Show comment
Hide comment
@refack

refack Jul 25, 2017

Member

@eugeneo do you know if the patch will be available in mainline V8 5.9 or 6.0?
/cc @nodejs/v8 @nodejs/v8-inspector @TimothyGu @nodejs/diagnostics

Member

refack commented Jul 25, 2017

@eugeneo do you know if the patch will be available in mainline V8 5.9 or 6.0?
/cc @nodejs/v8 @nodejs/v8-inspector @TimothyGu @nodejs/diagnostics

@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Jul 25, 2017

Contributor

@refack looks like V8 commit got rolled back tonight. This PR is on hold until the issue is resolved...

@TimothyGu should I make context registration happen in JS land in lib/vm.js? Re: name - what is your suggestion? I can add a symbol that is can be added to a sandbox object - e.g:

const sandbox = {};
sandbox[require('inspector').CONTEXT_NAME] = 'my context';
const ctx = require('vm').createContext(sandbox);

Surely, it will be optional.

Contributor

eugeneo commented Jul 25, 2017

@refack looks like V8 commit got rolled back tonight. This PR is on hold until the issue is resolved...

@TimothyGu should I make context registration happen in JS land in lib/vm.js? Re: name - what is your suggestion? I can add a symbol that is can be added to a sandbox object - e.g:

const sandbox = {};
sandbox[require('inspector').CONTEXT_NAME] = 'my context';
const ctx = require('vm').createContext(sandbox);

Surely, it will be optional.

@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Jul 26, 2017

Member

I'd prefer them to be options to createContext much like what I implemented for inspector.attachContext.

I'm fine with registering it in C++. I just want things in the Agent to be usable by everything rather than by only one thing, and want Agent to include support for every param Inspector has to offer, namely groupId, name, origin, and auxData (which is a JSON string used by Dev Tools for formatting purposes). Of course, some of these things may be unfit to be exposed to the JS side, like groupId and auxData, but Agent should be agnostic to all that IMO.

Member

TimothyGu commented Jul 26, 2017

I'd prefer them to be options to createContext much like what I implemented for inspector.attachContext.

I'm fine with registering it in C++. I just want things in the Agent to be usable by everything rather than by only one thing, and want Agent to include support for every param Inspector has to offer, namely groupId, name, origin, and auxData (which is a JSON string used by Dev Tools for formatting purposes). Of course, some of these things may be unfit to be exposed to the JS side, like groupId and auxData, but Agent should be agnostic to all that IMO.

@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Aug 2, 2017

Contributor

A new version of the V8 patch landed, should not get rolled back now... I backported it and rebased this pull request.

@TimothyGu I am still not sure what you are proposing. Should those options come through the vm.js module or you want the users to explicitly opt-in the contexts into inspector tracking?

Contributor

eugeneo commented Aug 2, 2017

A new version of the V8 patch landed, should not get rolled back now... I backported it and rebased this pull request.

@TimothyGu I am still not sure what you are proposing. Should those options come through the vm.js module or you want the users to explicitly opt-in the contexts into inspector tracking?

@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Aug 2, 2017

Contributor

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

I believe this PR is ready for the review. It does not expose any new API and is not visible to the users, except that breakpoints will start working in new contexts. If there is a need to add an API to name the contexts, etc - that can be done separately at the later time.

Contributor

eugeneo commented Aug 2, 2017

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

I believe this PR is ready for the review. It does not expose any new API and is not visible to the users, except that breakpoints will start working in new contexts. If there is a need to add an API to name the contexts, etc - that can be done separately at the later time.

@jkrems

jkrems approved these changes Aug 2, 2017

LGTM

Show outdated Hide outdated src/node_contextify.cc
@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Aug 2, 2017

Contributor

CI failures do not seem to be related to this change.

V8 patch will be in V8 6.1 release.

Contributor

eugeneo commented Aug 2, 2017

CI failures do not seem to be related to this change.

V8 patch will be in V8 6.1 release.

@ofrobots

This comment has been minimized.

Show comment
Hide comment
@ofrobots

ofrobots Aug 4, 2017

Contributor

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/838/

@eugeneo what are the chances of upstream approving a backport to 6.0?

Normally we bump the V8 version when floating a patch backwards. But given that V8 6.0 is still supported upstream, it is not safe to do given that upstream will be bumping versions too. (/cc @targos in case we bring in another update of 6.0 – we will need to re-float this at that time.) If upstream backports to 6.0, this problem goes away.

Contributor

ofrobots commented Aug 4, 2017

V8 CI: https://ci.nodejs.org/job/node-test-commit-v8-linux/838/

@eugeneo what are the chances of upstream approving a backport to 6.0?

Normally we bump the V8 version when floating a patch backwards. But given that V8 6.0 is still supported upstream, it is not safe to do given that upstream will be bumping versions too. (/cc @targos in case we bring in another update of 6.0 – we will need to re-float this at that time.) If upstream backports to 6.0, this problem goes away.

@targos

This comment has been minimized.

Show comment
Hide comment
@targos

targos Aug 4, 2017

Member

Could we have the patch merged to 6.1?

Member

targos commented Aug 4, 2017

Could we have the patch merged to 6.1?

@TimothyGu

This comment has been minimized.

Show comment
Hide comment
@TimothyGu

TimothyGu Aug 4, 2017

Member

@eugeneo Is the console property still unconditionally added? If it's not possible to remove it then at least some documentation is warranted.

Member

TimothyGu commented Aug 4, 2017

@eugeneo Is the console property still unconditionally added? If it's not possible to remove it then at least some documentation is warranted.

@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Aug 4, 2017

Contributor

CC: @ak239

Alexei, can you help with the information about V8 version and the backport?

Contributor

eugeneo commented Aug 4, 2017

CC: @ak239

Alexei, can you help with the information about V8 version and the backport?

@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Aug 4, 2017

Contributor

@eugeneo Is the console property still unconditionally added? If it's not possible to remove it then at least some documentation is warranted.

I see console unconditionally added in master, even without this patch.E.g. ./node -e "console.log(require('vm').runInNewContext('console', {}))" on master prints console object, while same line on an 8.x version throws an exception ReferenceError: console is not defined

Contributor

eugeneo commented Aug 4, 2017

@eugeneo Is the console property still unconditionally added? If it's not possible to remove it then at least some documentation is warranted.

I see console unconditionally added in master, even without this patch.E.g. ./node -e "console.log(require('vm').runInNewContext('console', {}))" on master prints console object, while same line on an 8.x version throws an exception ReferenceError: console is not defined

@ak239

This comment has been minimized.

Show comment
Hide comment
@ak239

ak239 Aug 4, 2017

Member

Ok, let me split problems: console and non-main context inspection are two different problems.
About CL required for non-main context inspection:
V8 CL is landed in 6.2. It's some kind of revert, since it just restored behavior which exists around 5.8 or 5.9. Unfortunately it's almost impossible to backport it to earlier version of V8 since AFAIK if we backport it to 6.1 or 6.0 then we need to merge it to corresponded Chrome version but this CL is not needed by Chrome (blink calls contextDestroyed method manually and manage context lifetime in different way). So is it possible to backport it only inside of Node.js?

About console:
Console was added in https://codereview.chromium.org/2785293002 and available in V8 since 6.0 so it sounds to me like if node 8 use v8 6.0 then it should always have builtin console object on any context.

Member

ak239 commented Aug 4, 2017

Ok, let me split problems: console and non-main context inspection are two different problems.
About CL required for non-main context inspection:
V8 CL is landed in 6.2. It's some kind of revert, since it just restored behavior which exists around 5.8 or 5.9. Unfortunately it's almost impossible to backport it to earlier version of V8 since AFAIK if we backport it to 6.1 or 6.0 then we need to merge it to corresponded Chrome version but this CL is not needed by Chrome (blink calls contextDestroyed method manually and manage context lifetime in different way). So is it possible to backport it only inside of Node.js?

About console:
Console was added in https://codereview.chromium.org/2785293002 and available in V8 since 6.0 so it sounds to me like if node 8 use v8 6.0 then it should always have builtin console object on any context.

@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Aug 9, 2017

Contributor

Per the process, this PR can be landed now (48 hours had passed, there's LGTM). Any last minute objections?

Contributor

eugeneo commented Aug 9, 2017

Per the process, this PR can be landed now (48 hours had passed, there's LGTM). Any last minute objections?

@TimothyGu

My original concern about console has resolved itself in 6.0 it seems.

@Pajn

This comment has been minimized.

Show comment
Hide comment
@Pajn

Pajn Aug 10, 2017

What is the status of this, how long may it take before this and the V8 patch is propagated to a release?
The V8 patch seems to have been reverted as well, which feels less fun. Would be great to get debugging support working with Jest in Node 8

Pajn commented Aug 10, 2017

What is the status of this, how long may it take before this and the V8 patch is propagated to a release?
The V8 patch seems to have been reverted as well, which feels less fun. Would be great to get debugging support working with Jest in Node 8

@TimothyGu TimothyGu referenced this pull request Aug 10, 2017

Closed

[wip] src, inspector: support opted-in VM contexts #14231

0 of 4 tasks complete
@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Aug 10, 2017

Contributor

@Pajn V8 patch is still there :) - v8/v8@f19b889

Contributor

eugeneo commented Aug 10, 2017

@Pajn V8 patch is still there :) - v8/v8@f19b889

@ak239

ak239 approved these changes Aug 10, 2017

@eugeneo eugeneo closed this Aug 11, 2017

@eugeneo eugeneo deleted the eugeneo:contexts-v8 branch Aug 11, 2017

@eugeneo eugeneo merged commit 63fc89a into nodejs:master Aug 11, 2017

@eugeneo

This comment has been minimized.

Show comment
Hide comment
@eugeneo

eugeneo Aug 11, 2017

Contributor

Landed in two commits - cherry pick as
73c59bb and Node side as 63fc89a

Contributor

eugeneo commented Aug 11, 2017

Landed in two commits - cherry pick as
73c59bb and Node side as 63fc89a

icarter09 added a commit to icarter09/node that referenced this pull request Aug 12, 2017

deps: cherry-pick f19b889 from V8 upstream
Original commit message:
	[inspector] support for cases when embedder doesn't call contextDestroyed

	Node.js doesn't have good place to call contextDestroyed.
	We need to cleanup everything on our side to allow clients to not call
	contextDestroyed method.

	R=dgozman@chromium.org,eostroukhov@chromium.com

	Bug: none
	Change-Id: Ibe3f01fd18afbfa579e5db66ab6f174d5fad7c82
	Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_chromium_rel_ng
	Reviewed-on: https://chromium-review.googlesource.com/575519
	Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
	Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
	Cr-Original-Commit-Position: refs/heads/master@{#46849}
	Reviewed-on: https://chromium-review.googlesource.com/596549
	Cr-Commit-Position: refs/heads/master@{#47060}

Ref: https://chromium.googlesource.com/v8/v8.git/+/f19b889be801bdebc04c49090e37c787f7ba8805
PR-URL: nodejs#14465
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>

icarter09 added a commit to icarter09/node that referenced this pull request Aug 12, 2017

inspector: support extra contexts
This enables inspector support for contexts created using the vm module.

PR-URL: nodejs#14465
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>

addaleax added a commit that referenced this pull request Aug 12, 2017

deps: cherry-pick f19b889 from V8 upstream
Original commit message:
	[inspector] support for cases when embedder doesn't call contextDestroyed

	Node.js doesn't have good place to call contextDestroyed.
	We need to cleanup everything on our side to allow clients to not call
	contextDestroyed method.

	R=dgozman@chromium.org,eostroukhov@chromium.com

	Bug: none
	Change-Id: Ibe3f01fd18afbfa579e5db66ab6f174d5fad7c82
	Cq-Include-Trybots: master.tryserver.blink:linux_trusty_blink_rel;master.tryserver.chromium.linux:linux_chromium_rel_ng
	Reviewed-on: https://chromium-review.googlesource.com/575519
	Reviewed-by: Dmitry Gozman <dgozman@chromium.org>
	Commit-Queue: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>
	Cr-Original-Commit-Position: refs/heads/master@{#46849}
	Reviewed-on: https://chromium-review.googlesource.com/596549
	Cr-Commit-Position: refs/heads/master@{#47060}

Ref: https://chromium.googlesource.com/v8/v8.git/+/f19b889be801bdebc04c49090e37c787f7ba8805
PR-URL: #14465
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>

addaleax added a commit that referenced this pull request Aug 12, 2017

inspector: support extra contexts
This enables inspector support for contexts created using the vm module.

PR-URL: #14465
Reviewed-By: Jan Krems <jan.krems@gmail.com>
Reviewed-By: Timothy Gu <timothygu99@gmail.com>
Reviewed-By: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>

@addaleax addaleax referenced this pull request Aug 13, 2017

Merged

v8.4.0 proposal #14811

addaleax added a commit that referenced this pull request Aug 14, 2017

2017-08-15, Version 8.4.0 (Current)
Notable changes

* **HTTP2**
  * Experimental support for the built-in `http2` has been added via the
    `--expose-http2` flag.
    [#14239](#14239)

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](#8837)
  * Multiple contexts, as created by the `vm` module, are supported now.
    [#14465](#14465)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](#14558)

PR-URL: #14811

evanlucas added a commit that referenced this pull request Aug 15, 2017

2017-08-15, Version 8.4.0 (Current)
Notable changes

* **HTTP2**
  * Experimental support for the built-in `http2` has been added via the
    `--expose-http2` flag.
    [#14239](#14239)

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](#8837)
  * Multiple contexts, as created by the `vm` module, are supported now.
    [#14465](#14465)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](#14558)

PR-URL: #14811

MSLaguana added a commit to nodejs/node-chakracore that referenced this pull request Aug 21, 2017

2017-08-15, Version 8.4.0 (Current)
Notable changes

* **HTTP2**
  * Experimental support for the built-in `http2` has been added via the
    `--expose-http2` flag.
    [#14239](nodejs/node#14239)

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](nodejs/node#8837)
  * Multiple contexts, as created by the `vm` module, are supported now.
    [#14465](nodejs/node#14465)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](nodejs/node#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](nodejs/node#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](nodejs/node#14558)

PR-URL: nodejs/node#14811

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 12, 2018

2017-08-15, Version 8.4.0 (Current)
Notable changes

* **HTTP2**
  * Experimental support for the built-in `http2` has been added via the
    `--expose-http2` flag.
    [#14239](nodejs#14239)

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](nodejs#8837)
  * Multiple contexts, as created by the `vm` module, are supported now.
    [#14465](nodejs#14465)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](nodejs#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](nodejs#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](nodejs#14558)

This applies parts of 1fe0741 that are
relevant to N-API.

PR-URL: nodejs#14811

gabrielschulhof added a commit to gabrielschulhof/node that referenced this pull request Mar 15, 2018

2017-08-15, Version 8.4.0 (Current)
Notable changes

* **HTTP2**
  * Experimental support for the built-in `http2` has been added via the
    `--expose-http2` flag.
    [#14239](nodejs#14239)

* **Inspector**
  * `require()` is available in the inspector console now.
    [#8837](nodejs#8837)
  * Multiple contexts, as created by the `vm` module, are supported now.
    [#14465](nodejs#14465)

* **N-API**
  * New APIs for creating number values have been introduced.
    [#14573](nodejs#14573)

* **Stream**
  * For `Duplex` streams, the high water mark option can now be set
    independently for the readable and the writable side.
    [#14636](nodejs#14636)

* **Util**
  * `util.format` now supports the `%o` and `%O` specifiers for printing
    objects.
    [#14558](nodejs#14558)

This applies parts of 1fe0741 that are
relevant to N-API.

PR-URL: nodejs#14811
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment