Bug 145090 – Web Inspector: TimelineAgent needs to handle nested runloops

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Matt Baker]

* SUMMARY
TimelineAgent needs to handle nested runloops.

Pausing script execution in the Web Inspector starts a nested runloop, which causes the TimelineAgent to pop the current rendering frame record before the outer runloop completes. We should track the runloop nesting level, and only push/pop rendering frame records when the stack depth is zero.

* STEPS TO REPRODUCE
Run the following inspector test: LayoutTests/inspector/timeline/debugger-paused-while-recording.html

[Radar WebKit Bug Importer]

<rdar://problem/20986375>

[Matt Baker]

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

[Matt Baker]

Created attachment 253329 [details]
[Patch] Proposed Fix

[Build Bot]

Comment on attachment 253329 [details]
[Patch] Proposed Fix

Attachment 253329 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5030757328224256

New failing tests:
platform/mac/fast/text/font-weights.html

[Build Bot]

Created attachment 253333 [details]
Archive of layout-test-results from ews101 for mac-mavericks

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

[Joseph Pecoraro]

Comment on attachment 253329 [details]
[Patch] Proposed Fix

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

A subtle comment, otherwise I'm fine with this.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:170
> +        m_runLoopNestingLevel--;
> +        if (!m_runLoopNestingLevel)
> +            didCompleteCurrentRecord(TimelineRecordType::RenderingFrame);

I think this should be wrapped in a block / comment to prevent decrementing the counter below 0:

    // Avoid dropping below 0. If we are already at 0, then we are leaving a nested run loop we were not aware of.
    if (m_runLoopNestingLevel) {
        m_runLoopNestingLevel--;
        if (!m_runLoopNestingLevel)
            didCompleteCurrentRecord(TimelineRecordType::RenderingFrame);
    }

See comment below for how I think this might happen.  Normally this would be an ASSERT, but I don't think this is actually something we can know, so we just need to handle whatever happens gracefully.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:179
> +    m_runLoopNestingLevel = 1;

This deserves a comment. We are assuming we are in a single run loop, and not currently running in a nested run loop. I'm not sure if that is technically accurate.

For instance, would our nesting level be non-1 if we:

  1. Pause on a breakpoint
  2. Start a Timeline Recording (thus triggering InspectorTimelineAgent::internalStart)

I think then, the StopRunLoop handler will still decrement m_runLoopNestingLevel and likely end up in problematic territory.

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:205
> +    if (m_runLoopNestingLevel) {
> +        m_runLoopNestingLevel = 0;

No need for the if, just an assignment is clearer.

[Mark Lam]

Matt, I skipped tests in r185259: <http://trac.webkit.org/r185259> which are blocked on this bug.

You might want to take a look at those tests and see if they already satisfy the testing requirement for your patch.  If so, you can just unskip the tests and land your patch.

[Matt Baker]

Comment on attachment 253329 [details]
[Patch] Proposed Fix

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

>> Source/WebCore/inspector/InspectorTimelineAgent.cpp:170
>> +            didCompleteCurrentRecord(TimelineRecordType::RenderingFrame);
> 
> I think this should be wrapped in a block / comment to prevent decrementing the counter below 0:
> 
>     // Avoid dropping below 0. If we are already at 0, then we are leaving a nested run loop we were not aware of.
>     if (m_runLoopNestingLevel) {
>         m_runLoopNestingLevel--;
>         if (!m_runLoopNestingLevel)
>             didCompleteCurrentRecord(TimelineRecordType::RenderingFrame);
>     }
> 
> See comment below for how I think this might happen.  Normally this would be an ASSERT, but I don't think this is actually something we can know, so we just need to handle whatever happens gracefully.

It shouldn't be possible for the nesting level to drop below zero, but I'll add an ASSERT as the same. See comment below.

>> Source/WebCore/inspector/InspectorTimelineAgent.cpp:179
>> +    m_runLoopNestingLevel = 1;
> 
> This deserves a comment. We are assuming we are in a single run loop, and not currently running in a nested run loop. I'm not sure if that is technically accurate.
> 
> For instance, would our nesting level be non-1 if we:
> 
>   1. Pause on a breakpoint
>   2. Start a Timeline Recording (thus triggering InspectorTimelineAgent::internalStart)
> 
> I think then, the StopRunLoop handler will still decrement m_runLoopNestingLevel and likely end up in problematic territory.

The runloop nesting level is relative to the runloop from which the debugging runloop executes. In the above example internalStart is executing in a nested runloop, but as long as the nesting level isn't updated while paused in the debugger the nesting level won't drop below zero.

[Matt Baker]

(In reply to comment #7)
> Matt, I skipped tests in r185259: <http://trac.webkit.org/r185259> which are
> blocked on this bug.
> 
> You might want to take a look at those tests and see if they already satisfy
> the testing requirement for your patch.  If so, you can just unskip the
> tests and land your patch.

Looks like we don't need a separate test for this. Tests which execute the debugger will cover nested runloop cases.

[Matt Baker]

(In reply to comment #9)
> (In reply to comment #7)
> > Matt, I skipped tests in r185259: <http://trac.webkit.org/r185259> which are
> > blocked on this bug.
> > 
> > You might want to take a look at those tests and see if they already satisfy
> > the testing requirement for your patch.  If so, you can just unskip the
> > tests and land your patch.
> 
> Looks like we don't need a separate test for this. Tests which execute the
> debugger will cover nested runloop cases.

I meant to say "Inspector tests which exercise the debugger". The tests in <http://trac.webkit.org/r185259> meet that requirement.

[Matt Baker]

Created attachment 255228 [details]
[Patch] Proposed Fix

[Matt Baker]

Created attachment 255232 [details]
[Patch] Proposed Fix

[Joseph Pecoraro]

Comment on attachment 255232 [details]
[Patch] Proposed Fix

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

Much better! r=me

> LayoutTests/TestExpectations:-527
> -webkit.org/b/145090 inspector/debugger/break-on-exception.html [ Skip ]

I'm not sure you can unstop tests. That would be nice but they are frequently flakey, likely an issue in the test harness.

[Mark Lam]

(In reply to comment #13)
> I'm not sure you can unstop tests. That would be nice but they are
> frequently flakey, likely an issue in the test harness.

There is still a global skip of inspector/debugger (I think) fir the flakiness issue.  So unskipping these specific tests is the right thing to do (since they were blocked on this bug).

[Matt Baker]

(In reply to comment #14)
> (In reply to comment #13)
> > I'm not sure you can unstop tests. That would be nice but they are
> > frequently flakey, likely an issue in the test harness.
> 
> There is still a global skip of inspector/debugger (I think) fir the
> flakiness issue.  So unskipping these specific tests is the right thing to
> do (since they were blocked on this bug).

Correct. From TestExpectations:
92: # These tests still mysteriously flake and/or time out.
93: webkit.org/b/137131 inspector/debugger [ Skip ]

[Joseph Pecoraro]

Comment on attachment 255232 [details]
[Patch] Proposed Fix

Oops. kk!

[WebKit Commit Bot]

Comment on attachment 255232 [details]
[Patch] Proposed Fix

Clearing flags on attachment: 255232

Committed r185777: <http://trac.webkit.org/changeset/185777>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.