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

5.x v8 debugger has broken 'restartframe' command #5221

Closed
3y3 opened this issue Feb 13, 2016 · 7 comments
Closed

5.x v8 debugger has broken 'restartframe' command #5221

3y3 opened this issue Feb 13, 2016 · 7 comments
Labels
v8 engine Issues and PRs related to the V8 dependency.

Comments

@3y3
Copy link

3y3 commented Feb 13, 2016

Is is broken in this line https://github.com/nodejs/node/blob/master/deps/v8/src/debug/debug.js#L2417

There is no RestartFrame func

@bnoordhuis
Copy link
Member

Can you report that here? I expect that the fix looks like this:

diff --git a/deps/v8/src/debug/debug.js b/deps/v8/src/debug/debug.js
index 50bd0a9..493527a 100644
--- a/deps/v8/src/debug/debug.js
+++ b/deps/v8/src/debug/debug.js
@@ -2414,7 +2414,7 @@ DebugCommandProcessor.prototype.restartFrameRequest_ = function(
     frame_mirror = this.exec_state_.frame();
   }

-  var result_description = Debug.LiveEdit.RestartFrame(frame_mirror);
+  var result_description = frame_mirror.restart();
   response.body = {result: result_description};
 };

We can cherry-pick it once it lands upstream.

See here you want to work on it yourself. If you run into difficulties, let me know and I'll walk you through the process.

@bnoordhuis bnoordhuis added the v8 engine Issues and PRs related to the V8 dependency. label Feb 14, 2016
ghost pushed a commit to facebookarchive/nuclide that referenced this issue Mar 22, 2016
Summary:This greatly simplifies the RN node executor.

Previously, messages from the RN app would be translated by `ChildManager` into calls of `Child` methods, which would then create its own message format to send to the executor. Responsibilities for handling these messages were divided between all three. After this change, the RN message format is maintained all the way to executor. Not only does this remove unnecessary transformations, it also makes things a lot easier to understand for people familiar with RN already.

In addition, I've moved the script loading into the executor. This results in a clearer division of responsibilities, with RN sending and receiving messages to the executor and Nuclide only being there to facilitate the transfer. With that in mind, I was able to replace `Child` with a function (`executeRnRequests`) that accepts a stream of RN messages and outputs a stream of results. Since this sums up all of Nuclide's responsibilities here, I'd like to remove `ChildManager` entirely (it's now just an OO interface to `executeRnRequests`) but it's currently being used by the `ExecutorServer`, so I'll hold off until we're able to remove that in favor of the new `DebuggerProxyClient`.

This refactor also allowed me to switch from calling `process._debugProcess` on a running process to starting it with `--debug-brk` which fixed the issue with the debugger not skipping the loader breakpoint when using node 5.x (since "restartframe" is no longer called?see nodejs/node#5221).

public

Reviewed By: ssorallen

Differential Revision: D3073832

fb-gh-sync-id: 3604c2d19c5ab336b854766ff10c5a77695bade5
shipit-source-id: 3604c2d19c5ab336b854766ff10c5a77695bade5
@matthewwithanm
Copy link

@bnoordhuis Can this be cherry-picked now? Fix here.

@MylesBorins
Copy link
Contributor

I'll give it a go

MylesBorins pushed a commit to MylesBorins/node that referenced this issue Apr 6, 2016
As requested in nodejs#5221

Original commit message:

  fix debug command processor wrt restart frame.

  R=jkummerow@chromium.org
  BUG=v8:4757
  LOG=N

  Review URL: https://codereview.chromium.org/1700693002

  Cr-Commit-Position: refs/heads/master@{nodejs#33983}
jasnell pushed a commit that referenced this issue Apr 7, 2016
As requested in #5221

Original commit message:

  fix debug command processor wrt restart frame.

  R=jkummerow@chromium.org
  BUG=v8:4757
  LOG=N

  Review URL: https://codereview.chromium.org/1700693002

  Cr-Commit-Position: refs/heads/master@{#33983}

PR-URL: #6086
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

@matthewloring this landed in 937ac37

@matthewloring
Copy link

Redirecting mention to @matthewwithanm

@MylesBorins
Copy link
Contributor

whoops.. that's what I get for going on autopilot

@matthewloring
Copy link

No worries 😄

MylesBorins pushed a commit that referenced this issue Apr 19, 2016
As requested in #5221

Original commit message:

  fix debug command processor wrt restart frame.

  R=jkummerow@chromium.org
  BUG=v8:4757
  LOG=N

  Review URL: https://codereview.chromium.org/1700693002

  Cr-Commit-Position: refs/heads/master@{#33983}

PR-URL: #6086
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Apr 20, 2016
As requested in #5221

Original commit message:

  fix debug command processor wrt restart frame.

  R=jkummerow@chromium.org
  BUG=v8:4757
  LOG=N

  Review URL: https://codereview.chromium.org/1700693002

  Cr-Commit-Position: refs/heads/master@{#33983}

PR-URL: #6086
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue Apr 20, 2016
As requested in #5221

Original commit message:

  fix debug command processor wrt restart frame.

  R=jkummerow@chromium.org
  BUG=v8:4757
  LOG=N

  Review URL: https://codereview.chromium.org/1700693002

  Cr-Commit-Position: refs/heads/master@{#33983}

PR-URL: #6086
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
jasnell pushed a commit that referenced this issue Apr 26, 2016
As requested in #5221

Original commit message:

  fix debug command processor wrt restart frame.

  R=jkummerow@chromium.org
  BUG=v8:4757
  LOG=N

  Review URL: https://codereview.chromium.org/1700693002

  Cr-Commit-Position: refs/heads/master@{#33983}

PR-URL: #6086
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue May 3, 2016
As requested in #5221

Original commit message:

  fix debug command processor wrt restart frame.

  R=jkummerow@chromium.org
  BUG=v8:4757
  LOG=N

  Review URL: https://codereview.chromium.org/1700693002

  Cr-Commit-Position: refs/heads/master@{#33983}

PR-URL: #6086
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue May 6, 2016
As requested in #5221

Original commit message:

  fix debug command processor wrt restart frame.

  R=jkummerow@chromium.org
  BUG=v8:4757
  LOG=N

  Review URL: https://codereview.chromium.org/1700693002

  Cr-Commit-Position: refs/heads/master@{#33983}

PR-URL: #6086
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this issue May 18, 2016
As requested in #5221

Original commit message:

  fix debug command processor wrt restart frame.

  R=jkummerow@chromium.org
  BUG=v8:4757
  LOG=N

  Review URL: https://codereview.chromium.org/1700693002

  Cr-Commit-Position: refs/heads/master@{#33983}

PR-URL: #6086
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Michaël Zasso <mic.besace@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

6 participants