* 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.
<rdar://problem/22337238>
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.
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.
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...
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.
Comment on attachment 259436 [details] [PATCH] Proposed Fix Clearing flags on attachment: 259436 Committed r188679: <http://trac.webkit.org/changeset/188679>
All reviewed patches have been landed. Closing bug.
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.