Bug 148158 – Web Inspector: DOMTree leaks on main resource changes

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

[Joseph Pecoraro]

* SUMMARY
DOMTree leaks on main resource changes.

* NOTES
- DOMTree registers itself as a listener on DOMTreeManager during construction:
    WebInspector.domTreeManager.addEventListener(WebInspector.DOMTreeManager.Event.DocumentUpdated, this._documentUpdated, this);
    WebInspector.domTreeManager.addEventListener(WebInspector.DOMTreeManager.Event.ContentFlowListWasUpdated, this._contentFlowListWasUpdated, this);
    WebInspector.domTreeManager.addEventListener(WebInspector.DOMTreeManager.Event.ContentFlowWasAdded, this._contentFlowWasAdded, this);
    WebInspector.domTreeManager.addEventListener(WebInspector.DOMTreeManager.Event.ContentFlowWasRemoved, this._contentFlowWasRemoved, this);
    - However, these are never removed. So that means the DOMTree, and everything it hangs onto (DOMNodes) will remain alive.

- Perhaps "invalidate" should be removing these event listeners. Needs a bit more investigation.

[Radar WebKit Bug Importer]

<rdar://problem/22337238>

[Joseph Pecoraro]

After studying the ownership of different DOM* objects, this is what I think:

  1. Frame's own DOMTrees which register DOMTreeManager listeners
    => when a Main Frame is replaced, each frame in the frame three should "disconnect" their DOMTree
    => when a Sub Frame is removed, it should "disconnect" its DOMTree

  2. DOMContentViews registers DOMTreeManager listeners
    => already "disconnects" its listeners when the ContentView is closed

  3. DOMContentViews own DOMTreeOutlines which register DOMTreeManager listeners
    => already informs its DOMTreeUpdater to "disconnect" when the ContentView is closed

So the issue here is only a `Frame <-> DOMTree <-> DOMTreeManager listener` leak. Though that could leak a bunch of Resources because the DOMTree references the Frame.

[Joseph Pecoraro]

Created attachment 259436 [details]
[PATCH] Proposed Fix

After testing this keeps the number of WebInspector.domTreeManager event listeners down to 2 after navigating / reloading, and ending up on a simple page.

[Devin Rousso]

Comment on attachment 259436 [details]
[PATCH] Proposed Fix

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

> Source/WebInspectorUI/UserInterface/Models/DOMTree.js:90
> +            this._invalidateTimeoutIdentifier = undefined;

Lack of knowledge question:  is there a reason to use "undefined" here instead of "null"?  As I understood it, timeout id's are just integers...

> Source/WebInspectorUI/UserInterface/Models/Frame.js:449
> +    _detachFromParentFrame()

NIT: It looks weird to me to see non-"this" objects calling a private method (seen on lines 334 and 345 above).  I would make this public so it doesn't look so weird...

[Timothy Hatcher]

Comment on attachment 259436 [details]
[PATCH] Proposed Fix

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

>> Source/WebInspectorUI/UserInterface/Models/DOMTree.js:90
>> +            this._invalidateTimeoutIdentifier = undefined;
> 
> Lack of knowledge question:  is there a reason to use "undefined" here instead of "null"?  As I understood it, timeout id's are just integers...

It will start as undefined until the timeout is used the first time. Though 0 would be fine too, we just tend to use undefined here.

>> Source/WebInspectorUI/UserInterface/Models/Frame.js:449
>> +    _detachFromParentFrame()
> 
> NIT: It looks weird to me to see non-"this" objects calling a private method (seen on lines 334 and 345 above).  I would make this public so it doesn't look so weird...

It is still internal to this class, so it is fine.

[WebKit Commit Bot]

Comment on attachment 259436 [details]
[PATCH] Proposed Fix

Clearing flags on attachment: 259436

Committed r188679: <http://trac.webkit.org/changeset/188679>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Joseph Pecoraro]

Comment on attachment 259436 [details]
[PATCH] Proposed Fix

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

>>> Source/WebInspectorUI/UserInterface/Models/DOMTree.js:90
>>> +            this._invalidateTimeoutIdentifier = undefined;
>> 
>> Lack of knowledge question:  is there a reason to use "undefined" here instead of "null"?  As I understood it, timeout id's are just integers...
> 
> It will start as undefined until the timeout is used the first time. Though 0 would be fine too, we just tend to use undefined here.

I have been using "undefined" or "false" for primitives that were deleted. Technically 0 or NaN would be okay here, but I don't want to confuse that with an actual number.
I have been using "null" for objects / arrays that were deleted. That should make it clear that an object occupies this property.