`Worker`s can fire events, and therefore would benefit from event breakpoints.
Created attachment 376723 [details] Patch
This patch modifies the inspector protocol. Please ensure that any frontend changes appropriately use feature checks for new protocol features.
Comment on attachment 376723 [details] Patch Attachment 376723 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12943401 New failing tests: inspector/worker/dom-debugger-event-listener-breakpoints.html inspector/worker/dom-debugger-event-interval-breakpoints.html inspector/worker/dom-debugger-event-timeout-breakpoints.html inspector/worker/dom-debugger-url-breakpoints.html
Created attachment 376728 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6
Comment on attachment 376723 [details] Patch Attachment 376723 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12943549 New failing tests: inspector/worker/dom-debugger-event-listener-breakpoints.html inspector/worker/dom-debugger-event-interval-breakpoints.html inspector/worker/dom-debugger-event-timeout-breakpoints.html inspector/worker/dom-debugger-url-breakpoints.html
Created attachment 376730 [details] Archive of layout-test-results from ews114 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews114 Port: mac-highsierra Platform: Mac OS X 10.13.6
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>