Bug 161951

Summary: Web Inspector: DebuggerManager.Event.Resumed introduces test flakiness
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, joepeck, keith_miller, mark.lam, mattbaker, msaboff, nvasilyev, ryanhaddad, saam, timothy, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
See Also: https://bugs.webkit.org/show_bug.cgi?id=176071
Bug Depends on:    
Bug Blocks: 163230, 164305    
Attachments:
Description Flags
[PATCH] Proposed Fix bburg: review+

Description Joseph Pecoraro 2016-09-14 00:15:47 PDT
Summary:
DebuggerManager.Event.Resumed introduces test flakiness.

If stepping runs too slowly (such as a Debug backend), the frontend will proactively trigger a Resumed event which may completely break the expected flow of the test.

This should be made deterministic for tests, and if made deterministic can mean the Web Inspector UI can reason about JavaScript microtasks that have completed (allowing it to show the most accurate Pause Reason in the Debugger sidebar).

Steps to Reproduce:
1. shell> run-webkit-tests --debug inspector/debugger/stepping

Notes:
DebuggerManager has this timeout:

    debuggerDidResume()
    {
        // Called from WebInspector.DebuggerObserver.

        // We delay clearing the state and firing events so the user interface does not flash
        // between brief steps or successive breakpoints.
        this._delayedResumeTimeout = setTimeout(this._didResumeInternal.bind(this), 50);
    }
Comment 1 Radar WebKit Bug Importer 2016-09-14 00:16:13 PDT
<rdar://problem/28295767>
Comment 2 Ryan Haddad 2016-10-07 15:18:09 PDT
Marked tests as flaky in http://trac.webkit.org/projects/webkit/changeset/206939
Comment 3 Ryan Haddad 2016-10-13 15:29:15 PDT
One more flaky test that uses DebuggerManager.Event.Resumed https://trac.webkit.org/changeset/207313
Comment 4 Joseph Pecoraro 2016-11-07 13:37:48 PST
*** Bug 164468 has been marked as a duplicate of this bug. ***
Comment 5 Joseph Pecoraro 2016-11-08 14:36:16 PST
Created attachment 294186 [details]
[PATCH] Proposed Fix
Comment 6 Joseph Pecoraro 2016-11-08 14:38:36 PST
Comment on attachment 294186 [details]
[PATCH] Proposed Fix

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

> Source/WebCore/inspector/InspectorClient.cpp:51
> +    SuspendExceptionScope scope(&frontendPage->inspectorController().vm());

@Mark: I didn't add an extra CatchScope and assertion that the inspector didn't produce an exception because it is possible that the inspector page does produce exceptions. We could add it and try to enforce that requirement, but I didn't think having an ASSERT would be any better then our normal debugging handling (UncaughtExceptionReporter + ability to resume using Web Inspector).
Comment 7 Joseph Pecoraro 2016-11-08 14:40:29 PST
With this change I was able to `run-webkit-tests inspector/debugger --time-out-ms=30000 --iterations=5 --force --v` on Release and Debug with no failures (other then 1 pre-existing issue). This was not possible for me before, especially with Debug builds.
Comment 8 BJ Burg 2016-11-08 15:19:35 PST
Comment on attachment 294186 [details]
[PATCH] Proposed Fix

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

r=me, please have mlam or someone glance over JSC parts though.

> LayoutTests/ChangeLog:13
> +        some tests using new style and best practices.

Shouldn't we be unskipping these tests as well, so that they actually run?

> LayoutTests/inspector/debugger/break-on-exception.html:56
> +        expression: `setTimeout(() => { testCatch(); TestPage.dispatchEventToFrontend("AfterTestFunction")})`,

Where is this used in this test? I think we are missing an event listener.

> LayoutTests/inspector/debugger/command-line-api-exception-nested-catch.html:78
> +                    InspectorTest.singleFireEventListener("AfterTestFunction", resolve);

..I see here in this other test we have a listener for it.

> LayoutTests/inspector/debugger/csp-exceptions.html:31
> +                WebInspector.debuggerManager.resume().then(resolve, reject);

Nice.

> LayoutTests/inspector/debugger/pause-reason.html:41
> +                if (setup)

Please propagate 'setup' to the 'setup' parameter, right between 'description' and 'test'.

> LayoutTests/inspector/debugger/paused-scopes.html:91
> +            WebInspector.debuggerManager.singleFireEventListener(WebInspector.DebuggerManager.Event.Paused, (event) => {

Does this need to unpause, or do we do that automatically?

EDIT: oh, the next test assumes it's already paused. I don't like this, but it's hard to do better.

> Source/JavaScriptCore/ChangeLog:8
> +

Might want to put a few sentences here from the bug to describe problem & solution.

> Source/JavaScriptCore/ChangeLog:34
> +        All places that do continueProgram should be audited. In error cases,

Maybe group this comment also with the methods above where it applies, i.e., continueToLocation.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.h:144
> +    enum class ShouldDispatchResumed { No, WhenIdle, WhenContinued };

This read a bit awkwardly to be, but I don't have suggestions :| Maybe the member should be m_conditionToDispatchResumed or something.

> Source/WebInspectorUI/UserInterface/Views/DebuggerSidebarPanel.js:-678
> -        this._debuggerStepOutButtonItem.enabled = indexOfActiveCallFrame < callFrames.length - 1;

I see this FIXME has annoyed you enough times to go ahead and remove it :P
Comment 9 Mark Lam 2016-11-08 16:01:43 PST
Comment on attachment 294186 [details]
[PATCH] Proposed Fix

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

LGTM.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:520
> +        m_frontendDispatcher->resumed();

I think this deserves a comment to explain why you're treating this like a resume instead of a big step.  I don't think it is obvious from just reading this code.
Comment 10 Matt Baker 2016-11-08 19:27:17 PST
Comment on attachment 294186 [details]
[PATCH] Proposed Fix

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

>> Source/JavaScriptCore/ChangeLog:8
>> +
> 
> Might want to put a few sentences here from the bug to describe problem & solution.

Extra newline.

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:529
> +    m_scriptDebugServer.continueProgram();    

Nit: extra whitespace at end of this line
Comment 11 Matt Baker 2016-11-08 19:29:29 PST
Comment on attachment 294186 [details]
[PATCH] Proposed Fix

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

> Source/JavaScriptCore/inspector/agents/InspectorDebuggerAgent.cpp:948
> +        m_frontendDispatcher->resumed();        

Nit: extra whitespace at end of line
Comment 12 Joseph Pecoraro 2016-11-09 22:11:48 PST
<https://trac.webkit.org/changeset/208523>
Comment 13 BJ Burg 2016-12-14 13:19:58 PST
*** Bug 145626 has been marked as a duplicate of this bug. ***