Bug 155325 – Web Inspector: Debugger: Better expression / statement stepping in the debugger

• Component: Web Inspector
• Status: RESOLVED FIXED

[Keith Miller]

If I have code like:

1) ...
2) foo(0, bar(0));
3) return 1;

If I am on line 1 in the inspector then step to line 2 and step into bar when I step out from inside bar the inspector returns to line 3. It would be nice if the inspector return to line 2 after bar has been executed rather than to line 3.

[Radar WebKit Bug Importer]

<rdar://problem/25094578>

[Keith Miller]

This same behavior happens if I were to step over from the return of bar as well.

[Antoine Quint]

Created attachment 274096 [details]
Testcase

[Antoine Quint]

Created attachment 274097 [details]
Screen recording showing the issue with the attached test case

[Timothy Hatcher]

<rdar://problem/12869638>

[Joseph Pecoraro]

I am using this bug for better debugger stepping in general. Since addressing this and testing it exposed numerous issues the code changes are very small once some general fixes were applied:

  - introduce pausing at certain expression locations (call sites)
    - this will fix step-in/out and stepping through lines with multiple function calls
  - correct, and make consistent, stepping behavior all over the place:
    - if/while/for pause locations (e.g. pause before condition, not before "if")
    - function stepping pause locations (e.g. remove entry pause, consistent exit pause)
    - remove unnecessary pauses (function entry, program entry, throw statement double pause)
  - add tests for this area of code that is severely lacking in tests!

[Joseph Pecoraro]

Created attachment 288776 [details]
[PATCH] Proposed Fix

The actual patch to JavaScriptCore is rather small.
The bulk of this patch is LayoutTests.

[Joseph Pecoraro]

Created attachment 288777 [details]
[TXT] Benchmark Results - Neutral

This adds a new debug hook for function calls. So I measured before and after the change forcing debugger bytecodes enabled to see if the extra op_debugs would affect performance. The results were completely neutral. Full results attached.

Geomean of preferred means:
   <scaled-result>                                  111.1668+-0.2292     ?    111.2490+-0.0709        ? might be 1.0007x slower

[Joseph Pecoraro]

Created attachment 288778 [details]
[PATCH] Proposed Fix

[WebKit Commit Bot]

Attachment 288778 [details] did not pass style-queue:


ERROR: LayoutTests/TestExpectations:135:  "webkit.org/b/161951" is not at the start of the line.  [test/expectations] [5]
Total errors found: 1 in 44 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Build Bot]

Comment on attachment 288778 [details]
[PATCH] Proposed Fix

Attachment 288778 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2070645

New failing tests:
inspector/debugger/tail-deleted-frames-from-vm-entry.html
inspector/debugger/regress-133182.html

[Build Bot]

Created attachment 288782 [details]
Archive of layout-test-results from ews103 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288778 [details]
[PATCH] Proposed Fix

Attachment 288778 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2070656

New failing tests:
inspector/debugger/tail-deleted-frames-from-vm-entry.html
inspector/debugger/regress-133182.html

[Build Bot]

Created attachment 288783 [details]
Archive of layout-test-results from ews105 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 288778 [details]
[PATCH] Proposed Fix

Attachment 288778 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/2070649

New failing tests:
inspector/debugger/tail-deleted-frames-from-vm-entry.html
inspector/debugger/stepping/stepping-function-calls.html
inspector/debugger/regress-133182.html

[Build Bot]

Created attachment 288785 [details]
Archive of layout-test-results from ews113 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Joseph Pecoraro]

Created attachment 288876 [details]
[PATCH] Proposed Fix

Rebaselined the other tests.

[Joseph Pecoraro]

Created attachment 288877 [details]
[PATCH] Proposed Fix

[Blaze Burg]

Comment on attachment 288877 [details]
[PATCH] Proposed Fix

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

> LayoutTests/inspector/debugger/regress-133182.html:66


OK.

> LayoutTests/inspector/debugger/stepping/stepOut-expected.txt:21
> +--- Source Unavailable ---

I would have expected this to stop at the statement after eval, since in a function context we stop at the end of the scope before returning. Wouldn't step-in into an eval stop on the eval'd statement(s)? It seems asymmetrical.

> LayoutTests/inspector/debugger/stepping/stepOut-expected.txt:70
> +     32        })();

This seems to not match behavior of pausing at end of scope in innerFunction.

> LayoutTests/inspector/debugger/stepping/stepOver-expected.txt:168
> + ->  34        }|)();

...I expected stepOut to pause here, like for stepIn/stepOver. Maybe I'm not remembering correctly what I expect to happen.

> LayoutTests/inspector/debugger/stepping/stepping-arrow-functions-expected.txt:103
> +EXPRESSION: setTimeout(entryArrowFunctionParenthesis)

Does this test use template strings? I don't see any...

> LayoutTests/inspector/debugger/stepping/stepping-classes-expected.txt:254
> +--- Source Unavailable ---

Why is the source unavailable?

> LayoutTests/inspector/debugger/stepping/stepping-classes-expected.txt:350
> +

OK.

[Joseph Pecoraro]

Comment on attachment 288877 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/inspector/debugger/stepping/stepOut-expected.txt:21
>> +--- Source Unavailable ---
> 
> I would have expected this to stop at the statement after eval, since in a function context we stop at the end of the scope before returning. Wouldn't step-in into an eval stop on the eval'd statement(s)? It seems asymmetrical.

I agree. I recall there being something weird about this particular case. I'll file a follow-up.

>> LayoutTests/inspector/debugger/stepping/stepOut-expected.txt:70
>> +     32        })();
> 
> This seems to not match behavior of pausing at end of scope in innerFunction.

step-out never pauses when leaving a function. The other steps would (step-in or step-over).

>> LayoutTests/inspector/debugger/stepping/stepOver-expected.txt:168
>> + ->  34        }|)();
> 
> ...I expected stepOut to pause here, like for stepIn/stepOver. Maybe I'm not remembering correctly what I expect to happen.

step-out should require two clicks to leave a function. Its normal behavior is one click takes you out.

>> LayoutTests/inspector/debugger/stepping/stepping-arrow-functions-expected.txt:103
>> +EXPRESSION: setTimeout(entryArrowFunctionParenthesis)
> 
> Does this test use template strings? I don't see any...

Copy&paste error! Good catch. There might be more. Each test name should be unique though.

>> LayoutTests/inspector/debugger/stepping/stepping-classes-expected.txt:254
>> +--- Source Unavailable ---
> 
> Why is the source unavailable?

This is the default constructor JavaScriptCore internally creates:

    function() {
        super(...arguments);
    }

In a real browser we would show that small snippet and "step through it" but that is kinda lame. I have FIXMEs below for this.

[Joseph Pecoraro]

Comment on attachment 288877 [details]
[PATCH] Proposed Fix

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

>>> LayoutTests/inspector/debugger/stepping/stepOver-expected.txt:168
>>> + ->  34        }|)();
>> 
>> ...I expected stepOut to pause here, like for stepIn/stepOver. Maybe I'm not remembering correctly what I expect to happen.
> 
> step-out should require two clicks to leave a function. Its normal behavior is one click takes you out.

Correction: *shouldn't* require two clicks

[Joseph Pecoraro]

Comment on attachment 288877 [details]
[PATCH] Proposed Fix

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

> LayoutTests/inspector/debugger/stepping/stepping-control-flow-expected.txt:26
> +ACTION: step-over
> +PAUSE AT entryIfSingleStatement:17:9
> +     13    
> +     14    function entryIfSingleStatement() {
> +     15        debugger;
> + ->  16        if (|true)
> +     17            a();
> +     18        if (false)
> +     19            a();

I just realized that moving breakpoints from the start of the statement to the condition is an issue with how we handle breakpoints right now.

The frontend sets the breakpoint at the line:column of the start of the statement ("|if") here. By changing the pause locations, JSC doesn't see that, it sees (|true) as the pause location.

There are a few possible solutions:

    1. Quick hack, emit a "WillExecuteStatementForBreakpointCheck" at the start of the statements where breakpoints could be set (if, while, for, throw, label, ...)
    2. Frontend solution: Find an appropriate location for a breakpoint on the line and send it to the backend.
    3. Backend solution: Build up a list of breakpoint, when the frontend says (line:column) slide to the appropriate (realLine:realColumn)

If (1) works I'll do it now and start working on one of the better fixes. (2) has risk because it can go out of date if the backend changes locations. (3) is probably ideal.

Note that (2-3) would also solve problems such as "set a breakpoint on an empty line, or a line with comments => real breakpoint is set on the next statement.

[Blaze Burg]

Comment on attachment 288877 [details]
[PATCH] Proposed Fix

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

Test code looks good. There are some surprising steps that I noted but nothing that should be addressed here. Holding r? for someone to look at JSC changes.

> LayoutTests/inspector/debugger/stepping/stepping-function-calls-expected.txt:763
> + ->  46        add(a(), x = 1, |b());

Oh, we skipped the assignment. Could that trigger a getter we need to step into?

> LayoutTests/inspector/debugger/stepping/stepping-function-calls-expected.txt:994
> + ->  51        |a(), b(), a();

Oh, that's weird. I guess it steps over the statement, but that might not be what people want here.

> LayoutTests/inspector/debugger/stepping/stepping-loops-expected.txt:167
> + ->  27        for (|let i = 0; i < 2; ++i)

Hmmm, don't we evaluate i < 2 before evaluating loop body?

> LayoutTests/inspector/debugger/stepping/stepping-loops-expected.txt:187
> + ->  27        for (let i = 0; i < 2; ++i|)

Hmm, is the column correct here? I would expect stop before ++i

> LayoutTests/inspector/debugger/stepping/stepping-loops-expected.txt:274
> + ->  27        for (let i = 0; |i < 2; ++i)

I see, step-in will evaluate this. Definitely should revisit whether this is expected.

> LayoutTests/inspector/debugger/stepping/stepping-template-string-expected.txt:70
> + ->   7        |return 1;

Errr, what is this stepping into? What's the evaluation order for template strings anyway?

> LayoutTests/inspector/debugger/stepping/stepping-try-catch-finally-expected.txt:174
> + ->  41            throw new Error|;

Hmm, interesting that this is its own step. I would have expected to go straight to the catch.

[Blaze Burg]

Comment on attachment 288877 [details]
[PATCH] Proposed Fix

Bugzilla has a tiny brain.

[Joseph Pecoraro]

Comment on attachment 288877 [details]
[PATCH] Proposed Fix

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

>> LayoutTests/inspector/debugger/stepping/stepping-function-calls-expected.txt:763
>> + ->  46        add(a(), x = 1, |b());
> 
> Oh, we skipped the assignment. Could that trigger a getter we need to step into?

Getters / setters are already a beast to be tackled separately. We will need to specifically handle it in a few ways (step out of a setter seems different from stepping out of a function). That said, no debugger seems to pause on simple assignment right now. I had FIXMEs in the tests themselves saying "opportunity to pause at assignments", but given no debuggers do that it would probably be unexpected for most developers.

>> LayoutTests/inspector/debugger/stepping/stepping-function-calls-expected.txt:994
>> + ->  51        |a(), b(), a();
> 
> Oh, that's weird. I guess it steps over the statement, but that might not be what people want here.

"step-over" should step over the entire statement.

FWIW, I have an ongoing discussion that "step-over" should step over the current LINE (even if there are multiple statements on the line). That is not the current behavior (step-over is statement level, not line level), but step-over going by line is what many users expect because of lldb, gdb. I will look into that later. But I think the behavior here is pretty expected in the comma expression.

>> LayoutTests/inspector/debugger/stepping/stepping-loops-expected.txt:167
>> + ->  27        for (|let i = 0; i < 2; ++i)
> 
> Hmmm, don't we evaluate i < 2 before evaluating loop body?

This is something I cobbled together. It can certainly be tweaked.

For now I figured most users would want a single step-over to enter the loop. So I made that behavior work with some special treatment. step-in will step through both.

I think this will most obviously get cleaned up if we move step-over to really "step over this line".

>> LayoutTests/inspector/debugger/stepping/stepping-loops-expected.txt:187
>> + ->  27        for (let i = 0; i < 2; ++i|)
> 
> Hmm, is the column correct here? I would expect stop before ++i

Correct, this is a bug for a follow-up.

>> LayoutTests/inspector/debugger/stepping/stepping-template-string-expected.txt:70
>> + ->   7        |return 1;
> 
> Errr, what is this stepping into? What's the evaluation order for template strings anyway?

Template strings evaluate like function calls.

Something like:

    let str = `literal ${1} assignment ${x=1} call ${a()} call2 ${|a()}`;
    let str = tag`literal ${1} assignment ${x=1} call ${a()} call2 ${|a()}`;
    let str = tag(["", "literal ", " assignment ", " call ", " call 2", ""], [1, x=1, a(), a()])

So it would be stepping through this:

    magic(1, x=1, a(), a())

>> LayoutTests/inspector/debugger/stepping/stepping-try-catch-finally-expected.txt:174
>> + ->  41            throw new Error|;
> 
> Hmm, interesting that this is its own step. I would have expected to go straight to the catch.

This is the "PauseOnException" test. The "NoPauseOnException" version above does go straight to the catch.

I think i you have PauseOnException enable you will want to pause when the exception happens, hopefully with more info pointing to exactly where the error happened.

We could use this as an opportunity to show an inline error message in the debugger at the pause line.

[Joseph Pecoraro]

Retitling the bug since the fix is more general.

[Joseph Pecoraro]

Created attachment 289905 [details]
[PATCH] Proposed Fix

Updated patch fixed an issue in tests and fixed output location for for..in and for..of loops.

[Build Bot]

Comment on attachment 289905 [details]
[PATCH] Proposed Fix

Attachment 289905 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/2152030

New failing tests:
inspector/debugger/stepping/stepping-loops.html

[Build Bot]

Created attachment 289907 [details]
Archive of layout-test-results from ews102 for mac-yosemite

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-yosemite  Platform: Mac OS X 10.10.5

[Build Bot]

Comment on attachment 289905 [details]
[PATCH] Proposed Fix

Attachment 289905 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/2152027

New failing tests:
inspector/debugger/stepping/stepping-loops.html

[Build Bot]

Created attachment 289908 [details]
Archive of layout-test-results from ews106 for mac-yosemite-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-yosemite-wk2  Platform: Mac OS X 10.10.5

[Joseph Pecoraro]

Apparently I didn't update the results for one of the tests...

[Joseph Pecoraro]

Created attachment 289913 [details]
[PATCH] Proposed Fix

[Mark Lam]

Comment on attachment 289913 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/ChangeLog:28
> +        These pauses are not needed because if there is a statement we

Missing comma between "statement" and "we".

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2754
>          generator.emitInc(i.get());
> +        generator.emitDebugHook(WillExecuteStatement, m_expr->firstLine(), m_expr->startOffset(), m_expr->lineStartOffset());

Don't we always break before an action?  Hence, should this debug hook be before the inc?

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2794
>          generator.emitInc(enumeratorIndex.get());
>          generator.emitEnumeratorStructurePropertyName(propertyName.get(), enumerator.get(), enumeratorIndex.get());
> +        generator.emitDebugHook(WillExecuteStatement, m_expr->firstLine(), m_expr->startOffset(), m_expr->lineStartOffset());

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2831
>          generator.emitInc(enumeratorIndex.get());
>          generator.emitEnumeratorGenericPropertyName(propertyName.get(), enumerator.get(), enumeratorIndex.get());
> +        generator.emitDebugHook(WillExecuteStatement, m_expr->firstLine(), m_expr->startOffset(), m_expr->lineStartOffset());

Ditto.

> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2987
> -    generator.emitDebugHook(WillLeaveCallFrame, lastLine(), startOffset(), lineStartOffset());
> +    generator.emitWillLeaveCallFrameDebugHook();
>      generator.emitReturn(returnRegister.get());

Since emitWillLeaveCallFrameDebugHook() uses the last } of the current scope, will this work in the following scenario:

    function foo() {
        {
            let x = 0;
            return x;
        } // <===== this is the } of the current scope.
    } // <===== we would expect this to be associate with the return.

Or am I mistaken?

[Mark Lam]

Comment on attachment 289913 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2987
>>      generator.emitReturn(returnRegister.get());
> 
> Since emitWillLeaveCallFrameDebugHook() uses the last } of the current scope, will this work in the following scenario:
> 
>     function foo() {
>         {
>             let x = 0;
>             return x;
>         } // <===== this is the } of the current scope.
>     } // <===== we would expect this to be associate with the return.
> 
> Or am I mistaken?

My mistake!  emitWillLeaveCallFrameDebugHook() uses m_scopeNode which is the scope node for the outer most scope.  Hence, my concern is invalid.

[Mark Lam]

Comment on attachment 289913 [details]
[PATCH] Proposed Fix

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

r=me

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2754
>> +        generator.emitDebugHook(WillExecuteStatement, m_expr->firstLine(), m_expr->startOffset(), m_expr->lineStartOffset());
> 
> Don't we always break before an action?  Hence, should this debug hook be before the inc?

Joe explained offline that this is in a for-in loop.  The iterator adjustment here is not visible to the user.  Hence, there is no issue here.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2794
>> +        generator.emitDebugHook(WillExecuteStatement, m_expr->firstLine(), m_expr->startOffset(), m_expr->lineStartOffset());
> 
> Ditto.

No issue here either.

>> Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp:2831
>> +        generator.emitDebugHook(WillExecuteStatement, m_expr->firstLine(), m_expr->startOffset(), m_expr->lineStartOffset());
> 
> Ditto.

Same.  No issue here.

[Joseph Pecoraro]

*** Bug 25734 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

https://trac.webkit.org/changeset/206652

[Joseph Pecoraro]

*** Bug 161721 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

*** Bug 161716 has been marked as a duplicate of this bug. ***

[Joseph Pecoraro]

*** Bug 139523 has been marked as a duplicate of this bug. ***