Bug 164833 – LayoutTest inspector/worker/debugger-pause.html is a flaky failure

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

[Ryan Haddad]

LayoutTest inspector/worker/debugger-pause.html is a flaky failure

--- /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/inspector/worker/debugger-pause-expected.txt
+++ /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/inspector/worker/debugger-pause-actual.txt
@@ -1,65 +1,14 @@
-CONSOLE MESSAGE: line 14: TypeError: undefined is not an object (evaluating '[].x.x')
 Ensure we can pause in Workers.
 
+ERROR: Exception during test execution: TypeError: null is not an object (evaluating 'sourceCode.requestContent') loadLinesFromSourceCode
+test
+runTestMethodInFrontend
+eval code
+eval@[native code]
+file:///Volumes/Data/slave/elcapitan-release-tests-wk2/build/WebKitBuild/Release/WebInspectorUI.framework/Resources/TestCombined.js:5689:24
+_flushPendingScripts@file:///Volumes/Data/slave/elcapitan-release-tests-wk2/build/WebKitBuild/Release/WebInspectorUI.framework/Resources/TestCombined.js:5157:24
+_dispatchResponse@file:///Volumes/Data/slave/elcapitan-release-tests-wk2/build/WebKitBuild/Release/WebInspectorUI.framework/Resources/TestCombined.js:5018:38
+dispatch@file:///Volumes/Data/slave/elcapitan-release-tests-wk2/build/WebKitBuild/Release/WebInspectorUI.framework/Resources/TestCombined.js:4955:35
+dispatch@file:///Volumes/Data/slave/elcapitan-release-tests-wk2/build/WebKitBuild/Release/WebInspectorUI.framework/Resources/TestCombined.js:4610:49
+dispatchNextQueuedMessageFromBackend@file:///Volumes/Data/slave/elcapitan-release-tests-wk2/build/WebKitBuild/Release/WebInspectorUI.framework/Resources/TestCombined.js:5445:34
 
-== Running test suite: Worker.Debugger.Pause
--- Running test case: Worker.Debugger.Pause.DebuggerStatement
-PASS: Paused
-PASS: Should be paused in a Worker CallFrame.
-PASS: Pause reason should be a debugger statement.
-PAUSE AT triggerDebuggerStatement:3:5
-      0    function triggerDebuggerStatement() {
-      1        let before = 1;
- ->   2        |debugger;
-      3        let after = 2;
-      4    }
-      5    
-
-
-
--- Running test case: Worker.Debugger.Pause.Breakpoint
-PASS: Paused
-PASS: Should be paused in a Worker CallFrame.
-PASS: Pause reason should be a breakpoint.
-PAUSE AT triggerBreakpoint:9:5
-      5    
-      6    function triggerBreakpoint() {
-      7        let alpha = 1;
- ->   8        |let beta = 2; // BREAKPOINT
-      9        let gamma = 3;
-     10    }
-     11    
-
-
-
--- Running test case: Worker.Debugger.Pause.Exception
-PASS: Paused
-PASS: Should be paused in a Worker CallFrame.
-PASS: Pause reason should be an exception.
-PAUSE AT triggerException:14:9
-     10    }
-     11    
-     12    function triggerException() {
- ->  13        [].x|.x;
-     14    }
-     15    
-     16    function triggerAssertion() {
-
-
-Uncaught exception in test page: TypeError: undefined is not an object (evaluating '[].x.x') [worker-debugger-pause.js:14]
-
--- Running test case: Worker.Debugger.Pause.Assert
-PASS: Paused
-PASS: Should be paused in a Worker CallFrame.
-PASS: Pause reason should be an exception.
-PAUSE AT triggerAssertion:18:19
-     14    }
-     15    
-     16    function triggerAssertion() {
- ->  17        console.assert|(false, "Assertion");
-     18    }
-     19    
-     20    onmessage = function(event) {
-
-
-

[Ryan Haddad]

Here is an instance with a different diff:

+FAIL: Main Thread's work fired before it could pause. Failing early.

https://build.webkit.org/results/Apple%20Yosemite%20Debug%20WK2%20(Tests)/r208798%20(16260)/results.html

[Radar WebKit Bug Importer]

<rdar://problem/29295404>

[Radar WebKit Bug Importer]

<rdar://problem/29295420>

[Ryan Haddad]

Marked test as flaky in http://trac.webkit.org/projects/webkit/changeset/208820

[Joseph Pecoraro]

Created attachment 346941 [details]
[PATCH] Proposed Fix

[Blaze Burg]

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

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

r=me

> LayoutTests/inspector/worker/debugger-pause.html:115
> +    Promise.resolve().then(() => {

It would be interesting to abstract away the Promise mechanism to turn the event loop. Say, turnEventLoop().then(... would make the comment disappear.

Is this something we should be doing in more worker tests? Would it be less fragile for the worker to post back a "I'm initialized!" message then continue the test? I think we use this pattern in Inspector layout test runner between the inspector and inspected page.

[Joseph Pecoraro]

(In reply to Brian Burg from comment #6)
> Comment on attachment 346941 [details]
> [PATCH] Proposed Fix
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=346941&action=review
> 
> r=me
> 
> > LayoutTests/inspector/worker/debugger-pause.html:115
> > +    Promise.resolve().then(() => {
> 
> It would be interesting to abstract away the Promise mechanism to turn the
> event loop. Say, turnEventLoop().then(... would make the comment disappear.
> 
> Is this something we should be doing in more worker tests? Would it be less
> fragile for the worker to post back a "I'm initialized!" message then
> continue the test? I think we use this pattern in Inspector layout test
> runner between the inspector and inspected page.

This one isn't the worker, its the frontend setting up the worker. Which is a shame.


I like the idea of turnEventLoop(), but it still would probably need a comment explains why.

[WebKit Commit Bot]

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

Clearing flags on attachment: 346941

Committed r234779: <https://trac.webkit.org/changeset/234779>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

It looks like this patch <https://trac.webkit.org/changeset/234779>
Did alleviate the failures in the test. Now the test is timing out and has had two crashes though those may be a persisting issue. 


Test History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fworker%2Fdebugger-pause.html

[Joseph Pecoraro]

(In reply to Truitt Savell from comment #10)
> It looks like this patch <https://trac.webkit.org/changeset/234779>
> Did alleviate the failures in the test. Now the test is timing out and has
> had two crashes though those may be a persisting issue. 
> 
> 
> Test History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=inspector%2Fworker%2Fdebugger-pause.html

Thanks for the heads up, looking.

[Joseph Pecoraro]

I was able to reproduce this 1 out of 100 times in a debug build on my desktop. By doing:

    $ run-webkit-tests inspector/worker --force --verbose --iterations=100 --exit-after-n-failures=1

In that case, the worker main resource wasn't available.

So while it seems the vast majority of the time 1 run loop turn is a long enough wait, sometimes it may not be.

I'm going to try this instead:

    1. Wait 1 turn
    2. If not available, wait for some amount of time (100ms? 1s?) before giving up.
    3. If not available, fail immediately instead of timeout

With this lets see what happens on the bots.

[Joseph Pecoraro]

Actually even better, I can produce an event when a target has its main resource set. Let me try that.

[Joseph Pecoraro]

(In reply to Joseph Pecoraro from comment #13)
> Actually even better, I can produce an event when a target has its main
> resource set. Let me try that.

https://bugs.webkit.org/show_bug.cgi?id=188580