103242 2012-11-26 04:38:14 -0800 Circular reference between Document and MediaQueryMatcher. 2012-12-11 13:54:20 -0800 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 1 marja marja abarth ap beidson hayato jochen kenneth luiz marcoos+bwo ojan webkit.review.bot yongjun_zhang oldest_to_newest 775356 0 marja 2012-11-26 04:38:14 -0800 Document has a RefPtr<MediaQueryMatcher>, and in ~Document it does this: if (m_mediaQueryMatcher) m_mediaQueryMatcher->documentDestroyed(); However, MediaQueryMatcher also references Document: MediaQueryMatcher has Vector<OwnPtr<Listener> > m_listeners: - MediaQueryMatcher::Listener has RefPtr<MediaQueryListListener> m_listener; - MediaQueryListListener has ScriptValue m_value; - In V8, ScriptValue has ScopedPersistent<v8::Value> m_value; and it ends up keeping the Document alive. This reference path is cleared in MediaQueryMatcher::documentDestroyed() which destroys the MediaQueryListListeners. So, for example, if a tab is reloaded in Chrome, and there are MediaQueryListListeners, ~Document never happens. This causes www.crbug.com/113983. 775367 1 175973 marja 2012-11-26 05:00:19 -0800 Created attachment 175973 Patch 775384 2 175973 webkit.review.bot 2012-11-26 05:42:06 -0800 Comment on attachment 175973 Patch Clearing flags on attachment: 175973 Committed r135709: <http://trac.webkit.org/changeset/135709> 775385 3 webkit.review.bot 2012-11-26 05:42:10 -0800 All reviewed patches have been landed. Closing bug. 775555 4 ap 2012-11-26 09:32:42 -0800 Is it correct that a document that has been detached can never be re-attached (and we'll never have such a code path)? > if (m_mediaQueryMatcher) > m_mediaQueryMatcher->documentDestroyed(); Calling this function from a function other than destructor is not telling the truth. You should have renamed it to documentDetached. 775651 5 175973 abarth 2012-11-26 10:48:23 -0800 Comment on attachment 175973 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=175973&action=review > Source/WebCore/dom/Document.cpp:2144 > + if (m_mediaQueryMatcher) > + m_mediaQueryMatcher->documentDestroyed(); Why does the m_mediaQueryMatcher need special handling here? Can't it use one of our more general mechanisms for getting notified about the Document's lifecycle? 776521 6 marja 2012-11-27 02:22:39 -0800 Update: We're discussing with abarth@ what is the right way to fix this bug. So I won't do the name change documentDestroyed -> documentDetached before the real fix. This quick and dirty fix was merged to Chromium, and I'm willing to work on the real fix, as soon as we figure out how it should be done. 788761 7 yongjun_zhang 2012-12-11 13:54:20 -0800 (In reply to comment #6) > Update: > > We're discussing with abarth@ what is the right way to fix this bug. So I won't do the name change documentDestroyed -> documentDetached before the real fix. > > This quick and dirty fix was merged to Chromium, and I'm willing to work on the real fix, as soon as we figure out how it should be done. Given the fact that this circular reference is caused by matchMedia listeners, I guess we could add a method removeAllListeners to MediaQueryMatcher and call that in Document::removeAllEventListeners()? 175973 2012-11-26 05:00:19 -0800 2012-11-26 10:48:23 -0800 Patch bug-103242-20121126135758.patch text/plain 1891 marja U3VidmVyc2lvbiBSZXZpc2lvbjogMTM1NzA2CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggMzJmNzE4MGExMWUyNzJk N2NlOGYyZGRiZjVkZDA3ODkwZTcwNmIyMC4uMjQzYzNmN2Y4YWVmN2FlYmFhN2ZjZmNkODZkZTU1 NGNjNjMzZTllOSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIwIEBACisyMDEyLTExLTI2ICBNYXJq YSBIw7ZsdHTDpCAgPG1hcmphQGNocm9taXVtLm9yZz4KKworICAgICAgICBDaXJjdWxhciByZWZl cmVuY2UgYmV0d2VlbiBEb2N1bWVudCBhbmQgTWVkaWFRdWVyeU1hdGNoZXIuCisgICAgICAgIGh0 dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMDMyNDIKKworICAgICAgICBS ZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBJdCdzIG5vdCBlbm91Z2ggdG8g Y2xlYW4gdXAgbGlzdGVuZXJzIGluIE1lZGlhUXVlcnlNYXRjaGVyIGluIH5Eb2N1bWVudCwKKyAg ICAgICAgc2luY2UgTWVkaWFRdWVyeUxpc3RMaXN0ZW5lciBrZWVwcyB0aGUgRG9jdW1lbnQgYWxp dmUuIFRoaXMgY2F1c2VkCisgICAgICAgIHd3dy5jcmJ1Zy5jb20vMTEzOTgzLgorCisgICAgICAg IE5vIG5ldyB0ZXN0czogTm8gdmlzaWJsZSBjaGFuZ2UgaW4gYmVoYXZpb3IgKGV4Y2VwdCB0aGF0 IGl0IGRvZXNuJ3QgbGVhayBtZW1vcnkpLgorCisgICAgICAgICogZG9tL0RvY3VtZW50LmNwcDoK KyAgICAgICAgKFdlYkNvcmU6OkRvY3VtZW50Ojp+RG9jdW1lbnQpOgorICAgICAgICAoV2ViQ29y ZTo6RG9jdW1lbnQ6OmRldGFjaCk6CisKIDIwMTItMTEtMjYgIFplbm8gQWxiaXNzZXIgIDx6ZW5v QHdlYmtpdC5vcmc+CiAKICAgICAgICAgR3JhcGhpY3NTdXJmYWNlIHNob3VsZCBvbmx5IHN0b3Jl IGl0cyBzaXplIGluIGEgc2luZ2xlIHBsYWNlLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUv ZG9tL0RvY3VtZW50LmNwcCBiL1NvdXJjZS9XZWJDb3JlL2RvbS9Eb2N1bWVudC5jcHAKaW5kZXgg MzNjMmQwNWE5NTYxYzQ0MTQyNTk5OWZiYTBhMzVjMTJkZWRlYjJlNS4uMzc1OWM0NDJjOTQ0YmE2 YmI2ODQ1NjNiYjYyMWMzOTU4YzE4NjA3NyAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvZG9t L0RvY3VtZW50LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9kb20vRG9jdW1lbnQuY3BwCkBAIC02 NDgsOSArNjQ4LDYgQEAgRG9jdW1lbnQ6On5Eb2N1bWVudCgpCiAKICAgICBtX3dlYWtSZWZlcmVu Y2UtPmNsZWFyKCk7CiAKLSAgICBpZiAobV9tZWRpYVF1ZXJ5TWF0Y2hlcikKLSAgICAgICAgbV9t ZWRpYVF1ZXJ5TWF0Y2hlci0+ZG9jdW1lbnREZXN0cm95ZWQoKTsKLQogICAgIGNsZWFyU3R5bGVS ZXNvbHZlcigpOyAvLyBXZSBuZWVkIHRvIGRlc3RvcnkgQ1NTRm9udFNlbGVjdG9yIGJlZm9yZSBk ZXN0cm95aW5nIG1fY2FjaGVkUmVzb3VyY2VMb2FkZXIuCiAKICAgICAvLyBJdCdzIHBvc3NpYmxl IGZvciBtdWx0aXBsZSBEb2N1bWVudHMgdG8gZW5kIHVwIHJlZmVyZW5jaW5nIHRoZSBzYW1lIENh Y2hlZFJlc291cmNlTG9hZGVyIChlLmcuLCBTVkdJbWFnZXMKQEAgLTIxNDIsNiArMjEzOSw5IEBA IHZvaWQgRG9jdW1lbnQ6OmRldGFjaCgpCiAgICAgLy8gY2FsbGVycyBvZiBEb2N1bWVudDo6ZGV0 YWNoKCkuCiAgICAgbV9mcmFtZSA9IDA7CiAgICAgbV9yZW5kZXJBcmVuYS5jbGVhcigpOworCisg ICAgaWYgKG1fbWVkaWFRdWVyeU1hdGNoZXIpCisgICAgICAgIG1fbWVkaWFRdWVyeU1hdGNoZXIt PmRvY3VtZW50RGVzdHJveWVkKCk7CiB9CiAKIHZvaWQgRG9jdW1lbnQ6OnByZXBhcmVGb3JEZXN0 cnVjdGlvbigpCg==