94631 2012-08-21 13:16:34 -0700 [chromium] Should be able to destroy a CCLayerTreeHost without manually setting the root layer 2012-08-21 15:35:20 -0700 1 1 1 Unclassified WebKit New Bugs 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 94174 1 jamesr jamesr cc-bugs webkit.review.bot oldest_to_newest 701046 0 jamesr 2012-08-21 13:16:34 -0700 [chromium] Should be able to destroy a CCLayerTreeHost without manually setting the root layer 701054 1 159752 jamesr 2012-08-21 13:23:30 -0700 Created attachment 159752 Patch 701080 2 159752 enne 2012-08-21 13:38:05 -0700 Comment on attachment 159752 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=159752&action=review > Source/WebCore/ChangeLog:12 > + In the depths of time when dinosaurs roamed the earth, LayerChromium and CCLayerTreeHost were both reference > + counted and there was a cycle between the root LayerChromium and CCLayerTreeHost. This required all users of > + CCLayerTreeHost to manually break the cycle by calling setRootLayer(0) before dropping their reference to the > + host. Nowadays, CCLayerTreeHost has single ownership and LayerChromiums only have a weak pointer to their host > + so we should just do this cleanup ourselves instead of imposing it on callers. This is a fanciful historical retcon. CCLayerTreeHost doesn't have single ownership, although it'd be nice if it did. > Source/WebCore/platform/graphics/chromium/cc/CCLayerTreeHost.cpp:117 > + if (m_rootLayer) > + m_rootLayer->setLayerTreeHost(0); I think you need to do this recursively or make LayerChromium not ref-counted so that you can be guaranteed of its destruction here (possibly clearing its pointer first so that the CCLayerTreeHost weak pointer is still valid during ~LayerChromium). This code probably works at this moment given that the only other owner of LayerChromiums is the render surface layer lists, which are only non-empty during CCLTH::updateLayers, but it seems a little dodgy. 701090 3 jamesr 2012-08-21 13:46:47 -0700 What I meant is CCLayerTreeHost is OwnPtr<>, not RefCounted. Maybe I should rewrite that? LayerChromium::setLayerTreeHost() is recursive - it'll set the m_layerTreeHost to 0 for all layers under the root (including mask/replicas) 701095 4 159752 enne 2012-08-21 13:50:03 -0700 Comment on attachment 159752 Patch (In reply to comment #3) > What I meant is CCLayerTreeHost is OwnPtr<>, not RefCounted. Maybe I should rewrite that? Yeah, I guess "has single ownership" (ambiguous "...of something else") vs. "has a single owner". I don't feel too strongly about it. > LayerChromium::setLayerTreeHost() is recursive - it'll set the m_layerTreeHost to 0 for all layers under the root (including mask/replicas) Oh, quite right. I was misremembering. This seems totally fine. R=me. 701097 5 jamesr 2012-08-21 13:52:10 -0700 "CCLayerTreeHost has a single owner" is much better, IMO. 701198 6 jamesr 2012-08-21 15:35:20 -0700 Committed r126198: <http://trac.webkit.org/changeset/126198> 159752 2012-08-21 13:23:30 -0700 2012-08-21 13:50:03 -0700 Patch bug-94631-20120821132329.patch text/plain 4023 jamesr U3VidmVyc2lvbiBSZXZpc2lvbjogMTI2MTc0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggYjZiNWYyYjJmNTU2ZjU0 OTgyMWE4NGIyMDg4MGIyZGJlNmIzOTYwZi4uZDU4Y2FhNzk0Y2RhNTM2YjBmMGM5YTM0OTI1NDgy ZThjN2Q2ZDA4MSAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSw1ICsxLDIzIEBACiAyMDEyLTA4LTIxICBKYW1l cyBSb2JpbnNvbiAgPGphbWVzckBjaHJvbWl1bS5vcmc+CiAKKyAgICAgICAgW2Nocm9taXVtXSBT aG91bGQgYmUgYWJsZSB0byBkZXN0cm95IGEgQ0NMYXllclRyZWVIb3N0IHdpdGhvdXQgbWFudWFs bHkgc2V0dGluZyB0aGUgcm9vdCBsYXllcgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9y Zy9zaG93X2J1Zy5jZ2k/aWQ9OTQ2MzEKKworICAgICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9P UFMhKS4KKworICAgICAgICBJbiB0aGUgZGVwdGhzIG9mIHRpbWUgd2hlbiBkaW5vc2F1cnMgcm9h bWVkIHRoZSBlYXJ0aCwgTGF5ZXJDaHJvbWl1bSBhbmQgQ0NMYXllclRyZWVIb3N0IHdlcmUgYm90 aCByZWZlcmVuY2UKKyAgICAgICAgY291bnRlZCBhbmQgdGhlcmUgd2FzIGEgY3ljbGUgYmV0d2Vl biB0aGUgcm9vdCBMYXllckNocm9taXVtIGFuZCBDQ0xheWVyVHJlZUhvc3QuIFRoaXMgcmVxdWly ZWQgYWxsIHVzZXJzIG9mCisgICAgICAgIENDTGF5ZXJUcmVlSG9zdCB0byBtYW51YWxseSBicmVh ayB0aGUgY3ljbGUgYnkgY2FsbGluZyBzZXRSb290TGF5ZXIoMCkgYmVmb3JlIGRyb3BwaW5nIHRo ZWlyIHJlZmVyZW5jZSB0byB0aGUKKyAgICAgICAgaG9zdC4gTm93YWRheXMsIENDTGF5ZXJUcmVl SG9zdCBoYXMgc2luZ2xlIG93bmVyc2hpcCBhbmQgTGF5ZXJDaHJvbWl1bXMgb25seSBoYXZlIGEg d2VhayBwb2ludGVyIHRvIHRoZWlyIGhvc3QKKyAgICAgICAgc28gd2Ugc2hvdWxkIGp1c3QgZG8g dGhpcyBjbGVhbnVwIG91cnNlbHZlcyBpbnN0ZWFkIG9mIGltcG9zaW5nIGl0IG9uIGNhbGxlcnMu CisKKyAgICAgICAgVW5pdCB0ZXN0IGFkZGVkIHRvIExheWVyQ2hyb21pdW1UZXN0LmNwcAorCisg ICAgICAgICogcGxhdGZvcm0vZ3JhcGhpY3MvY2hyb21pdW0vY2MvQ0NMYXllclRyZWVIb3N0LmNw cDoKKyAgICAgICAgKFdlYkNvcmU6OkNDTGF5ZXJUcmVlSG9zdDo6fkNDTGF5ZXJUcmVlSG9zdCk6 CisKKzIwMTItMDgtMjEgIEphbWVzIFJvYmluc29uICA8amFtZXNyQGNocm9taXVtLm9yZz4KKwog ICAgICAgICBVbnJldmlld2VkLCByb2xsaW5nIG91dCByMTI2MTcwLgogICAgICAgICBodHRwOi8v dHJhYy53ZWJraXQub3JnL2NoYW5nZXNldC8xMjYxNzAKICAgICAgICAgaHR0cHM6Ly9idWdzLndl YmtpdC5vcmcvc2hvd19idWcuY2dpP2lkPTk0NjE0CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0 L2Nocm9taXVtL0NoYW5nZUxvZyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9nCmlu ZGV4IDJlOTk1OTkwYWU3ZGUxYjNmNDE3ZTZkNmJkNzEzYmU3OGYwYzRlYTEuLjZiZGNiNzRlOTMx NTZhMjkxZDk5ZWE3MTVjMWZjMTExMmM4ODlmM2EgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQv Y2hyb21pdW0vQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vQ2hhbmdlTG9n CkBAIC0xLDUgKzEsMTYgQEAKIDIwMTItMDgtMjEgIEphbWVzIFJvYmluc29uICA8amFtZXNyQGNo cm9taXVtLm9yZz4KIAorICAgICAgICBbY2hyb21pdW1dIFNob3VsZCBiZSBhYmxlIHRvIGRlc3Ry b3kgYSBDQ0xheWVyVHJlZUhvc3Qgd2l0aG91dCBtYW51YWxseSBzZXR0aW5nIHRoZSByb290IGxh eWVyCisgICAgICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD05NDYz MQorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIFRlc3Rz IHRoYXQgZGVzdHJveWluZyBhIENDTGF5ZXJUcmVlSG9zdCB0aGF0IHN0aWxsIHBvaW50cyB0byBh IG5vbi1udWxsIHJvb3QgZG9lc24ndCBjcmFzaC4KKworICAgICAgICAqIHRlc3RzL0xheWVyQ2hy b21pdW1UZXN0LmNwcDoKKworMjAxMi0wOC0yMSAgSmFtZXMgUm9iaW5zb24gIDxqYW1lc3JAY2hy b21pdW0ub3JnPgorCiAgICAgICAgIFVucmV2aWV3ZWQsIHJvbGxpbmcgb3V0IHIxMjYxNzAuCiAg ICAgICAgIGh0dHA6Ly90cmFjLndlYmtpdC5vcmcvY2hhbmdlc2V0LzEyNjE3MAogICAgICAgICBo dHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9OTQ2MTQKZGlmZiAtLWdpdCBh L1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL2NjL0NDTGF5ZXJUcmVl SG9zdC5jcHAgYi9Tb3VyY2UvV2ViQ29yZS9wbGF0Zm9ybS9ncmFwaGljcy9jaHJvbWl1bS9jYy9D Q0xheWVyVHJlZUhvc3QuY3BwCmluZGV4IGViZDczMzkwMmU0YTlkYzgxN2JhZWZlZmJkMmZmZmEy NzdiZDAyMmUuLjJlM2E3ZGViM2QxNWU1MzJlNjM3ODU4MGU2ZGQyMWM5YTAxNDZhNDkgMTAwNjQ0 Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3BsYXRmb3JtL2dyYXBoaWNzL2Nocm9taXVtL2NjL0NDTGF5 ZXJUcmVlSG9zdC5jcHAKKysrIGIvU291cmNlL1dlYkNvcmUvcGxhdGZvcm0vZ3JhcGhpY3MvY2hy b21pdW0vY2MvQ0NMYXllclRyZWVIb3N0LmNwcApAQCAtMTEzLDYgKzExMyw4IEBAIGJvb2wgQ0NM YXllclRyZWVIb3N0Ojppbml0aWFsaXplKCkKIAogQ0NMYXllclRyZWVIb3N0Ojp+Q0NMYXllclRy ZWVIb3N0KCkKIHsKKyAgICBpZiAobV9yb290TGF5ZXIpCisgICAgICAgIG1fcm9vdExheWVyLT5z ZXRMYXllclRyZWVIb3N0KDApOwogICAgIEFTU0VSVChDQ1Byb3h5Ojppc01haW5UaHJlYWQoKSk7 CiAgICAgVFJBQ0VfRVZFTlQwKCJjYyIsICJDQ0xheWVyVHJlZUhvc3Q6On5DQ0xheWVyVHJlZUhv c3QiKTsKICAgICBBU1NFUlQobV9wcm94eSk7CmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViS2l0L2No cm9taXVtL3Rlc3RzL0xheWVyQ2hyb21pdW1UZXN0LmNwcCBiL1NvdXJjZS9XZWJLaXQvY2hyb21p dW0vdGVzdHMvTGF5ZXJDaHJvbWl1bVRlc3QuY3BwCmluZGV4IGI5MDI1YjRjODkwMGQzZTZkOTFi ZWQ1NmE3MTZhMDRkMDdlNjJkMDguLjBlNmViMGViNDFiMDJiZGFkNTFjNjdkYjZkODUwOTM3MzM5 NmRkOTEgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vdGVzdHMvTGF5ZXJDaHJv bWl1bVRlc3QuY3BwCisrKyBiL1NvdXJjZS9XZWJLaXQvY2hyb21pdW0vdGVzdHMvTGF5ZXJDaHJv bWl1bVRlc3QuY3BwCkBAIC04MDMsNiArODAzLDE4IEBAIFRFU1QoTGF5ZXJDaHJvbWl1bUxheWVy VHJlZUhvc3RUZXN0LCByZXBsYWNlTWFza0FuZFJlcGxpY2FMYXllcikKICAgICBXZWJLaXQ6Oldl YkNvbXBvc2l0b3I6OnNodXRkb3duKCk7CiB9CiAKK1RFU1QoTGF5ZXJDaHJvbWl1bUxheWVyVHJl ZUhvc3RUZXN0LCBkZXN0cm95SG9zdFdpdGhOb25OdWxsUm9vdExheWVyKQoreworICAgIFdlYktp dDo6V2ViQ29tcG9zaXRvcjo6aW5pdGlhbGl6ZSgwKTsKKyAgICBSZWZQdHI8TGF5ZXJDaHJvbWl1 bT4gcm9vdCA9IExheWVyQ2hyb21pdW06OmNyZWF0ZSgpOworICAgIFJlZlB0cjxMYXllckNocm9t aXVtPiBjaGlsZCA9IExheWVyQ2hyb21pdW06OmNyZWF0ZSgpOworICAgIHJvb3QtPmFkZENoaWxk KGNoaWxkKTsKKyAgICBPd25QdHI8RmFrZUNDTGF5ZXJUcmVlSG9zdD4gbGF5ZXJUcmVlSG9zdChG YWtlQ0NMYXllclRyZWVIb3N0OjpjcmVhdGUoKSk7CisgICAgbGF5ZXJUcmVlSG9zdC0+c2V0Um9v dExheWVyKHJvb3QpOworICAgIGxheWVyVHJlZUhvc3QuY2xlYXIoKTsKKyAgICBXZWJLaXQ6Oldl YkNvbXBvc2l0b3I6OnNodXRkb3duKCk7Cit9CisKIGNsYXNzIE1vY2tMYXllckNocm9taXVtIDog cHVibGljIExheWVyQ2hyb21pdW0gewogcHVibGljOgogICAgIGJvb2wgbmVlZHNEaXNwbGF5KCkg Y29uc3QgeyByZXR1cm4gbV9uZWVkc0Rpc3BsYXk7IH0K