| Summary: | Web Inspector: DOMTree leaks on main resource changes | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||
| Component: | Web Inspector | Assignee: | Joseph Pecoraro <joepeck> | ||||
| Status: | RESOLVED FIXED | ||||||
| Severity: | Normal | CC: | bburg, commit-queue, graouts, joepeck, mattbaker, nvasilyev, timothy, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | WebKit Nightly Build | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Bug Depends on: | |||||||
| Bug Blocks: | 148223 | ||||||
| Attachments: |
|
||||||
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. |
* 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.