Fixes #7742: src: --inspect notify contextDestroyed #7756

nojvek · 2016-07-15T21:45:58Z

Checklist

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

Affected core subsystem(s)

inspector

Description of change

When node --inspect is run, Runtime.contextDestroyed is fired before debugger goes into a wait loop.

This signals the client that it can stop any profilers if they were running and disconnect

/cc @ofrobots @pavelfeldman

I'm not familiar with v8 internals. What is the best way for V8Inspector to hold a context reference so it gets nicely garbage collected ?

I tried using a v8::Localv8::Context* but that didn't work.
…

bnoordhuis · 2016-07-16T08:50:58Z

Note also that the commit log should confirm to the guidelines from CONTRIBUTING.md.

nojvek · 2016-07-16T15:06:05Z

The first line should be 50 characters or less and contain a short
description of the change prefixed with the name of the changed subsystem
(e.g. "net: add localAddress and localPort to Socket").

Got it.

I'll change the format.

On Saturday, July 16, 2016, Ben Noordhuis notifications@github.com wrote:

Note also that the commit log should confirm to the guidelines from
CONTRIBUTING.md.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#7756 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA-JVJsoEjsCRFcDSLUVDeMlaVbzuryKks5qWJumgaJpZM4JNz9x
.

nojvek · 2016-07-27T17:31:38Z

@pavelfeldman / @ofrobots ,

It seems the v8inspector update from blink landed today but it doesn't have the notifyContextDestroyed.

Do you want me to send a patch to chromium repo ?

How often do you bring changes from chromium down to nodejs ?

pavelfeldman · 2016-07-27T17:37:22Z

We should now land change to V8Inspector (if that is still necessary) into
chromium and follow-up in node.

On Wed, Jul 27, 2016 at 10:32 AM, Manoj Patel notifications@github.com
wrote:

@pavelfeldman https://github.com/pavelfeldman / @ofrobots
https://github.com/ofrobots ,

It seems the v8inspector update from blink landed today but it doesn't
have the notifyContextDestroyed.

Do you want me to send a patch to chromium repo ?

Not sure when you plan next to bring minor updates back to nodejs repo ?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7756 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA19BcRB3Akl8Pki803NMxT59DaieGsbks5qZ5YzgaJpZM4JNz9x
.

nojvek · 2016-07-27T17:41:22Z

Okay I'll send a patch to chromium tonight.

On Wed, Jul 27, 2016 at 10:38 AM, Pavel Feldman notifications@github.com
wrote:

We should now land change to V8Inspector (if that is still necessary) into
chromium and follow-up in node.

On Wed, Jul 27, 2016 at 10:32 AM, Manoj Patel notifications@github.com
wrote:

@pavelfeldman https://github.com/pavelfeldman / @ofrobots
https://github.com/ofrobots ,

It seems the v8inspector update from blink landed today but it doesn't
have the notifyContextDestroyed.

Do you want me to send a patch to chromium repo ?

Not sure when you plan next to bring minor updates back to nodejs repo ?

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
#7756 (comment), or
mute
the thread
<
https://github.com/notifications/unsubscribe-auth/AA19BcRB3Akl8Pki803NMxT59DaieGsbks5qZ5YzgaJpZM4JNz9x

.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#7756 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA-JVGV7U1mXh_XAfpt5oqPhqIZJ-sSlks5qZ5eNgaJpZM4JNz9x
.

When node script execution finishes, inspector sends Runtime.contextDestroyed event over websocket. V8Inspector changes will be also sent to chromium repo.

nojvek · 2016-07-28T06:40:17Z

See https://codereview.chromium.org/2188723004/

nojvek · 2016-07-28T23:21:21Z

Landed in chromium.

@pavelfeldman, when do you expect v8 to land in nodejs next?

Regards.

pavelfeldman · 2016-07-28T23:41:47Z

@eugeneo, @ofrobots should we roll weekly?

ofrobots · 2016-07-29T16:33:53Z

Weekly sounds like a good cadence to start with; we can adjust as we go. The rolling process is actually quite simple (collaboration welcome):

PR to update third_party/v8_inspector in https://github.com/pavelfeldman/v8_inspector from Chromium upstream.
PR to update deps/v8_inspector in Node.

nojvek · 2016-07-29T19:35:12Z

I'm not sure If I fully understand the process.

Is the pavelfeldman/v8_inspector automatically updated from chromium
sources nightly ?

I know https://github.com/ChromeDevTools/devtools-frontend is a mirror of a
chromium branch. Should pfeldman/v8_inspector be something similar?

The PR to update deps/v8_inspector is basically a diff with the chromium
repo right?

Regards.

On Fri, Jul 29, 2016 at 9:34 AM, Ali Ijaz Sheikh notifications@github.com
wrote:

Weekly sounds like a good cadence to start with; we can adjust as we go.
The rolling process is actually quite simple (collaboration welcome):

PR to update third_party/v8_inspector in
https://github.com/pavelfeldman/v8_inspector from Chromium upstream.

PR to update deps/v8_inspector in Node.

—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#7756 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/AA-JVOUdo_9anC0vNDXGCtUJOkrZA46Mks5qaiurgaJpZM4JNz9x
.

pavelfeldman · 2016-07-29T22:07:36Z

There is no process yet, we are talking about establishing it...

ofrobots · 2016-08-08T15:01:07Z

#8014

nojvek · 2016-10-20T16:38:05Z

This has been open for a while and it seems V8Inspector.cpp is moved. I'll update the PR again.

jasnell · 2017-03-01T01:43:35Z

@nodejs/v8-inspector ... is this still necessary?

eugeneo · 2017-03-01T01:52:29Z

Yes. But the implementation will be much easier once we have inspector agent always created (once inspector becomes a handler for the debug signal).

…

On Tue, Feb 28, 2017 at 5:43 PM James M Snell ***@***.***> wrote: @nodejs/v8-inspector <https://github.com/orgs/nodejs/teams/v8-inspector> ... is this still necessary? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#7756 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AARkraDOrHbvy2pqk6E-iGWVzk9eD55Sks5rhM1LgaJpZM4JNz9x> .

nojvek · 2017-03-01T04:50:01Z

I had a working implementation once. But we were waiting for v8 changes to land in node. By the time they landed underlying API had changed. At that time inspect was still considered unstable so this was put on hold. But as Eugene points out, it's not an easy fix. On Tue, Feb 28, 2017 at 5:53 PM Eugene Ostroukhov <notifications@github.com> wrote:

…

Yes. But the implementation will be much easier once we have inspector agent always created (once inspector becomes a handler for the debug signal). On Tue, Feb 28, 2017 at 5:43 PM James M Snell ***@***.***> wrote: > @nodejs/v8-inspector <https://github.com/orgs/nodejs/teams/v8-inspector> > ... is this still necessary? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#7756 (comment)>, or mute > the thread > < https://github.com/notifications/unsubscribe-auth/AARkraDOrHbvy2pqk6E-iGWVzk9eD55Sks5rhM1LgaJpZM4JNz9x > > . > — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#7756 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AA-JVEy7puBHz_ThzN46mGckGBUi7OyGks5rhM9-gaJpZM4JNz9x> .

jasnell · 2017-03-01T05:38:11Z

Ok, will keep this open then.

auchenberg · 2017-04-06T17:59:05Z

Has this landed with the recent V8_inspector upgrades?

eugeneo · 2017-04-06T18:51:02Z

Not yet. Will need to be rebased/redone.

nojvek · 2017-05-03T17:54:29Z

@eugeneo any updates on this?

eugeneo · 2017-05-03T19:03:13Z

@nojvek I created a new PR (#12814) as I don't think I can contribute to this PR.

nojvek · 2017-05-03T21:40:42Z

Closing since @eugeneo has a newer PR that fixes this.

PR-URL: nodejs#12814 Reimplements: nodejs#7756 Fixes: nodejs#7742 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Aleksey Kozyatinskiy <kozyatinskiy@chromium.org>

nodejs-github-bot added c++ Issues and PRs that require attention from people who are familiar with C++. inspector Issues and PRs related to the V8 inspector protocol labels Jul 15, 2016

src: --inspector notify contextDestroyed

57bbbfe

When node script execution finishes, inspector sends Runtime.contextDestroyed event over websocket. V8Inspector changes will be also sent to chromium repo.

nojvek changed the title ~~Fixes #7742: node --inspect should fire Runtime.executionContextDestoyed when script finishes executing~~ Fixes #7742: src: --inspector notify contextDestroyed Jul 28, 2016

nojvek changed the title ~~Fixes #7742: src: --inspector notify contextDestroyed~~ Fixes #7742: src: --inspect notify contextDestroyed Jul 28, 2016

nojvek mentioned this pull request Jul 28, 2016

node --inspect fire Runtime.executionContextDestroyed when script finishes executing #7742

Closed

Trott force-pushed the master branch from b0df363 to c5ce7f4 Compare September 21, 2016 00:09

rvagg force-pushed the master branch 2 times, most recently from c133999 to 83c7a88 Compare October 18, 2016 17:02

MylesBorins force-pushed the master branch from 8df7ee0 to 54fef67 Compare February 1, 2017 01:00

jasnell added the stalled Issues and PRs that are stalled. label Mar 1, 2017

refack force-pushed the master branch from 16073c0 to fbe946b Compare April 14, 2017 04:11

eugeneo mentioned this pull request May 3, 2017

inspector: report when main context is destroyed #12814

Merged

3 tasks

nojvek closed this May 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #7742: src: --inspect notify contextDestroyed #7756

Fixes #7742: src: --inspect notify contextDestroyed #7756

nojvek commented Jul 15, 2016 •

edited

Loading

bnoordhuis commented Jul 16, 2016

nojvek commented Jul 16, 2016

nojvek commented Jul 27, 2016 •

edited

Loading

pavelfeldman commented Jul 27, 2016

nojvek commented Jul 27, 2016

nojvek commented Jul 28, 2016

nojvek commented Jul 28, 2016

pavelfeldman commented Jul 28, 2016

ofrobots commented Jul 29, 2016

nojvek commented Jul 29, 2016

pavelfeldman commented Jul 29, 2016

ofrobots commented Aug 8, 2016

nojvek commented Oct 20, 2016

jasnell commented Mar 1, 2017

eugeneo commented Mar 1, 2017 via email

nojvek commented Mar 1, 2017 via email

jasnell commented Mar 1, 2017

auchenberg commented Apr 6, 2017

eugeneo commented Apr 6, 2017

nojvek commented May 3, 2017

eugeneo commented May 3, 2017

nojvek commented May 3, 2017

Fixes #7742: src: --inspect notify contextDestroyed #7756

Fixes #7742: src: --inspect notify contextDestroyed #7756

Conversation

nojvek commented Jul 15, 2016 • edited Loading

Checklist

Affected core subsystem(s)

Description of change

bnoordhuis commented Jul 16, 2016

nojvek commented Jul 16, 2016

nojvek commented Jul 27, 2016 • edited Loading

pavelfeldman commented Jul 27, 2016

nojvek commented Jul 27, 2016

nojvek commented Jul 28, 2016

nojvek commented Jul 28, 2016

pavelfeldman commented Jul 28, 2016

ofrobots commented Jul 29, 2016

nojvek commented Jul 29, 2016

pavelfeldman commented Jul 29, 2016

ofrobots commented Aug 8, 2016

nojvek commented Oct 20, 2016

jasnell commented Mar 1, 2017

eugeneo commented Mar 1, 2017 via email

nojvek commented Mar 1, 2017 via email

jasnell commented Mar 1, 2017

auchenberg commented Apr 6, 2017

eugeneo commented Apr 6, 2017

nojvek commented May 3, 2017

eugeneo commented May 3, 2017

nojvek commented May 3, 2017

nojvek commented Jul 15, 2016 •

edited

Loading

nojvek commented Jul 27, 2016 •

edited

Loading