104203 2012-12-05 18:18:36 -0800 [V8] Remove addImplicitReferencesForNodeWithEventListeners() 2012-12-17 01:30:00 -0800 1 1 1 Unclassified WebKit WebCore JavaScript 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 haraken haraken abarth danno japhet loislo pfeldman webkit.review.bot yurys oldest_to_newest 784423 0 haraken 2012-12-05 18:18:36 -0800 We can use opaqueRootForGC() instead. By this change, we can remove all AddImplicitReferences() from V8 bindings. 784426 1 177899 haraken 2012-12-05 18:20:32 -0800 Created attachment 177899 Patch 784457 2 177899 abarth 2012-12-05 18:40:43 -0800 Comment on attachment 177899 Patch I tried this before but ran into trouble (some tests failed). It's likely I screwed it up though. 784463 3 177899 abarth 2012-12-05 18:42:26 -0800 Comment on attachment 177899 Patch There is a subtle difference here: Previously when someone was hold an event listener function, they wouldn't hold the node it was attached to (because AddImplicitReferences are one-way connections). Now we'll hold the node as well (because addToGroup is a two-way connection). I can't see how that would matter in practice. 784501 4 haraken 2012-12-05 19:41:53 -0800 (In reply to comment #3) > (From update of attachment 177899 [details]) > There is a subtle difference here: Previously when someone was hold an event listener function, they wouldn't hold the node it was attached to (because AddImplicitReferences are one-way connections). Now we'll hold the node as well (because addToGroup is a two-way connection). I can't see how that would matter in practice. Good point. In theory, the following situation would be possible: - There is a tree T. - A node P is in T. P has an event listener L. - A JS object X has a reference to L. X is reachable from JS roots. - All nodes in T become unreachable. - T is expected to be collected. However, due to the reference from X to L, T is not collected. Maybe we shouldn't make this change? 784533 5 abarth 2012-12-05 20:44:58 -0800 > Maybe we shouldn't make this change? I don't think we should worry about it. 784534 6 177899 haraken 2012-12-05 20:45:27 -0800 Comment on attachment 177899 Patch ok 784540 7 177899 webkit.review.bot 2012-12-05 20:55:05 -0800 Comment on attachment 177899 Patch Clearing flags on attachment: 177899 Committed r136794: <http://trac.webkit.org/changeset/136794> 784541 8 webkit.review.bot 2012-12-05 20:55:09 -0800 All reviewed patches have been landed. Closing bug. 792535 9 yurys 2012-12-17 00:48:36 -0800 (In reply to comment #5) > > Maybe we shouldn't make this change? > > I don't think we should worry about it. Could you elaborate on this? Now adding a listener to a node will keep the tree alive forever unless the listener is also collected. I can easily imagine a case when the same function is reused as an event listener for different DOM trees and in this case all the trees will never be GCed. What is the benefit of this patch given that it introduces this regression? I think it should be rolled out. 792538 10 haraken 2012-12-17 00:54:14 -0800 (In reply to comment #9) > (In reply to comment #5) > > > Maybe we shouldn't make this change? > > > > I don't think we should worry about it. > > Could you elaborate on this? Now adding a listener to a node will keep the tree alive forever unless the listener is also collected. I can easily imagine a case when the same function is reused as an event listener for different DOM trees and in this case all the trees will never be GCed. > > What is the benefit of this patch given that it introduces this regression? I think it should be rolled out. Adam is on vacation. I'll roll out the patch for now. 792549 11 haraken 2012-12-17 01:27:20 -0800 Rolled out in r136794. 792550 12 yurys 2012-12-17 01:30:00 -0800 (In reply to comment #11) > Rolled out in r136794. You probably meant r137885 177899 2012-12-05 18:20:32 -0800 2012-12-05 20:55:05 -0800 Patch bug-104203-20121206111757.patch text/plain 3714 haraken U3VidmVyc2lvbiBSZXZpc2lvbjogMTM2Nzc3CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNTEzYmFmMzY5MjY0ZTVi YTUxYWJiMjIxNGVlN2YzMDBkMzAzOTRhMS4uOGI4YTc0NjIzNzE5MDc1MDQ1YTY1NGIyN2MwMDdh ZDY3MGIxNjIzYyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDIxIEBACiAyMDEyLTEyLTA1ICBLZW50 YXJvIEhhcmEgIDxoYXJha2VuQGNocm9taXVtLm9yZz4KIAorICAgICAgICBbVjhdIFJlbW92ZSBh ZGRJbXBsaWNpdFJlZmVyZW5jZXNGb3JOb2RlV2l0aEV2ZW50TGlzdGVuZXJzKCkKKyAgICAgICAg aHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTEwNDIwMworCisgICAgICAg IFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFdlIGNhbiB1c2Ugb3BhcXVl Um9vdEZvckdDKCkgaW5zdGVhZC4gQnkgdGhpcyBjaGFuZ2UsIHdlIGNhbiByZW1vdmUKKyAgICAg ICAgYWxsIEFkZEltcGxpY2l0UmVmZXJlbmNlcygpIGZyb20gVjggYmluZGluZ3MuCisKKyAgICAg ICAgVGVzdHM6IGZhc3QvZG9tL2djLWltYWdlLWVsZW1lbnQuaHRtbAorICAgICAgICAgICAgICAg ZmFzdC9kb20vZ2MtaW1hZ2UtZWxlbWVudC0yLmh0bWwKKyAgICAgICAgICAgICAgIGZhc3QvZG9t L2lubGluZS1ldmVudC1hdHRyaWJ1dGVzLWxvb2t1cC1yZW1vdmVkLmh0bWwKKworICAgICAgICAq IGJpbmRpbmdzL3Y4L1Y4R0NDb250cm9sbGVyLmNwcDoKKworMjAxMi0xMi0wNSAgS2VudGFybyBI YXJhICA8aGFyYWtlbkBjaHJvbWl1bS5vcmc+CisKICAgICAgICAgVW5yZXZpZXdlZCwgcm9sbGlu ZyBvdXQgcjEzNjQ4MS4KICAgICAgICAgaHR0cDovL3RyYWMud2Via2l0Lm9yZy9jaGFuZ2VzZXQv MTM2NDgxCiAgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0x MDM4NjgKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4R0NDb250cm9s bGVyLmNwcCBiL1NvdXJjZS9XZWJDb3JlL2JpbmRpbmdzL3Y4L1Y4R0NDb250cm9sbGVyLmNwcApp bmRleCA4NDA1MzBlOTEyNmRiMmQ1MDYxYWQ5MDMyYjdmYmZhNGE4YzExMGM1Li4zZTk1ODc1Zjcx MDIyYzE3MmFhYWI1OGExNGI1ZmY1NzE5YmI2OWU1IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29y ZS9iaW5kaW5ncy92OC9WOEdDQ29udHJvbGxlci5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvYmlu ZGluZ3MvdjgvVjhHQ0NvbnRyb2xsZXIuY3BwCkBAIC0xMTMsMjkgKzExMyw2IEBAIHByaXZhdGU6 CiAgICAgVmVjdG9yPEltcGxpY2l0Q29ubmVjdGlvbj4gbV9jb25uZWN0aW9uczsKIH07CiAKLS8v IEZJWE1FOiBUaGlzIHNob3VsZCB1c2Ugb3BhcXVlIEdDIHJvb3RzLgotc3RhdGljIHZvaWQgYWRk SW1wbGljaXRSZWZlcmVuY2VzRm9yTm9kZVdpdGhFdmVudExpc3RlbmVycyhOb2RlKiBub2RlLCB2 ODo6UGVyc2lzdGVudDx2ODo6T2JqZWN0PiB3cmFwcGVyKQotewotICAgIEFTU0VSVChub2RlLT5o YXNFdmVudExpc3RlbmVycygpKTsKLQotICAgIFZlY3Rvcjx2ODo6UGVyc2lzdGVudDx2ODo6VmFs dWU+ID4gbGlzdGVuZXJzOwotCi0gICAgRXZlbnRMaXN0ZW5lckl0ZXJhdG9yIGl0ZXJhdG9yKG5v ZGUpOwotICAgIHdoaWxlIChFdmVudExpc3RlbmVyKiBsaXN0ZW5lciA9IGl0ZXJhdG9yLm5leHRM aXN0ZW5lcigpKSB7Ci0gICAgICAgIGlmIChsaXN0ZW5lci0+dHlwZSgpICE9IEV2ZW50TGlzdGVu ZXI6OkpTRXZlbnRMaXN0ZW5lclR5cGUpCi0gICAgICAgICAgICBjb250aW51ZTsKLSAgICAgICAg VjhBYnN0cmFjdEV2ZW50TGlzdGVuZXIqIHY4bGlzdGVuZXIgPSBzdGF0aWNfY2FzdDxWOEFic3Ry YWN0RXZlbnRMaXN0ZW5lcio+KGxpc3RlbmVyKTsKLSAgICAgICAgaWYgKCF2OGxpc3RlbmVyLT5o YXNFeGlzdGluZ0xpc3RlbmVyT2JqZWN0KCkpCi0gICAgICAgICAgICBjb250aW51ZTsKLSAgICAg ICAgbGlzdGVuZXJzLmFwcGVuZCh2OGxpc3RlbmVyLT5leGlzdGluZ0xpc3RlbmVyT2JqZWN0UGVy c2lzdGVudEhhbmRsZSgpKTsKLSAgICB9Ci0KLSAgICBpZiAobGlzdGVuZXJzLmlzRW1wdHkoKSkK LSAgICAgICAgcmV0dXJuOwotCi0gICAgdjg6OlY4OjpBZGRJbXBsaWNpdFJlZmVyZW5jZXMod3Jh cHBlciwgbGlzdGVuZXJzLmRhdGEoKSwgbGlzdGVuZXJzLnNpemUoKSk7Ci19Ci0KIHZvaWQqIFY4 R0NDb250cm9sbGVyOjpvcGFxdWVSb290Rm9yR0MoTm9kZSogbm9kZSkKIHsKICAgICBpZiAobm9k ZS0+aW5Eb2N1bWVudCgpIHx8IChub2RlLT5oYXNUYWdOYW1lKEhUTUxOYW1lczo6aW1nVGFnKSAm JiBzdGF0aWNfY2FzdDxIVE1MSW1hZ2VFbGVtZW50Kj4obm9kZSktPmhhc1BlbmRpbmdBY3Rpdml0 eSgpKSkKQEAgLTIwMiwxMSArMTc5LDIxIEBAIHB1YmxpYzoKICAgICAgICAgICAgIEFTU0VSVCgh d3JhcHBlci5Jc0luZGVwZW5kZW50KCkpOwogCiAgICAgICAgICAgICBOb2RlKiBub2RlID0gc3Rh dGljX2Nhc3Q8Tm9kZSo+KG9iamVjdCk7CisgICAgICAgICAgICB2b2lkKiByb290ID0gVjhHQ0Nv bnRyb2xsZXI6Om9wYXF1ZVJvb3RGb3JHQyhub2RlKTsKKworICAgICAgICAgICAgaWYgKG5vZGUt Pmhhc0V2ZW50TGlzdGVuZXJzKCkpIHsKKyAgICAgICAgICAgICAgICBFdmVudExpc3RlbmVySXRl cmF0b3IgaXRlcmF0b3Iobm9kZSk7CisgICAgICAgICAgICAgICAgd2hpbGUgKEV2ZW50TGlzdGVu ZXIqIGxpc3RlbmVyID0gaXRlcmF0b3IubmV4dExpc3RlbmVyKCkpIHsKKyAgICAgICAgICAgICAg ICAgICAgaWYgKGxpc3RlbmVyLT50eXBlKCkgIT0gRXZlbnRMaXN0ZW5lcjo6SlNFdmVudExpc3Rl bmVyVHlwZSkKKyAgICAgICAgICAgICAgICAgICAgICAgIGNvbnRpbnVlOworICAgICAgICAgICAg ICAgICAgICBWOEFic3RyYWN0RXZlbnRMaXN0ZW5lciogdjhsaXN0ZW5lciA9IHN0YXRpY19jYXN0 PFY4QWJzdHJhY3RFdmVudExpc3RlbmVyKj4obGlzdGVuZXIpOworICAgICAgICAgICAgICAgICAg ICBpZiAoIXY4bGlzdGVuZXItPmhhc0V4aXN0aW5nTGlzdGVuZXJPYmplY3QoKSkKKyAgICAgICAg ICAgICAgICAgICAgICAgIGNvbnRpbnVlOworICAgICAgICAgICAgICAgICAgICBtX2dyb3VwZXIu YWRkVG9Hcm91cChyb290LCB2OGxpc3RlbmVyLT5leGlzdGluZ0xpc3RlbmVyT2JqZWN0UGVyc2lz dGVudEhhbmRsZSgpKTsKKyAgICAgICAgICAgICAgICB9CisgICAgICAgICAgICB9CiAKLSAgICAg ICAgICAgIGlmIChub2RlLT5oYXNFdmVudExpc3RlbmVycygpKQotICAgICAgICAgICAgICAgIGFk ZEltcGxpY2l0UmVmZXJlbmNlc0Zvck5vZGVXaXRoRXZlbnRMaXN0ZW5lcnMobm9kZSwgd3JhcHBl cik7Ci0KLSAgICAgICAgICAgIG1fZ3JvdXBlci5hZGRUb0dyb3VwKFY4R0NDb250cm9sbGVyOjpv cGFxdWVSb290Rm9yR0Mobm9kZSksIHdyYXBwZXIpOworICAgICAgICAgICAgbV9ncm91cGVyLmFk ZFRvR3JvdXAocm9vdCwgd3JhcHBlcik7CiAgICAgICAgIH0gZWxzZSBpZiAoY2xhc3NJZCA9PSB2 OERPTU9iamVjdENsYXNzSWQpIHsKICAgICAgICAgICAgIG1fZ3JvdXBlci5hZGRUb0dyb3VwKHR5 cGUtPm9wYXF1ZVJvb3RGb3JHQyhvYmplY3QsIHdyYXBwZXIpLCB3cmFwcGVyKTsKICAgICAgICAg fSBlbHNlIHsK