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 [email protected] 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 [email protected]
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 [email protected]
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 [email protected]
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 [email protected]
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 <[email protected]> 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#7756Fixes: nodejs#7742 Reviewed-By: Ben Noordhuis <[email protected]> Reviewed-By: Timothy Gu <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]> Reviewed-By: Aleksey Kozyatinskiy <[email protected]>

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 contextDestroyedJul 28, 2016

nojvek changed the title ~~Fixes #7742: src: --inspector notify contextDestroyed~~Fixes #7742: src: --inspect notify contextDestroyedJul 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 c5ce7f4Compare September 21, 2016 00:09

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

MylesBorins force-pushed the master branch from 8df7ee0 to 54fef67Compare 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 fbe946bCompare 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

Uh oh!

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

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

Uh oh!

Conversation

nojvek commented Jul 15, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Affected core subsystem(s)

Description of change

Uh oh!

bnoordhuis commented Jul 16, 2016

Uh oh!

nojvek commented Jul 16, 2016

Uh oh!

nojvek commented Jul 27, 2016• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pavelfeldman commented Jul 27, 2016

Uh oh!

nojvek commented Jul 27, 2016

Uh oh!

nojvek commented Jul 28, 2016

Uh oh!

nojvek commented Jul 28, 2016

Uh oh!

pavelfeldman commented Jul 28, 2016

Uh oh!

ofrobots commented Jul 29, 2016

Uh oh!

nojvek commented Jul 29, 2016

Uh oh!

pavelfeldman commented Jul 29, 2016

Uh oh!

ofrobots commented Aug 8, 2016

Uh oh!

nojvek commented Oct 20, 2016

Uh oh!

jasnell commented Mar 1, 2017

Uh oh!

eugeneo commented Mar 1, 2017 via email

Uh oh!

nojvek commented Mar 1, 2017 via email

Uh oh!

jasnell commented Mar 1, 2017

Uh oh!

auchenberg commented Apr 6, 2017

Uh oh!

eugeneo commented Apr 6, 2017

Uh oh!

nojvek commented May 3, 2017

Uh oh!

eugeneo commented May 3, 2017

Uh oh!

nojvek commented May 3, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

nojvek commented Jul 15, 2016•
edited
Loading

nojvek commented Jul 27, 2016•
edited
Loading