v8 remote debug: export BreakEvent, BreakPoint and CompileEvent by develar · Pull Request #4231 · nodejs/node · GitHub

JetBrains debugger overrides BreakEvent/CompileEvent.toJSONProtocol implementation and it works in the previous version of V8 (nodejs v4).

Corresponding V8 review: https://codereview.chromium.org/1477233002/

V8 team will accept my patch, but unlikely that V8 will be updated in Nodejs 5.x, right? So, I created this pull request. It is required to fix https://youtrack.jetbrains.com/issue/WEB-16397 not only for Nodejs 0.12.x and 4.x, but for 5.x too.

Can you request a back-port to the 4.7 branch when your CL lands? You can do so with the tools/release/merge_to_branch.py script, it will cherry-pick the commit and file a CL for you.

If upstream rejects it, I'll be happy to land this patch, but it's worth giving it a shot.

lgtm

@bnoordhuis this PR is against v5.x so it should be merged regardless of whether it gets backported to V8 4.7 right?

@develar thanks for the fix. I think this is your first contribution to core, if so, welcome on board!

CL landed (https://chromium.googlesource.com/v8/v8/+/b201a7b93f35a3d66c319038d0f3419c7bd935cc) I will try to back-port to the 4.7 branch.

this PR is against v5.x so it should be merged regardless of whether it gets backported to V8 4.7 right?

True enough. The change itself LGTM but the commit log should follow the common template for cherry-picked-from-upstream patches. git log deps/v8 will show examples.

It is required to fix https://youtrack.jetbrains.com/issue/WEB-16397 not only for Nodejs 0.12.x and 4.x, but for 5.x too.

v0.12 might be problematic. This patch is logically a semver-minor (because it adds to the debug API) but I don't think we really have a semver policy for v0.12 at the moment.

(Speaking for myself, that doesn't bother me and I'm fine landing it in v0.12. It would need to be back-ported though.)

v0.12 might be problematic

It works in 0.12 and 4.0 (any version < 5.x). Broken only since 5.0 (due to V8 update). So, I ask you to apply this patch only to 5.x branch (I suppose, nodejs 6.x+ will use new version of V8 (since my patch in the V8 master))).

Okay, understood. Can you update the commit log?

Сommit message updated.

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

As unlikely as it sounds, this PR seems to break cross-compiling for the Raspberry Pis: https://ci.nodejs.org/job/node-cross-compile/796/nodes=cross-compiler-pi1p/console

@bnoordhuis May be I am wrong, but it seems now builds are ok — https://ci.nodejs.org/job/node-cross-compile/803/ (started by me) and yours https://ci.nodejs.org/job/node-cross-compile/802/

Should I do something? I want to stop user complaints about #3875 (comment) ("Uncheck ‘js.debugger.v8.use.any.breakpoint’.")

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

The last CI on this appears to have died... Here's another: https://ci.nodejs.org/job/node-test-pull-request/1243/

end line?

Was landed in V8 master so. My fault. Should I fix it?

CI was green. @jasnell You want to land this on v5.x?

Ping @jasnell or perhaps @thealphanerd?

i think we should wait until the current security push is finished before landing this. Thoughts @rvagg @Fishrock123 ?

@thealphanerd yes, good call, I've been cherry-picking for v5.x since the last release but avoiding semver-minor commits and this is borderline so lets sit on it till next week.

I hope this fix will be included into v5.7 because there is a major regression #3875 and until real fix is not done, it can help a bit.

Just a friendly ping :)

Original commit message: Export BreakEvent and CompileEvent R=yangguo@chromium.org Review URL: https://codereview.chromium.org/1477233002 Cr-Commit-Position: refs/heads/master@{nodejs#32861}

/cc @rvagg

Sorry, but is there any chance that this simple 3-lines patch will be finally landed in the 5.x?

A lot of people want to debug babel code (transpiled on the fly) and use NodeJS 5, not NodeJS 4.
And a lot of people blame us that I've spent hour and hours trying various things to get it to work; but it just will not correctly pick up the inline sourcemaps.

@develar did you get the patch merged onto v8? Can you share a link? I'll bring this up with people in the morning to see it land.

to clarify... things are only regressing on v5 and up?

@thealphanerd yes, 3 months ago.
V8 review: https://codereview.chromium.org/1477233002/
V8 commit: https://chromium.googlesource.com/v8/v8/+/b201a7b93f35a3d66c319038d0f3419c7bd935cc

to clarify... things are only regressing on v5 and up?

Yes, because in the v5+ most of the V8 debugger API requires explicit export.

Here is the patch over on github: https://github.com/v8/v8/commit/b201a7b93f35a3d66c319038d0f3419c7bd935cc.patch

CI one more time to be safe: https://ci.nodejs.org/job/node-test-pull-request/1917/

If CI passes this has a LGTM from me. As it appears that both @rvagg and @bnoordhuis have given an LGTM in the past and both of their concerns have been met

I just marked it semver-minor as per the current conversations. @Fishrock123 how long until the next minor release of 5.0? I would potentially argue this should just go in as a patch as it fixes expected behavior that is working in other release lines at the moment.

@develar thank you for being patient through all of this, the back to back security releases caused a bit of mayhem, I don't expect you will be waiting much longer for this

Original commit message: Export BreakEvent and CompileEvent R=yangguo@chromium.org Review URL: https://codereview.chromium.org/1477233002 Cr-Commit-Position: refs/heads/master@{#32861} PR-URL: #4231 Reviewed-By: Rod Vagg <rod@vagg.org> Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Myles Borins <myles.borins@gmail.com>

landed in b6c355d

Notable changes: * **contextify**: Fixed a memory consumption issue related to heavy use of `vm.createContext` and `vm.runInNewContext`. (Ali Ijaz Sheikh) #5392 * **governance**: The following members have been added as collaborators: - Andreas Madsen (@AndreasMadsen) - Benjamin Gruenbaum (@benjamingr) - Claudio Rodriguez (@claudiorodriguez) - Glen Keane (@thekemkid) - Jeremy Whitlock (@whitlockjc) - Matt Loring (@matthewloring) - Phillip Johnsen (@phillipj) * **lib**: copy arguments object instead of leaking it (Nathan Woltman) #4361 * **src**: allow combination of -i and -e cli flags (Rich Trott) #5655 * **v8**: backport fb4ccae from v8 upstream (Vladimir Krivosheev) #4231 - breakout events from v8 to offer better support for external debuggers * **zlib**: add support for concatenated members (Kári Tristan Helgason) #5120 PR-URL: #5702

bnoordhuis added the v8 engine Issues and PRs related to the V8 dependency. label Dec 10, 2015

mscdex added the debugger label Dec 10, 2015

develar force-pushed the patch-1 branch from fba9766 to 2ed9a61 Compare December 15, 2015 17:26

bnoordhuis mentioned this pull request Dec 16, 2015

Add v5.x to REBASE_ONTO dropdown nodejs/build#280

Closed

develar mentioned this pull request Dec 23, 2015

WebStorm debugger is extremely slow to start w/ Node.js 5.0.0 #3875

Closed

ArnonHongklay reviewed Jan 15, 2016
View reviewed changes

develar mentioned this pull request Jan 26, 2016

Relative SourceMap paths for debugging in WebStorm webpack/webpack#238

Closed

MylesBorins added dont-land-on-v5.x and removed dont-land-on-v5.x labels Jan 26, 2016

develar mentioned this pull request Jan 30, 2016

Debugger support avajs/ava#342

Closed

rvagg force-pushed the v5.x branch from 63e314a to 9dcc45e Compare February 9, 2016 12:08

deps: backport fb4ccae from v8 upstream

1231bf5

Original commit message: Export BreakEvent and CompileEvent R=yangguo@chromium.org Review URL: https://codereview.chromium.org/1477233002 Cr-Commit-Position: refs/heads/master@{nodejs#32861}

develar force-pushed the patch-1 branch from 2ed9a61 to 1231bf5 Compare February 29, 2016 09:41

bnoordhuis added the land-on-v5.x label Mar 1, 2016

MylesBorins added the semver-minor PRs that contain new features and should be released in the next minor version. label Mar 14, 2016

MylesBorins self-assigned this Mar 14, 2016

MylesBorins mentioned this pull request Mar 16, 2016

v5.9.0 proposal #5702

Merged

MylesBorins closed this Mar 16, 2016

MylesBorins removed their assignment Dec 27, 2016

v8 remote debug: export BreakEvent, BreakPoint and CompileEvent by develar · Pull Request #4231 · nodejs/node · GitHub

Uh oh!

Uh oh!

v8 remote debug: export BreakEvent, BreakPoint and CompileEvent - #4231

v8 remote debug: export BreakEvent, BreakPoint and CompileEvent#4231develar wants to merge 1 commit intonodejs:v5.xnodejs/node:v5.xfrom develar:patch-1develar/node:patch-1Copy head branch name to clipboard

Conversation

develar commented Dec 10, 2015

Uh oh!

bnoordhuis commented Dec 10, 2015

Uh oh!

rvagg commented Dec 14, 2015

Uh oh!

develar commented Dec 15, 2015

Uh oh!

bnoordhuis commented Dec 15, 2015

Uh oh!

develar commented Dec 15, 2015

Uh oh!

bnoordhuis commented Dec 15, 2015

Uh oh!

develar commented Dec 16, 2015

Uh oh!

bnoordhuis commented Dec 16, 2015

Uh oh!

bnoordhuis commented Dec 16, 2015

Uh oh!

develar commented Dec 17, 2015

Uh oh!

develar commented Dec 23, 2015

Uh oh!

bnoordhuis commented Jan 3, 2016

Uh oh!

jasnell commented Jan 15, 2016

Uh oh!

ArnonHongklay Jan 15, 2016

Choose a reason for hiding this comment

Uh oh!

develar Jan 26, 2016

Choose a reason for hiding this comment

Uh oh!

bnoordhuis commented Jan 17, 2016

Uh oh!

bnoordhuis commented Jan 26, 2016

Uh oh!

MylesBorins commented Jan 26, 2016

Uh oh!

rvagg commented Jan 27, 2016

Uh oh!

develar commented Feb 10, 2016

Uh oh!

develar commented Feb 23, 2016

Uh oh!

MylesBorins commented Mar 1, 2016

Uh oh!

develar commented Mar 14, 2016

Uh oh!

MylesBorins commented Mar 14, 2016

Uh oh!

develar commented Mar 14, 2016

Uh oh!

MylesBorins commented Mar 14, 2016

Uh oh!

MylesBorins commented Mar 16, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

v8 remote debug: export BreakEvent, BreakPoint and CompileEvent#4231
develar wants to merge 1 commit into
nodejs:v5.xfrom
develar:patch-1