Bug 148223 - Web Inspector: add a regression test for the fix introduced in r188679
Summary: Web Inspector: add a regression test for the fix introduced in r188679
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: BJ Burg
URL:
Keywords: InRadar
Depends on: 148158
Blocks:
  Show dependency treegraph
 
Reported: 2015-08-20 09:26 PDT by BJ Burg
Modified: 2015-08-31 18:22 PDT (History)
7 users (show)

See Also:


Attachments
Proposed Fix (8.46 KB, patch)
2015-08-20 10:45 PDT, BJ Burg
no flags Details | Formatted Diff | Diff
Proposed Fix (revised) (12.51 KB, patch)
2015-08-20 15:44 PDT, BJ Burg
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description BJ Burg 2015-08-20 09:26:31 PDT
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.
Comment 1 Radar WebKit Bug Importer 2015-08-20 09:26:58 PDT
<rdar://problem/22362146>
Comment 2 BJ Burg 2015-08-20 10:45:11 PDT
Created attachment 259480 [details]
Proposed Fix

I manually verified that the last assertion in the test fails prior to the mentioned revision.
Comment 3 Joseph Pecoraro 2015-08-20 14:04:32 PDT
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.
Comment 4 BJ Burg 2015-08-20 15:37:46 PDT
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?
Comment 5 BJ Burg 2015-08-20 15:44:51 PDT
Created attachment 259511 [details]
Proposed Fix (revised)
Comment 6 Joseph Pecoraro 2015-08-20 17:12:40 PDT
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
Comment 7 BJ Burg 2015-08-21 10:06:13 PDT
Committed r188756: <http://trac.webkit.org/changeset/188756>
Comment 8 Alexey Proskuryakov 2015-08-31 16:33:36 PDT
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?
Comment 9 Joseph Pecoraro 2015-08-31 18:20:57 PDT
(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?