Bug 200651 - Web Inspector: DOMDebugger: support event breakpoints in Worker contexts
Summary: Web Inspector: DOMDebugger: support event breakpoints in Worker contexts
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Devin Rousso
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-08-12 18:19 PDT by Devin Rousso
Modified: 2019-08-30 10:02 PDT (History)
13 users (show)

See Also:


Attachments
Patch (93.63 KB, patch)
2019-08-19 17:17 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-highsierra (3.35 MB, application/zip)
2019-08-19 18:02 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews114 for mac-highsierra (3.23 MB, application/zip)
2019-08-19 19:04 PDT, Build Bot
no flags Details
Patch (97.70 KB, patch)
2019-08-19 20:04 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (95.91 KB, patch)
2019-08-19 20:07 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (94.68 KB, patch)
2019-08-29 16:16 PDT, Devin Rousso
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Devin Rousso 2019-08-12 18:19:24 PDT
`Worker`s can fire events, and therefore would benefit from event breakpoints.
Comment 1 Devin Rousso 2019-08-19 17:17:50 PDT
Created attachment 376723 [details]
Patch
Comment 2 Build Bot 2019-08-19 17:19:42 PDT Comment hidden (obsolete)
Comment 3 Build Bot 2019-08-19 18:02:24 PDT Comment hidden (obsolete)
Comment 4 Build Bot 2019-08-19 18:02:26 PDT Comment hidden (obsolete)
Comment 5 Build Bot 2019-08-19 19:04:12 PDT Comment hidden (obsolete)
Comment 6 Build Bot 2019-08-19 19:04:15 PDT Comment hidden (obsolete)
Comment 7 Devin Rousso 2019-08-19 20:04:54 PDT
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`.
Comment 8 Devin Rousso 2019-08-19 20:07:06 PDT
Created attachment 376735 [details]
Patch

Rebase
Comment 9 Joseph Pecoraro 2019-08-29 11:37:12 PDT
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.
Comment 10 Devin Rousso 2019-08-29 16:16:27 PDT
Created attachment 377649 [details]
Patch
Comment 11 WebKit Commit Bot 2019-08-29 18:08:10 PDT
Comment on attachment 377649 [details]
Patch

Clearing flags on attachment: 377649

Committed r249305: <https://trac.webkit.org/changeset/249305>
Comment 12 WebKit Commit Bot 2019-08-29 18:08:11 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Radar WebKit Bug Importer 2019-08-29 18:09:29 PDT
<rdar://problem/54864027>
Comment 14 Ryan Haddad 2019-08-30 09:51:29 PDT
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
Comment 15 Devin Rousso 2019-08-30 10:02:07 PDT
(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>