Bug 161721 - Web Inspector: Stepping all the way through program should not cause a pause on the next program that executes
Summary: Web Inspector: Stepping all the way through program should not cause a pause ...
Status: RESOLVED DUPLICATE of bug 155325
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Joseph Pecoraro
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-09-07 17:23 PDT by Joseph Pecoraro
Modified: 2016-09-30 12:36 PDT (History)
11 users (show)

See Also:


Attachments
[PATCH] Proposed Fix (14.33 KB, patch)
2016-09-07 19:07 PDT, Joseph Pecoraro
no flags Details | Formatted Diff | Diff
[PATCH] Proposed Fix (14.98 KB, patch)
2016-09-07 19:21 PDT, Joseph Pecoraro
mattbaker: review+
joepeck: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joseph Pecoraro 2016-09-07 17:23:11 PDT
Summary:
Stepping all the way through program should not cause a pause on the next program that executes 

Test:
1. <script>
2. setTimeout(() => {
3.     console.log("here");
4. }, 3000);
5. debugger;
6. </script>

Steps to Reproduce:
1. Inspect test page
2. Reload
  => Pause on debugger statement
3. Step in until the program completes
4. Wait a few seconds
  => Unexpected pause on timer firing, or other code executing in the page

Notes:
- Stepping through a program should not affect the next program that runs
- Chrome does pause at the next program execution
- Firefox does not pause at the next program execution
Comment 1 Radar WebKit Bug Importer 2016-09-07 17:24:00 PDT
<rdar://problem/28198547>
Comment 2 Joseph Pecoraro 2016-09-07 18:51:17 PDT
One possible location where this would be useful is stepping across microtasks:

For example, "step-into" a line with setTimeout would eventually pause when the timeout fires:

    debugger;
 >  setTimeout(() => {
       alert("here");
    });
    ...

Or a Promise constructor to step into the function:

    debugger;
 >  new Promise((resolve, reject) => {
       alert("here");
    });
    ...

This would work right now with current step-into behavior if you step-into all the way out of the first program.

But if there is anything in the "..." location (or parent call frames), then this is super bad to rely on "step-into" to trigger this. I'd rather this be a separate command, "step-follow" or "step-through" which recognizes that there is a callback and pauses inside it.

I'm going to drop this behavior from step-into. It is too arbitrary as is.
Comment 3 Joseph Pecoraro 2016-09-07 19:07:25 PDT
Created attachment 288224 [details]
[PATCH] Proposed Fix
Comment 4 Joseph Pecoraro 2016-09-07 19:21:55 PDT
Created attachment 288225 [details]
[PATCH] Proposed Fix
Comment 5 Joseph Pecoraro 2016-09-07 21:23:08 PDT
Comment on attachment 288225 [details]
[PATCH] Proposed Fix

View in context: https://bugs.webkit.org/attachment.cgi?id=288225&action=review

> LayoutTests/inspector/debugger/stepInto.html:54
> +    // Always step-in when call frames change.

This comment should be "step-into".

> LayoutTests/inspector/debugger/stepOver.html:54
> +    // Always step-in when call frames change.

This comment should be "step-over".
Comment 6 Matt Baker 2016-09-08 11:48:47 PDT
Comment on attachment 288225 [details]
[PATCH] Proposed Fix

r=me.

Maybe there should be a change log comment making it more clear that the change to Debugger.cpp alone isn't enough to fix the issue. On first read it wasn't clear that InspectorDOMDebuggerAgent::stepInfo was causing the next call to pauseOnNativeEventIfNeeded to break in the debugger.
Comment 7 BJ Burg 2016-09-08 11:59:19 PDT
View in context: https://bugs.webkit.org/attachment.cgi?id=288225&action=review

r=me too

> LayoutTests/inspector/debugger/stepInto.html:19
> +    testAlert("log 1");

Please update this to "log from inside entry2" so that the expected output is more comprehensible. I thought it was code accidentally left in from debugging.

> LayoutTests/inspector/debugger/stepOver-expected.txt:6
> +-- Running test case: Debugger.stepOver.statements

This test is very nice.
Comment 8 Joseph Pecoraro 2016-09-30 12:36:40 PDT
Fixed as part of bug 155325:
https://trac.webkit.org/changeset/206652

*** This bug has been marked as a duplicate of bug 155325 ***