Bug 148223 – Web Inspector: add a regression test for the fix introduced in r188679

• Component: Web Inspector
• Status: RESOLVED FIXED

[Blaze Burg]

Here's the key trick:

    function listenerRecieversWithPrototype(proto, emitter) {
        let results = new Set;
        for (let eventType in emitter._listeners) {
            let recordsForEvent = emitter._listeners[eventType];
            for (let listener of recordsForEvent) {
                if (listener.thisObject instanceof proto)
                    results.add(listener.thisObject);
            }
        }
        return results;
    }

We can check that the number of instances stays the same across reloads or navigations between simple pages.

[Radar WebKit Bug Importer]

<rdar://problem/22362146>

[Blaze Burg]

Created attachment 259480 [details]
Proposed Fix

I manually verified that the last assertion in the test fails prior to the mentioned revision.

[Joseph Pecoraro]

Comment on attachment 259480 [details]
Proposed Fix

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

Keeping r? regarding the possibly unexpected change in behavior.

> Source/WebInspectorUI/UserInterface/Models/Frame.js:341
> +        this._detachFromParentFrame();

This doesn't sound right to me.

Currently if a MainFrame reloads, DOMManager should send out a DocumentUpdated event and the DOMTree associated with the MainFrame will clear and re-request its root DOM node. I don't understand it all, but it seems keeping the DOMTree of the MainFrame, but removing all the child frames, should be fine without this step.

That said, you did point out an interesting case on IRC. If there is a n>2 depth tree, this doesn't seem to recursively go through and disconnect child frames, and that should likely happen.

> LayoutTests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation-expected.txt:14
> +PASS: There should not be a DOMTree listening to DOMTreeManager events after a main frame navigation.

I'm not sure this really matters. Currently, we create a new FrameDOMTreeContentView across reloads but we continue to use the MainFrame's DOMTree. The changes you've made in this patch may be for the better though. I just want to make sure we've through through them all.

Is DOM.documentUpdated still needed? Maybe for overwriting the DOM, something like replacing the document, without an explicit navigation?

> LayoutTests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html:10
> +    function listenerRecieversWithPrototype(proto, emitter) {

Typo: "Recievers" => "Receivers"

> LayoutTests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html:18
> +        for (let eventType in emitter._listeners) {
> +            let recordsForEvent = emitter._listeners[eventType];
> +            for (let listener of recordsForEvent) {
> +                if (listener.thisObject instanceof proto)
> +                    results.add(listener.thisObject);
> +            }
> +        }

This, looking into the internals of WebInspector.Object kinda irks me and was why I didn't write a test in the first place.

That said, I have a patch that moves _listeners to a Symbol, and that feels a bit better than this if we use a well defined symbol.

[Blaze Burg]

Comment on attachment 259480 [details]
Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Models/Frame.js:341
>> +        this._detachFromParentFrame();
> 
> This doesn't sound right to me.
> 
> Currently if a MainFrame reloads, DOMManager should send out a DocumentUpdated event and the DOMTree associated with the MainFrame will clear and re-request its root DOM node. I don't understand it all, but it seems keeping the DOMTree of the MainFrame, but removing all the child frames, should be fine without this step.
> 
> That said, you did point out an interesting case on IRC. If there is a n>2 depth tree, this doesn't seem to recursively go through and disconnect child frames, and that should likely happen.

In my understanding, re-using the DOMTree instance in some cases (reload) doesn't really buy us anything, nor does forcing a new one to be created cause problems. It would be great to have this straightforward, no-exceptions story for the lifetimes of DOMTrees.

In the next patch I'll make it recurse, so it's clearer that this._detachFromParentFrame() is the "base case".

>> LayoutTests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation-expected.txt:14
>> +PASS: There should not be a DOMTree listening to DOMTreeManager events after a main frame navigation.
> 
> I'm not sure this really matters. Currently, we create a new FrameDOMTreeContentView across reloads but we continue to use the MainFrame's DOMTree. The changes you've made in this patch may be for the better though. I just want to make sure we've through through them all.
> 
> Is DOM.documentUpdated still needed? Maybe for overwriting the DOM, something like replacing the document, without an explicit navigation?

This change will force creation of a new DOMTree, so there's no way we could accidentally look up and get the old one. Even if we always create a new DOMTree, there's no reason to keep the old one around in a Frame / DOMTreeManager. If a view needs the old tree for some reason, then it keeps a reference and it won't get GC'd.

I don't want to touch documentUpdated.

>> LayoutTests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html:18
>> +        }
> 
> This, looking into the internals of WebInspector.Object kinda irks me and was why I didn't write a test in the first place.
> 
> That said, I have a patch that moves _listeners to a Symbol, and that feels a bit better than this if we use a well defined symbol.

What about just moving this method to Object?

[Blaze Burg]

Created attachment 259511 [details]
Proposed Fix (revised)

[Joseph Pecoraro]

Comment on attachment 259511 [details]
Proposed Fix (revised)

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

Awesome, this looks great! r=me

> Source/WebInspectorUI/UserInterface/Base/Object.js:114
> +    static retainedObjectsWithPrototype(proto) {

Style: Braces

> LayoutTests/http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html:32
> +             // Force creation of child DOM trees.

Style: spaces

[Blaze Burg]

Committed r188756: <http://trac.webkit.org/changeset/188756>

[Alexey Proskuryakov]

This test (http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.html) always times out on Windows. Brian, could you please take care of that?

[Joseph Pecoraro]

(In reply to comment #8)
> This test
> (http/tests/inspector/dom/disconnect-dom-tree-after-main-frame-navigation.
> html) always times out on Windows. Brian, could you please take care of that?

I thought windows skipped all inspector tests. Maybe it wasn't skipping http/tests/inspector?