Bug 161951 – Web Inspector: DebuggerManager.Event.Resumed introduces test flakiness

• Component: Web Inspector
• Status: RESOLVED FIXED

[Joseph Pecoraro]

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);
    }

[Radar WebKit Bug Importer]

<rdar://problem/28295767>

[Ryan Haddad]

Marked tests as flaky in http://trac.webkit.org/projects/webkit/changeset/206939

[Ryan Haddad]

One more flaky test that uses DebuggerManager.Event.Resumed https://trac.webkit.org/changeset/207313

[Joseph Pecoraro]

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

[Joseph Pecoraro]

Created attachment 294186 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

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).

[Joseph Pecoraro]

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.

[Blaze Burg]

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

[Mark Lam]

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.

[Matt Baker]

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

[Matt Baker]

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

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/208523>

[Blaze Burg]

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