Skip to content
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

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

Closed
wants to merge 1 commit into from

Conversation

develar
Copy link

@develar develar commented Dec 10, 2015

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.

@bnoordhuis
Copy link
Member

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.

@bnoordhuis bnoordhuis added the v8 engine Issues and PRs related to the V8 dependency. label Dec 10, 2015
@rvagg
Copy link
Member

rvagg commented Dec 14, 2015

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!

@develar
Copy link
Author

develar commented Dec 15, 2015

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

@bnoordhuis
Copy link
Member

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.)

@develar
Copy link
Author

develar commented Dec 15, 2015

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))).

@bnoordhuis
Copy link
Member

Okay, understood. Can you update the commit log?

@develar
Copy link
Author

develar commented Dec 16, 2015

Сommit message updated.

@bnoordhuis
Copy link
Member

@bnoordhuis
Copy link
Member

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

@develar
Copy link
Author

develar commented Dec 17, 2015

@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/

@develar
Copy link
Author

develar commented Dec 23, 2015

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

@bnoordhuis
Copy link
Member

@jasnell
Copy link
Member

jasnell commented Jan 15, 2016

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

Vladimir Shutoff <vovan@shutoff.ru>
Yu Yin <xwafish@gmail.com>
Zhongping Wang <kewpie.w.zp@gmail.com>
柳荣一 <admin@web-tinker.com>
柳荣一 <admin@web-tinker.com>

Choose a reason for hiding this comment

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

end line?

Copy link
Author

Choose a reason for hiding this comment

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

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

@bnoordhuis
Copy link
Member

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

@bnoordhuis
Copy link
Member

Ping @jasnell or perhaps @thealphanerd?

@MylesBorins
Copy link
Contributor

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

@rvagg
Copy link
Member

rvagg commented Jan 27, 2016

@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.

@develar
Copy link
Author

develar commented Feb 10, 2016

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.

@develar
Copy link
Author

develar commented Feb 23, 2016

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}
@MylesBorins
Copy link
Contributor

/cc @rvagg

@develar
Copy link
Author

develar commented Mar 14, 2016

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.

@MylesBorins
Copy link
Contributor

@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?

@develar
Copy link
Author

develar commented Mar 14, 2016

@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.

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

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

@MylesBorins MylesBorins self-assigned this Mar 14, 2016
@MylesBorins MylesBorins mentioned this pull request Mar 16, 2016
MylesBorins pushed a commit that referenced this pull request Mar 16, 2016
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>
@MylesBorins
Copy link
Contributor

landed in b6c355d

evanlucas added a commit that referenced this pull request Mar 16, 2016
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
evanlucas added a commit that referenced this pull request Mar 16, 2016
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
@MylesBorins MylesBorins removed their assignment Dec 27, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
semver-minor PRs that contain new features and should be released in the next minor version. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants