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

Step out/Shift F11/Out debug command does not stop when exception is thrown inside a function #9175

Closed
bikashmishra100 opened this issue Oct 19, 2016 · 7 comments
Labels
confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.

Comments

@bikashmishra100
Copy link

Version: v6.8.1
Platform: Windows 10 64 bit

function exception_function() {
    try {
        throw 'error';
    } catch (err) {
        console.log(err);
    }
}
console.log('Starting script.');
exception_function();
console.log('Stopping script.');

When i am inside the exception_function in the debugger and execute the out function, it does not stop any more. If i do single step or step over all the way then it works fine.

< Debugger listening on [::]:5858
connecting to 127.0.0.1:5858 ... ok
break in C:\Users\xxxx\Desktop\exception.js:10
  8 }
  9
>10 console.log('Starting script.');
 11 exception_function();
 12 console.log('Stopping script.');
debug> next
< Starting script.
break in C:\Users\xxxx\Desktop\exception.js:11
  9
 10 console.log('Starting script.');
>11 exception_function();
 12 console.log('Stopping script.');
 13
debug> step
break in C:\Users\xxxx\Desktop\exception.js:4
  2 function exception_function() {
  3     try {
> 4         throw 'error';
  5     } catch (err) {
  6         console.log(err);
debug> out
< error
< Stopping script.
debug> quit
@bnoordhuis bnoordhuis added confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency. labels Oct 22, 2016
@bnoordhuis
Copy link
Member

I can confirm the issue and it also happens with master.

It works when you run breakOnException before out. You end up in the catch block and can then run out again to step out of the function.

It seems V8 clears the one-shot breakpoints that out adds when an exception is thrown. This patch should fix it:

diff --git a/deps/v8/src/debug/debug.cc b/deps/v8/src/debug/debug.cc
index e046957..803fd4a 100644
--- a/deps/v8/src/debug/debug.cc
+++ b/deps/v8/src/debug/debug.cc
@@ -920,17 +920,17 @@ void Debug::PrepareStepInSuspendedGenerator() {
   clear_suspended_generator();
 }

 void Debug::PrepareStepOnThrow() {
   if (!is_active()) return;
   if (last_step_action() == StepNone) return;
   if (in_debug_scope()) return;

-  ClearOneShot();
+  if (last_step_action() != StepOut) ClearOneShot();

   // Iterate through the JavaScript stack looking for handlers.
   JavaScriptFrameIterator it(isolate_);
   while (!it.done()) {
     JavaScriptFrame* frame = it.frame();
     if (frame->LookupExceptionHandlerInTable(nullptr, nullptr) > 0) break;
     it.Advance();
   }

cc @nodejs/v8-inspector - I know the old debugger is being subsumed by the V8 inspector. Is it still useful to send this upstream and if so, how should I approach regression tests?

@ofrobots
Copy link
Contributor

/cc @hashseed.

@hashseed
Copy link
Member

hashseed commented Oct 25, 2016

Corresponding V8 bug: https://bugs.chromium.org/p/v8/issues/detail?id=5559
Fix: https://codereview.chromium.org/2445233004
@ofrobots please label the V8 bug accordingly if you plan to merge.

targos added a commit to targos/node that referenced this issue Jan 8, 2017
Original commit message:

  fix stepping out of across throwing.

  [email protected]
  BUG=v8:5559

  Review-Url: https://codereview.chromium.org/2445233004
  Cr-Commit-Position: refs/heads/master@{nodejs#40549}

Fixes: nodejs#9175
targos added a commit to targos/node that referenced this issue Jan 8, 2017
Original commit message:

  fix stepping out of across throwing.

  [email protected]
  BUG=v8:5559

  Review-Url: https://codereview.chromium.org/2445233004
  Cr-Commit-Position: refs/heads/master@{nodejs#40549}

Fixes: nodejs#9175
@targos
Copy link
Member

targos commented Jan 8, 2017

Cherry-pick to v7.x: #10688
Cherry-pick to v6.x: #10689

@ofrobots it seems the fix has not been merged in 5.5. Is it still possible to do it?

@ofrobots
Copy link
Contributor

Merge CL for 5.5 here: https://codereview.chromium.org/2621883003. Will land it upstream once approved.

@ofrobots
Copy link
Contributor

Merged as 5.5.372.39 upstream.

jasnell pushed a commit that referenced this issue Jan 10, 2017
Original commit message:

  fix stepping out of across throwing.

  [email protected]
  BUG=v8:5559

  Review-Url: https://codereview.chromium.org/2445233004
  Cr-Commit-Position: refs/heads/master@{#40549}

PR-URL: #10689
Fixes: #9175
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
targos added a commit that referenced this issue Jan 17, 2017
Original commit message:

  fix stepping out of across throwing.

  [email protected]
  BUG=v8:5559

  Review-Url: https://codereview.chromium.org/2445233004
  Cr-Commit-Position: refs/heads/master@{#40549}

PR-URL: #10688
Fixes: #9175
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 24, 2017
Original commit message:

  fix stepping out of across throwing.

  [email protected]
  BUG=v8:5559

  Review-Url: https://codereview.chromium.org/2445233004
  Cr-Commit-Position: refs/heads/master@{#40549}

PR-URL: #10689
Fixes: #9175
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
MylesBorins pushed a commit that referenced this issue Jan 31, 2017
Original commit message:

  fix stepping out of across throwing.

  [email protected]
  BUG=v8:5559

  Review-Url: https://codereview.chromium.org/2445233004
  Cr-Commit-Position: refs/heads/master@{#40549}

PR-URL: #10689
Fixes: #9175
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Ali Ijaz Sheikh <[email protected]>
Reviewed-By: Michael Dawson <[email protected]>
@bnoordhuis
Copy link
Member

Closing, fixed and released.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
confirmed-bug Issues with confirmed bugs. v8 engine Issues and PRs related to the V8 dependency.
Projects
None yet
Development

No branches or pull requests

6 participants