Bug 195248 - Web Inspector: DOMDebugger: protocol error on first open
Summary: Web Inspector: DOMDebugger: protocol error on first open
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: 195170
Blocks:
  Show dependency treegraph
 
Reported: 2019-03-02 12:44 PST by Devin Rousso
Modified: 2019-03-11 19:54 PDT (History)
6 users (show)

See Also:


Attachments
Patch (32.77 KB, patch)
2019-03-02 15:21 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (34.41 KB, patch)
2019-03-07 01:39 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (34.41 KB, patch)
2019-03-07 01:40 PST, Devin Rousso
no flags Details | Formatted Diff | Diff
Patch (24.34 KB, patch)
2019-03-11 14:29 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-03-02 12:44:08 PST
Controllers/DOMDebuggerManager.js:472:30: CONSOLE ERROR 'DOMDebugger' domain was not found
Comment 1 Radar WebKit Bug Importer 2019-03-02 14:36:31 PST
<rdar://problem/48538465>
Comment 2 Devin Rousso 2019-03-02 15:21:01 PST
Created attachment 363433 [details]
Patch
Comment 3 Joseph Pecoraro 2019-03-06 23:22:58 PST
Comment on attachment 363433 [details]
Patch

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

r- for some multi-target logic. I didn't go in depth after finding a few issues.

The Multimap code looks good, you may want to separate that out into its own patch.

> Source/WebInspectorUI/UserInterface/Base/Multimap.js:65
> +        let deleted = valueSet.delete(value);
> +
> +        // Allow an entire key to be removed by not passing a value.
> +        if (arguments.length === 1 || !valueSet.size)
> +            deleted = this._map.delete(key);

I'd go with:

    // Allow an entire key to be removed by not passing a value.
    if (arguments.length === 1)
        return this._map.delete(key);

    let deleted = valueSet.delete(value);
    if (deleted && !valueSet.size)
        this._map.delete(key);
    return deleted;

Which:

    1. Avoids a `set.delete(undefined)` when arguments.length is 1
    2. Avoids a `map.delete(key)` when valueSet shouldn't be empty
    3. Avoids a duplicate unnecessary assignment to `deleted` when valueSet is empty

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:180
> +            for (let target of WI.targets)
> +                this._updateDOMBreakpoint(breakpoint, target);

This is another where maybe we use WI.assumingMainTarget().

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:211
> +            for (let target of WI.targets)
> +                target.DOMDebuggerAgent.removeDOMBreakpoint(nodeIdentifier, breakpoint.type);

It is not safe to assume the target has a DOMDebuggerAgent (a ServiceWorker target will not). Typically the first thing we *should* do inside of WI.target loops is check for the relevant agent the loop will make use of. Currently many call sites don't because they deal with agents that are guaranteed to exist in all targets (RuntimeAgent, DebuggerAgent, and ConsoleAgent). But like the loop you just added for CanvasAgent and another for HeapAgent checking is the right thing to do.

The pattern I'd suggest is:

    for (let target of WI.targets) {
        if (target.DOMDebuggerAgent) {
            ...
        }
    }

Note that some of your other code gets around this by burying the agent check inside functions, which is fine but tricky to review.

We could add a utility such as:

    for (let target of WI.targetsWith("DOMDebugger")) {
        ...
    }

Which might reduce complexity at sites like this. Then this entire file could follow that pattern.

---

In any case, this specific code (DOM Breakpoints / Node breakpoint) will likely be incorrect for the coming future anyways. A DOMNode / nodeIdentifier will only be valid in a single Target (eventually a Frame target). We probably don't want to tell all targets about a nodeIdentifier that doesn't correspond to them. Ideally in this case the breakpoint would know the target the nodeIdentifier is associated with, so you'd end up with:

    breakpoint.target.removeDOMBreakpoint(nodeIdentifier, breakpoint.type);

So for now maybe convert this one to just:

    // We should get the target associated with the nodeIdentifier in this breakpoint.
    let target = WI.assumingMainTarget();
    target.DOMDebuggerAgent.removeDOMBreakpoint(nodeIdentifier, breakpoint.type);

Put simply:
• Some DOMDebugger breakpoint types (Event, XHR, URL) are universal, and make sense to broadcast messages to all targets with a DOMDebuggerAgent.
• Some DOMDebugger breakpoint types (Node, EventListener) are context specific, and don't make sense to broadcast, but instead should be associated with a single target.

Let me know if that all made sense or if you view this differently.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:285
> +        for (let target of WI.targets) {

Check target.DOMDebuggerAgent.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:355
> +        for (let target of WI.targets) {

Check target.DOMDebuggerAgent.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:449
> -        this._updateDOMBreakpoint(breakpoint);
> +        this._updateDOMBreakpoint(breakpoint, WI.assumingMainTarget());

Lets add a similar comment that ideally we would find the target associated with this target specific DOM breakpoint.

I like the idea of adding comments next to assumingMainTarget so when we come across it later we know why!

> LayoutTests/inspector/unit-tests/multimap.html:86
> +    suite.addTestCase({
> +        name: "Multimap.prototype.delete",
> +        test() {
> +            let multimap = new Multimap;
> +
> +            multimap.add(0, 1);
> +            multimap.add(2, 3);
> +            multimap.add(2, 3);
> +
> +            InspectorTest.log(multimap);
> +
> +            InspectorTest.expectThat(multimap.delete(0, 1), "The key and the value were successfully deleted.");
> +
> +            InspectorTest.log(multimap);
> +
> +            InspectorTest.expectThat(multimap.delete(2, 3), "The key and the value were successfully deleted.");
> +
> +            InspectorTest.log(multimap);
> +        },
> +    });

I'd like to see this test deleting a value from a set that has multiple values.

    multimap.add(0, 1);
    multimap.add(2, 3);
    multimap.add(2, 4);

    log();
    delete(0, 1); log(); // should see `0` keys go away entirely
    delete(2, 3); log(); // should see `[2,4]` still
    delete(2, 4); log(); // should see empty

> LayoutTests/inspector/unit-tests/multimap.html:155
> +            multimap.add("badger", "raccoon");

Would be nice to give this more values.
Comment 4 Devin Rousso 2019-03-07 00:50:35 PST
Comment on attachment 363433 [details]
Patch

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

>> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:211
>> +                target.DOMDebuggerAgent.removeDOMBreakpoint(nodeIdentifier, breakpoint.type);
> 
> It is not safe to assume the target has a DOMDebuggerAgent (a ServiceWorker target will not). Typically the first thing we *should* do inside of WI.target loops is check for the relevant agent the loop will make use of. Currently many call sites don't because they deal with agents that are guaranteed to exist in all targets (RuntimeAgent, DebuggerAgent, and ConsoleAgent). But like the loop you just added for CanvasAgent and another for HeapAgent checking is the right thing to do.
> 
> The pattern I'd suggest is:
> 
>     for (let target of WI.targets) {
>         if (target.DOMDebuggerAgent) {
>             ...
>         }
>     }
> 
> Note that some of your other code gets around this by burying the agent check inside functions, which is fine but tricky to review.
> 
> We could add a utility such as:
> 
>     for (let target of WI.targetsWith("DOMDebugger")) {
>         ...
>     }
> 
> Which might reduce complexity at sites like this. Then this entire file could follow that pattern.
> 
> ---
> 
> In any case, this specific code (DOM Breakpoints / Node breakpoint) will likely be incorrect for the coming future anyways. A DOMNode / nodeIdentifier will only be valid in a single Target (eventually a Frame target). We probably don't want to tell all targets about a nodeIdentifier that doesn't correspond to them. Ideally in this case the breakpoint would know the target the nodeIdentifier is associated with, so you'd end up with:
> 
>     breakpoint.target.removeDOMBreakpoint(nodeIdentifier, breakpoint.type);
> 
> So for now maybe convert this one to just:
> 
>     // We should get the target associated with the nodeIdentifier in this breakpoint.
>     let target = WI.assumingMainTarget();
>     target.DOMDebuggerAgent.removeDOMBreakpoint(nodeIdentifier, breakpoint.type);
> 
> Put simply:
> • Some DOMDebugger breakpoint types (Event, XHR, URL) are universal, and make sense to broadcast messages to all targets with a DOMDebuggerAgent.
> • Some DOMDebugger breakpoint types (Node, EventListener) are context specific, and don't make sense to broadcast, but instead should be associated with a single target.
> 
> Let me know if that all made sense or if you view this differently.

Ah oops!  I was in a bit of a rush/anger to rewrite this, as I had done it once and then accidentally `git checkout`d my repo, which killed all my work :/
Looks like I missed a few things when redoing it :(
Comment 5 Devin Rousso 2019-03-07 01:39:14 PST
Created attachment 363858 [details]
Patch
Comment 6 Devin Rousso 2019-03-07 01:40:09 PST
Created attachment 363859 [details]
Patch

Oops.  Copypasta :P
Comment 7 Joseph Pecoraro 2019-03-11 13:48:23 PDT
Comment on attachment 363859 [details]
Patch

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

r=me

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:180
> +        if (!breakpoint.disabled) {
> +            this._updateDOMBreakpoint(breakpoint, WI.assumingMainTarget());
> +        }

Style: No braces

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:256
> +            for (let target of WI.targets)
> +                this._updateEventBreakpoint(breakpoint, target);

It would make more sense to me to put the if (target.DOMDebuggerAgent) here in this loop then buried in _updateEventBreakpoint. It could be an assert there.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:328
> +            for (let target of WI.targets)
> +                this._updateURLBreakpoint(breakpoint, target);

It would make more sense to me to put the if (target.DOMDebuggerAgent) here in this loop then buried in _updateURLBreakpoint. It could be an assert there.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:542
> +        for (let target of WI.targets)
> +            this._updateDOMBreakpoint(breakpoint, target);

It would make more sense to me to put the if (target.DOMDebuggerAgent) here in this loop then buried in _updateDOMBreakpoint. It could be an assert there.

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:549
> +        for (let target of WI.targets)

Ditto

> Source/WebInspectorUI/UserInterface/Controllers/DOMDebuggerManager.js:561
> +        for (let target of WI.targets)

Ditto
Comment 8 Devin Rousso 2019-03-11 14:29:48 PDT
Created attachment 364288 [details]
Patch
Comment 9 WebKit Commit Bot 2019-03-11 14:56:37 PDT
Comment on attachment 364288 [details]
Patch

Clearing flags on attachment: 364288

Committed r242743: <https://trac.webkit.org/changeset/242743>
Comment 10 WebKit Commit Bot 2019-03-11 14:56:39 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Truitt Savell 2019-03-11 16:54:42 PDT
It appears that the changes in https://trac.webkit.org/changeset/242743/webkit

has caused the test inspector/dom-debugger/dom-breakpoints.html

to become a constant failure. 

History:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=inspector%2Fdom-debugger%2Fdom-breakpoints.html

Diff:
--- /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/inspector/dom-debugger/dom-breakpoints-expected.txt
+++ /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/inspector/dom-debugger/dom-breakpoints-actual.txt
@@ -59,8 +59,6 @@
 PASS: Added 'subtree-modified' breakpoint.
 PASS: Added 'attribute-modified' breakpoint.
 PASS: Added 'node-removed' breakpoint.
-PASS: Removed 3 breakpoints.
-PASS: DOM node should have no breakpoints.
 -- Running test teardown.
 
 -- Running test case: SetBreakpointWithInvalidNodeId
Comment 12 Devin Rousso 2019-03-11 19:54:46 PDT
(In reply to Truitt Savell from comment #11)
> It appears that the changes in
> https://trac.webkit.org/changeset/242743/webkit
> 
> has caused the test inspector/dom-debugger/dom-breakpoints.html
> 
> to become a constant failure. 
> 
> History:
> https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=inspector%2Fdom-debugger%2Fdom-breakpoints.html
> 
> Diff:
> ---
> /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/
> inspector/dom-debugger/dom-breakpoints-expected.txt
> +++
> /Volumes/Data/slave/highsierra-release-tests-wk2/build/layout-test-results/
> inspector/dom-debugger/dom-breakpoints-actual.txt
> @@ -59,8 +59,6 @@
>  PASS: Added 'subtree-modified' breakpoint.
>  PASS: Added 'attribute-modified' breakpoint.
>  PASS: Added 'node-removed' breakpoint.
> -PASS: Removed 3 breakpoints.
> -PASS: DOM node should have no breakpoints.
>  -- Running test teardown.
>  
>  -- Running test case: SetBreakpointWithInvalidNodeId
Landed a followup fix <https://trac.webkit.org/changeset/242765/webkit>.