Bug 195248 – Web Inspector: DOMDebugger: protocol error on first open

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Devin Rousso]

Controllers/DOMDebuggerManager.js:472:30: CONSOLE ERROR 'DOMDebugger' domain was not found

[Radar WebKit Bug Importer]

<rdar://problem/48538465>

[Devin Rousso]

Created attachment 363433 [details]
Patch

[Joseph Pecoraro]

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.

[Devin Rousso]

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 :(

[Devin Rousso]

Created attachment 363858 [details]
Patch

[Devin Rousso]

Created attachment 363859 [details]
Patch

Oops.  Copypasta :P

[Joseph Pecoraro]

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

[Devin Rousso]

Created attachment 364288 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 364288 [details]
Patch

Clearing flags on attachment: 364288

Committed r242743: <https://trac.webkit.org/changeset/242743>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Truitt Savell]

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

[Devin Rousso]

(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>.