148158 2015-08-18 18:48:07 -0700 Web Inspector: DOMTree leaks on main resource changes 2015-08-20 09:26:52 -0700 1 1 1 Unclassified WebKit Web Inspector WebKit Nightly Build All All RESOLVED FIXED InRadar P2 Normal --- 148223 1 joepeck joepeck bburg commit-queue graouts joepeck mattbaker nvasilyev timothy webkit-bug-importer oldest_to_newest 1118775 0 joepeck 2015-08-18 18:48:07 -0700 * 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. 1118776 1 webkit-bug-importer 2015-08-18 18:48:35 -0700 <rdar://problem/22337238> 1118974 2 joepeck 2015-08-19 12:51:56 -0700 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. 1119130 3 259436 joepeck 2015-08-19 17:52:29 -0700 Created attachment 259436 [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. 1119155 4 259436 hi 2015-08-19 18:51:30 -0700 Comment on attachment 259436 [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... 1119167 5 259436 timothy 2015-08-19 20:25:46 -0700 Comment on attachment 259436 [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. 1119170 6 259436 commit-queue 2015-08-19 21:09:23 -0700 Comment on attachment 259436 [PATCH] Proposed Fix Clearing flags on attachment: 259436 Committed r188679: <http://trac.webkit.org/changeset/188679> 1119171 7 commit-queue 2015-08-19 21:09:27 -0700 All reviewed patches have been landed. Closing bug. 1119176 8 259436 joepeck 2015-08-19 21:35:50 -0700 Comment on attachment 259436 [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. 259436 2015-08-19 17:52:29 -0700 2015-08-19 21:09:23 -0700 [PATCH] Proposed Fix leak-1.patch text/plain 5314 joepeck ZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvQ2hhbmdlTG9nCmluZGV4IDk1NGRhMzIuLjVkNTI2MGIgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL1dlYkluc3BlY3Rv clVJL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDI4IEBACiAyMDE1LTA4LTE5ICBKb3NlcGggUGVjb3Jh cm8gIDxwZWNvcmFyb0BhcHBsZS5jb20+CiAKKyAgICAgICAgV2ViIEluc3BlY3RvcjogRE9NVHJl ZSBsZWFrcyBvbiBtYWluIHJlc291cmNlIGNoYW5nZXMKKyAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTE0ODE1OAorCisgICAgICAgIFJldmlld2VkIGJ5IE5P Qk9EWSAoT09QUyEpLgorCisgICAgICAgICogVXNlckludGVyZmFjZS9Nb2RlbHMvRE9NVHJlZS5q czoKKyAgICAgICAgKFdlYkluc3BlY3Rvci5ET01UcmVlLnByb3RvdHlwZS5kaXNjb25uZWN0KToK KyAgICAgICAgQWRkIGEgd2F5IHRvIGRpc2Nvbm5lY3QgdGhlIERPTVRyZWUgYW5kIGFsbG93IGl0 IHRvIGdldCBnYXJiYWdlIGNvbGxlY3RlZC4KKworICAgICAgICAoV2ViSW5zcGVjdG9yLkRPTVRy ZWUucHJvdG90eXBlLmludmFsaWRhdGUucGVyZm9ybUludmFsaWRhdGUpOgorICAgICAgICAoV2Vi SW5zcGVjdG9yLkRPTVRyZWUucHJvdG90eXBlLmludmFsaWRhdGUpOgorICAgICAgICAoV2ViSW5z cGVjdG9yLkRPTVRyZWUucHJvdG90eXBlLl9yZXF1ZXN0Um9vdERPTU5vZGUpOgorICAgICAgICAo V2ViSW5zcGVjdG9yLkRPTVRyZWUucHJvdG90eXBlLl9mcmFtZVBhZ2VFeGVjdXRpb25Db250ZXh0 Q2hhbmdlZCk6CisgICAgICAgIFJlbW92ZSBzb21lIHVzZXMgb2YgdGhlIGRlbGV0ZSBvcGVyYXRv ci4KKworICAgICAgICAqIFVzZXJJbnRlcmZhY2UvTW9kZWxzL0ZyYW1lLmpzOgorICAgICAgICAo V2ViSW5zcGVjdG9yLkZyYW1lLnByb3RvdHlwZS5yZW1vdmVDaGlsZEZyYW1lKToKKyAgICAgICAg KFdlYkluc3BlY3Rvci5GcmFtZS5wcm90b3R5cGUucmVtb3ZlQWxsQ2hpbGRGcmFtZXMpOgorICAg ICAgICAoV2ViSW5zcGVjdG9yLkZyYW1lLnByb3RvdHlwZS5fZGV0YWNoRnJvbVBhcmVudEZyYW1l KToKKyAgICAgICAgV2hlbiBhIGZyYW1lIGdldHMgcmVtb3ZlZCBmcm9tIGl0cyBwYXJlbnQsIGRp c2Nvbm5lY3QgaXRzIERPTVRyZWUuCisKKzIwMTUtMDgtMTkgIEpvc2VwaCBQZWNvcmFybyAgPHBl Y29yYXJvQGFwcGxlLmNvbT4KKwogICAgICAgICBXZWIgSW5zcGVjdG9yOiBVcGRhdGUgQ29weXJp Z2h0IGluIG1pbmlmaWVkIC8gY29tYmluZWQgb3V0cHV0CiAgICAgICAgIGh0dHBzOi8vYnVncy53 ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xNDgyMDEKIApkaWZmIC0tZ2l0IGEvU291cmNlL1dl Ykluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvTW9kZWxzL0RPTVRyZWUuanMgYi9Tb3VyY2UvV2Vi SW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9Nb2RlbHMvRE9NVHJlZS5qcwppbmRleCAwNjc0NjMy Li40N2U3ZTNkIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFj ZS9Nb2RlbHMvRE9NVHJlZS5qcworKysgYi9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVy ZmFjZS9Nb2RlbHMvRE9NVHJlZS5qcwpAQCAtNjcsNiArNjcsMTIgQEAgV2ViSW5zcGVjdG9yLkRP TVRyZWUgPSBjbGFzcyBET01UcmVlIGV4dGVuZHMgV2ViSW5zcGVjdG9yLk9iamVjdAogICAgICAg ICByZXR1cm4gT2JqZWN0LmtleXModGhpcy5fZmxvd01hcCkubGVuZ3RoOwogICAgIH0KIAorICAg IGRpc2Nvbm5lY3QoKQorICAgIHsKKyAgICAgICAgV2ViSW5zcGVjdG9yLmRvbVRyZWVNYW5hZ2Vy LnJlbW92ZUV2ZW50TGlzdGVuZXIobnVsbCwgbnVsbCwgdGhpcyk7CisgICAgICAgIHRoaXMuX2Zy YW1lLnJlbW92ZUV2ZW50TGlzdGVuZXIobnVsbCwgbnVsbCwgdGhpcyk7CisgICAgfQorCiAgICAg aW52YWxpZGF0ZSgpCiAgICAgewogICAgICAgICAvLyBTZXQgdG8gbnVsbCBzbyBpdCBpcyBmZXRj aGVkIGFnYWluIG5leHQgdGltZSByZXF1ZXN0Um9vdERPTU5vZGUgaXMgY2FsbGVkLgpAQCAtNzQs MTQgKzgwLDE0IEBAIFdlYkluc3BlY3Rvci5ET01UcmVlID0gY2xhc3MgRE9NVHJlZSBleHRlbmRz IFdlYkluc3BlY3Rvci5PYmplY3QKIAogICAgICAgICAvLyBDbGVhciB0aGUgcGVuZGluZyBjYWxs YmFja3MuIEl0IGlzIHRoZSByZXNwb25zaWJpbGl0eSBvZiB0aGUgY2xpZW50IHRvIGxpc3RlbiBm b3IKICAgICAgICAgLy8gdGhlIFJvb3RET01Ob2RlSW52YWxpZGF0ZWQgZXZlbnQgYW5kIHJlcXVl c3QgdGhlIHJvb3QgRE9NIG5vZGUgYWdhaW4uCi0gICAgICAgIGRlbGV0ZSB0aGlzLl9wZW5kaW5n Um9vdERPTU5vZGVSZXF1ZXN0czsKKyAgICAgICAgdGhpcy5fcGVuZGluZ1Jvb3RET01Ob2RlUmVx dWVzdHMgPSBudWxsOwogCiAgICAgICAgIGlmICh0aGlzLl9pbnZhbGlkYXRlVGltZW91dElkZW50 aWZpZXIpCiAgICAgICAgICAgICByZXR1cm47CiAKICAgICAgICAgZnVuY3Rpb24gcGVyZm9ybUlu dmFsaWRhdGUoKQogICAgICAgICB7Ci0gICAgICAgICAgICBkZWxldGUgdGhpcy5faW52YWxpZGF0 ZVRpbWVvdXRJZGVudGlmaWVyOworICAgICAgICAgICAgdGhpcy5faW52YWxpZGF0ZVRpbWVvdXRJ ZGVudGlmaWVyID0gdW5kZWZpbmVkOwogCiAgICAgICAgICAgICB0aGlzLmRpc3BhdGNoRXZlbnRU b0xpc3RlbmVycyhXZWJJbnNwZWN0b3IuRE9NVHJlZS5FdmVudC5Sb290RE9NTm9kZUludmFsaWRh dGVkKTsKICAgICAgICAgfQpAQCAtMTk3LDcgKzIwMyw3IEBAIFdlYkluc3BlY3Rvci5ET01UcmVl ID0gY2xhc3MgRE9NVHJlZSBleHRlbmRzIFdlYkluc3BlY3Rvci5PYmplY3QKIAogICAgICAgICAg ICAgZm9yICh2YXIgaSA9IDA7IGkgPCB0aGlzLl9wZW5kaW5nUm9vdERPTU5vZGVSZXF1ZXN0cy5s ZW5ndGg7ICsraSkKICAgICAgICAgICAgICAgICB0aGlzLl9wZW5kaW5nUm9vdERPTU5vZGVSZXF1 ZXN0c1tpXSh0aGlzLl9yb290RE9NTm9kZSk7Ci0gICAgICAgICAgICBkZWxldGUgdGhpcy5fcGVu ZGluZ1Jvb3RET01Ob2RlUmVxdWVzdHM7CisgICAgICAgICAgICB0aGlzLl9wZW5kaW5nUm9vdERP TU5vZGVSZXF1ZXN0cyA9IG51bGw7CiAgICAgICAgIH0KIAogICAgICAgICAvLyBGb3IgdGhlIG1h aW4gZnJhbWUgd2UgY2FuIHVzZSB0aGUgbW9yZSBzdHJhaWdodCBmb3J3YXJkIHJlcXVlc3REb2N1 bWVudCBmdW5jdGlvbi4gRm9yCkBAIC0yNDIsNyArMjQ4LDcgQEAgV2ViSW5zcGVjdG9yLkRPTVRy ZWUgPSBjbGFzcyBET01UcmVlIGV4dGVuZHMgV2ViSW5zcGVjdG9yLk9iamVjdAogICAgICAgICAg ICAgY29uc29sZS5hc3NlcnQodGhpcy5fZnJhbWUucGFnZUV4ZWN1dGlvbkNvbnRleHQpOwogICAg ICAgICAgICAgY29uc29sZS5hc3NlcnQodGhpcy5fcGVuZGluZ1Jvb3RET01Ob2RlUmVxdWVzdHMg JiYgdGhpcy5fcGVuZGluZ1Jvb3RET01Ob2RlUmVxdWVzdHMubGVuZ3RoKTsKIAotICAgICAgICAg ICAgZGVsZXRlIHRoaXMuX3Jvb3RET01Ob2RlUmVxdWVzdFdhaXRpbmdGb3JFeGVjdXRpb25Db250 ZXh0OworICAgICAgICAgICAgdGhpcy5fcm9vdERPTU5vZGVSZXF1ZXN0V2FpdGluZ0ZvckV4ZWN1 dGlvbkNvbnRleHQgPSBmYWxzZTsKIAogICAgICAgICAgICAgdGhpcy5fcmVxdWVzdFJvb3RET01O b2RlKCk7CiAgICAgICAgIH0KZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2Vy SW50ZXJmYWNlL01vZGVscy9GcmFtZS5qcyBiL1NvdXJjZS9XZWJJbnNwZWN0b3JVSS9Vc2VySW50 ZXJmYWNlL01vZGVscy9GcmFtZS5qcwppbmRleCAwNjE5YzQ3Li5iMTRiMzZkIDEwMDY0NAotLS0g YS9Tb3VyY2UvV2ViSW5zcGVjdG9yVUkvVXNlckludGVyZmFjZS9Nb2RlbHMvRnJhbWUuanMKKysr IGIvU291cmNlL1dlYkluc3BlY3RvclVJL1VzZXJJbnRlcmZhY2UvTW9kZWxzL0ZyYW1lLmpzCkBA IC0zMzEsNyArMzMxLDcgQEAgV2ViSW5zcGVjdG9yLkZyYW1lID0gY2xhc3MgRnJhbWUgZXh0ZW5k cyBXZWJJbnNwZWN0b3IuT2JqZWN0CiAgICAgICAgIHRoaXMuX2NoaWxkRnJhbWVzLnJlbW92ZShj aGlsZEZyYW1lKTsKICAgICAgICAgZGVsZXRlIHRoaXMuX2NoaWxkRnJhbWVJZGVudGlmaWVyTWFw W2NoaWxkRnJhbWUuX2lkXTsKIAotICAgICAgICBjaGlsZEZyYW1lLl9wYXJlbnRGcmFtZSA9IG51 bGw7CisgICAgICAgIGNoaWxkRnJhbWUuX2RldGFjaEZyb21QYXJlbnRGcmFtZSgpOwogCiAgICAg ICAgIHRoaXMuZGlzcGF0Y2hFdmVudFRvTGlzdGVuZXJzKFdlYkluc3BlY3Rvci5GcmFtZS5FdmVu dC5DaGlsZEZyYW1lV2FzUmVtb3ZlZCwge2NoaWxkRnJhbWV9KTsKICAgICB9CkBAIC0zNDEsOCAr MzQxLDggQEAgV2ViSW5zcGVjdG9yLkZyYW1lID0gY2xhc3MgRnJhbWUgZXh0ZW5kcyBXZWJJbnNw ZWN0b3IuT2JqZWN0CiAgICAgICAgIGlmICghdGhpcy5fY2hpbGRGcmFtZXMubGVuZ3RoKQogICAg ICAgICAgICAgcmV0dXJuOwogCi0gICAgICAgIGZvciAodmFyIGkgPSAwOyBpIDwgdGhpcy5fY2hp bGRGcmFtZXMubGVuZ3RoOyArK2kpCi0gICAgICAgICAgICB0aGlzLl9jaGlsZEZyYW1lc1tpXS5f cGFyZW50RnJhbWUgPSBudWxsOworICAgICAgICBmb3IgKGxldCBjaGlsZEZyYW1lIG9mIHRoaXMu X2NoaWxkRnJhbWVzKQorICAgICAgICAgICAgY2hpbGRGcmFtZS5fZGV0YWNoRnJvbVBhcmVudEZy YW1lKCk7CiAKICAgICAgICAgdGhpcy5fY2hpbGRGcmFtZXMgPSBbXTsKICAgICAgICAgdGhpcy5f Y2hpbGRGcmFtZUlkZW50aWZpZXJNYXAgPSB7fTsKQEAgLTQ0Niw2ICs0NDYsMTYgQEAgV2ViSW5z cGVjdG9yLkZyYW1lID0gY2xhc3MgRnJhbWUgZXh0ZW5kcyBXZWJJbnNwZWN0b3IuT2JqZWN0CiAK ICAgICAvLyBQcml2YXRlCiAKKyAgICBfZGV0YWNoRnJvbVBhcmVudEZyYW1lKCkKKyAgICB7Cisg ICAgICAgIGlmICh0aGlzLl9kb21UcmVlKSB7CisgICAgICAgICAgICB0aGlzLl9kb21UcmVlLmRp c2Nvbm5lY3QoKTsKKyAgICAgICAgICAgIHRoaXMuX2RvbVRyZWUgPSBudWxsOworICAgICAgICB9 CisKKyAgICAgICAgdGhpcy5fcGFyZW50RnJhbWUgPSBudWxsOworICAgIH0KKwogICAgIF9pc1By b3Zpc2lvbmFsUmVzb3VyY2UocmVzb3VyY2UpCiAgICAgewogICAgICAgICByZXR1cm4gKHJlc291 cmNlLmxvYWRlcklkZW50aWZpZXIgJiYgdGhpcy5fcHJvdmlzaW9uYWxMb2FkZXJJZGVudGlmaWVy ICYmIHJlc291cmNlLmxvYWRlcklkZW50aWZpZXIgPT09IHRoaXMuX3Byb3Zpc2lvbmFsTG9hZGVy SWRlbnRpZmllcik7Cg==