Bug 164833

Summary: LayoutTest inspector/worker/debugger-pause.html is a flaky failure
Product: WebKit Reporter: Ryan Haddad <ryanhaddad>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: bburg, commit-queue, inspector-bugzilla-changes, joepeck, tsavell, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=188580
https://bugs.webkit.org/show_bug.cgi?id=206285
Attachments:
Description Flags
[PATCH] Proposed Fix none

Ryan Haddad
Reported 2016-11-16 13:48:07 PST
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) { - - -
Attachments
[PATCH] Proposed Fix (3.17 KB, patch)
2018-08-10 16:55 PDT, Joseph Pecoraro 		no flags
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Ryan Haddad
Comment 1 2016-11-16 13:48:49 PST
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
Comment 2 2016-11-16 13:50:39 PST
<rdar://problem/29295404>
Radar WebKit Bug Importer
Comment 3 2016-11-16 13:51:28 PST Comment hidden (obsolete)
Ryan Haddad
Comment 4 2016-11-16 15:32:54 PST
Marked test as flaky in http://trac.webkit.org/projects/webkit/changeset/208820
Joseph Pecoraro
Comment 5 2018-08-10 16:55:28 PDT
Created attachment 346941 [details] [PATCH] Proposed Fix
Blaze Burg
Comment 6 2018-08-10 17:17:34 PDT
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
Comment 7 2018-08-10 17:33:27 PDT
(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 8 2018-08-10 19:21:17 PDT
Comment on attachment 346941 [details] [PATCH] Proposed Fix Clearing flags on attachment: 346941 Committed r234779: <https://trac.webkit.org/changeset/234779>
WebKit Commit Bot
Comment 9 2018-08-10 19:21:19 PDT
All reviewed patches have been landed. Closing bug.
Truitt Savell
Comment 10 2018-08-13 16:51:49 PDT
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
Comment 11 2018-08-14 10:41:35 PDT
(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
Comment 12 2018-08-14 14:09:26 PDT
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
Comment 13 2018-08-14 14:10:57 PDT
Actually even better, I can produce an event when a target has its main resource set. Let me try that.
Joseph Pecoraro
Comment 14 2018-08-14 15:11:10 PDT
(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
Note You need to log in before you can comment on or make changes to this bug.