Bug 200651

Created attachment 376723 [details] Patch

Created attachment 376734 [details] Patch `WI.DebuggerManager` automatically tries to `pause` all other `WI.Target`s when it receives a `Debugger.paused` event. If the initial `Debugger.paused` came from a `Worker`, this would cause the main thread to also become paused. In WK1, this would cause the test to hang because the next time any results would be logged to the page, the debugger would instead pause on that log, which would get out of sync with the test. The fix to this is simply to avoid running any JavaScript on the main thread until after we `Debugger.resume`.

Created attachment 376735 [details] Patch Rebase

Comment on attachment 376735 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376735&action=review r=me. Nice tests! Probably needs a rebaseline. > Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:55 > + if (typeString == "subtree-modified") > + return SubtreeModified; > + if (typeString == "attribute-modified") > + return AttributeModified; > + if (typeString == "node-removed") > + return NodeRemoved; Weird that we can do this comparison and the right hand doesn't need `_s` suffix. I wonder which is more performant? > Source/WebCore/inspector/agents/page/PageDOMDebuggerAgent.cpp:65 > + case SubtreeModified: return "subtree-modified"_s; > + case AttributeModified: return "attribute-modified"_s; > + case NodeRemoved: return "node-removed"_s; Lets fix this style while moving the code. > LayoutTests/inspector/worker/dom-debugger-dom-breakpoints.html:60 > + let listener = WI.targetManager.addEventListener(WI.TargetManager.Event.TargetAdded, (event) => { > + let {target} = event.data; > + if (target.type !== WI.Target.Type.Worker) > + return; > + > + workerTarget = target; > + WI.targetManager.removeEventListener(WI.TargetManager.Event.TargetAdded, listener); > + > + suite.runTestCasesAndFinish(); > + }); > + > + InspectorTest.evaluateInPage(`createWorker()`); I feel like this is a pattern we may end up doing more and more often with worker tests. (I just noticed you do it in each of these tests!) We could turn this into a helper that could even send all the code to the frontend: createWorkerTargetForTests("resources/worker-dom-debugger.js", () => { suite.runTestCasesAndFinish(); }); Or a Promise if desired: createWorkerTargetForTests("resources/worker-dom-debugger.js").then(() => { suite.runTestCasesAndFinish(); }); Implementation could be (untested): TestPage.registerInitializer(() => { window.createWorkerTargetForTests = function(workerScript, callback) { InspectorTest.evaluateInPage(`window[Symbol()] = new Worker(${JSON.stringify(workerScript)});`); let listener = WI.targetManager.addEventListener(WI.TargetManager.Event.TargetAdded, (event) => { let {target} = event.data; if (target.type !== WI.Target.Type.Worker) return; window.workerTarget = target; WI.targetManager.removeEventListener(WI.TargetManager.Event.TargetAdded, listener); callback(); }); } }); Which eliminates the `createWorker` code, `workerTarget` declaration and pretty much all boilerplate in the tests.

Created attachment 377649 [details] Patch

Comment on attachment 377649 [details] Patch Clearing flags on attachment: 377649 Committed r249305: <https://trac.webkit.org/changeset/249305>

All reviewed patches have been landed. Closing bug.

<rdar://problem/54864027>

inspector/dom-debugger/dom-breakpoints.html is failing after this change. Rebaseline? --- /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/inspector/dom-debugger/dom-breakpoints-expected.txt +++ /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/inspector/dom-debugger/dom-breakpoints-actual.txt @@ -115,7 +115,7 @@ -- Running test case: SetBreakpointWithInvalidType Attempting to set breakpoint. -Protocol result: Unknown type: custom-breakpoint-type +Protocol result: Unknown DOM breakpoint type: custom-breakpoint-type PASS: Protocol should return an error. -- Running test teardown. @@ -127,7 +127,7 @@ -- Running test case: RemoveBreakpointWithInvalidType Attempting to remove breakpoint. -Protocol result: Unknown type: custom-breakpoint-type +Protocol result: Unknown DOM breakpoint type: custom-breakpoint-type PASS: Protocol should return an error. -- Running test teardown. https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fdom-debugger%2Fdom-breakpoints.html

(In reply to Ryan Haddad from comment #14) > inspector/dom-debugger/dom-breakpoints.html is failing after this change. Rebaseline? > > --- > /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/inspector/dom-debugger/dom-breakpoints-expected.txt > +++ > /Volumes/Data/slave/mojave-release-tests-wk2/build/layout-test-results/inspector/dom-debugger/dom-breakpoints-actual.txt > @@ -115,7 +115,7 @@ > > -- Running test case: SetBreakpointWithInvalidType > Attempting to set breakpoint. > -Protocol result: Unknown type: custom-breakpoint-type > +Protocol result: Unknown DOM breakpoint type: custom-breakpoint-type > PASS: Protocol should return an error. > -- Running test teardown. > > @@ -127,7 +127,7 @@ > > -- Running test case: RemoveBreakpointWithInvalidType > Attempting to remove breakpoint. > -Protocol result: Unknown type: custom-breakpoint-type > +Protocol result: Unknown DOM breakpoint type: custom-breakpoint-type > PASS: Protocol should return an error. > -- Running test teardown. > > https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fdom-debugger%2Fdom-breakpoints.html Rebased in r249330 <http://trac.webkit.org/r249330>