195844 2019-03-15 22:03:19 -0700 Scrolling state nodes should hold references to GraphicsLayers 2019-03-19 09:05:41 -0700 1 1 1 Unclassified WebKit Scrolling WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 simon.fraser simon.fraser cmarcelo ews-watchlist fred.wang jamesr koivisto luiz simon.fraser thorton tonikitoo webkit-bug-importer oldest_to_newest 1517365 0 simon.fraser 2019-03-15 22:03:19 -0700 I'm seeing crashes with bad GraphicsLayers in the scrolling state tree. For example: 1. Enable async overflow scrolling on MiniBrowser on macOS 2. Load https://palace-games.com/ 3. Click "Reserve Your Room Today" 4. Scroll the overlay Crash: * frame #0: 0x000000011b468ec9 WebCore`WebCore::LayerRepresentation::toRepresentation(this=0x000000014e6b9e48, representation=PlatformLayerRepresentation) const at ScrollingStateNode.h:166:55 frame #1: 0x000000011b474b18 WebCore`WebCore::ScrollingStateNode::ScrollingStateNode(this=0x000000014fe08618, stateNode=0x000000014e6b9e10, adoptiveTree=0x000000013b922d68) at ScrollingStateNode.cpp:55:36 frame #2: 0x000000011b4772ba WebCore`WebCore::ScrollingStatePositionedNode::ScrollingStatePositionedNode(this=0x000000014fe08618, node=0x000000014e6b9e10, adoptiveTree=0x000000013b922d68) at ScrollingStatePositionedNode.cpp:49:7 frame #3: 0x000000011b477395 WebCore`WebCore::ScrollingStatePositionedNode::ScrollingStatePositionedNode(this=0x000000014fe08618, node=0x000000014e6b9e10, adoptiveTree=0x000000013b922d68) at ScrollingStatePositionedNode.cpp:52:1 frame #4: 0x000000011b477487 WebCore`WebCore::ScrollingStatePositionedNode::clone(this=0x000000014e6b9e10, adoptiveTree=0x000000013b922d68) at ScrollingStatePositionedNode.cpp:59:26 frame #5: 0x000000011b474eaf WebCore`WebCore::ScrollingStateNode::cloneAndReset(this=0x000000014e6b9e10, adoptiveTree=0x000000013b922d68) at ScrollingStateNode.cpp:79:24 frame #6: 0x000000011b474fc0 WebCore`WebCore::ScrollingStateNode::cloneAndResetChildren(this=0x000000016ad7f000, clone=0x000000016dfa0500, adoptiveTree=0x000000013b922d68) at ScrollingStateNode.cpp:95:34 frame #7: 0x000000011b474ed1 WebCore`WebCore::ScrollingStateNode::cloneAndReset(this=0x000000016ad7f000, adoptiveTree=0x000000013b922d68) at ScrollingStateNode.cpp:84:5 frame #8: 0x000000011b474fc0 WebCore`WebCore::ScrollingStateNode::cloneAndResetChildren(this=0x00000001437da960, clone=0x0000000151bd1840, adoptiveTree=0x000000013b922d68) at ScrollingStateNode.cpp:95:34 frame #9: 0x000000011b474ed1 WebCore`WebCore::ScrollingStateNode::cloneAndReset(this=0x00000001437da960, adoptiveTree=0x000000013b922d68) at ScrollingStateNode.cpp:84:5 frame #10: 0x000000011b474fc0 WebCore`WebCore::ScrollingStateNode::cloneAndResetChildren(this=0x000000014170de00, clone=0x0000000161bfd480, adoptiveTree=0x000000013b922d68) at ScrollingStateNode.cpp:95:34 frame #11: 0x000000011b474ed1 WebCore`WebCore::ScrollingStateNode::cloneAndReset(this=0x000000014170de00, adoptiveTree=0x000000013b922d68) at ScrollingStateNode.cpp:84:5 frame #12: 0x000000011b474fc0 WebCore`WebCore::ScrollingStateNode::cloneAndResetChildren(this=0x000000014366f000, clone=0x000000016dfa0280, adoptiveTree=0x000000013b922d68) at ScrollingStateNode.cpp:95:34 frame #13: 0x000000011b474ed1 WebCore`WebCore::ScrollingStateNode::cloneAndReset(this=0x000000014366f000, adoptiveTree=0x000000013b922d68) at ScrollingStateNode.cpp:84:5 frame #14: 0x000000011b47beb5 WebCore`WebCore::ScrollingStateTree::commit(this=0x00000001384bd3a8, preferredLayerRepresentation=PlatformLayerRepresentation) at ScrollingStateTree.cpp:296:115 frame #15: 0x0000000118a11cfb WebCore`WebCore::ScrollingCoordinatorMac::commitTreeState(this=0x00000001384f0900) at ScrollingCoordinatorMac.mm:123:70 frame #16: 0x0000000118a12339 WebCore`WebCore::ScrollingCoordinatorMac::commitTreeStateIfNeeded(this=0x00000001384f0900) at ScrollingCoordinatorMac.mm:81:5 frame #17: 0x0000000110dba8ce WebKit`WebKit::TiledCoreAnimationDrawingArea::flushLayers(this=0x000000013847b000) at TiledCoreAnimationDrawingArea.mm:498:35 frame #18: 0x0000000110dbec15 WebKit`WebKit::TiledCoreAnimationDrawingArea::layerFlushRunLoopCallback(this=0x000000013847b000) at TiledCoreAnimationDrawingArea.mm:946:5 1517373 1 webkit-bug-importer 2019-03-15 22:23:51 -0700 <rdar://problem/48949634> 1518080 2 365105 simon.fraser 2019-03-18 19:00:37 -0700 Created attachment 365105 Patch 1518083 3 simon.fraser 2019-03-18 19:06:01 -0700 https://trac.webkit.org/r243126 1518125 4 simon.fraser 2019-03-18 21:14:07 -0700 Followup in https://trac.webkit.org/changeset/243129/webkit 1518232 5 365105 koivisto 2019-03-19 06:39:29 -0700 Comment on attachment 365105 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=365105&action=review > Source/WebCore/ChangeLog:10 > + GraphicsLayers are refcounted, and the scrolling tree keeps GraphicsLayer pointers, > + so for safely the scrolling tree should store RefPtr<GraphicsLayer> instead. Why is the state tree referencing detached layers? While the change is a good idea in general, as a fix for this issue it appears to be just hiding the actual bug. We should still be asserting this is not happening. 1518291 6 simon.fraser 2019-03-19 09:05:41 -0700 (In reply to Antti Koivisto from comment #5) > Comment on attachment 365105 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=365105&action=review > > > Source/WebCore/ChangeLog:10 > > + GraphicsLayers are refcounted, and the scrolling tree keeps GraphicsLayer pointers, > > + so for safely the scrolling tree should store RefPtr<GraphicsLayer> instead. > > Why is the state tree referencing detached layers? While the change is a > good idea in general, as a fix for this issue it appears to be just hiding > the actual bug. We should still be asserting this is not happening. It should never do so, because the scrolling tree is updated at the same time we do compositing updates. I wasn't able to reproduce the crash I saw earlier (possibly because r243120 fixed it). But if we have a bug, I'd prefer it not to turn into a UAF. 365105 2019-03-18 19:00:37 -0700 2019-03-18 19:01:28 -0700 Patch bug-195844-20190318190036.patch text/plain 3305 simon.fraser U3VidmVyc2lvbiBSZXZpc2lvbjogMjQzMTIwCmRpZmYgLS1naXQgYS9Tb3VyY2UvV2ViQ29yZS9D aGFuZ2VMb2cgYi9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKaW5kZXggNmIwOGY2MmU4ZDRlYjEx N2YwMzZlZWVmM2JhNTUwYzgxMmM1NjkxNS4uOTdkZTgwYzM1YTliODdiNWVlMjlkMmQwYTcxNDli NTgwZWRiYjQ5NiAxMDA2NDQKLS0tIGEvU291cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCisrKyBiL1Nv dXJjZS9XZWJDb3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDIxIEBACisyMDE5LTAzLTE4ICBTaW1v biBGcmFzZXIgIDxzaW1vbi5mcmFzZXJAYXBwbGUuY29tPgorCisgICAgICAgIFNjcm9sbGluZyBz dGF0ZSBub2RlcyBzaG91bGQgaG9sZCByZWZlcmVuY2VzIHRvIEdyYXBoaWNzTGF5ZXJzCisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xOTU4NDQKKyAgICAg ICAgPHJkYXI6Ly9wcm9ibGVtLzQ4OTQ5NjM0PgorCisgICAgICAgIFJldmlld2VkIGJ5IE5PQk9E WSAoT09QUyEpLgorCisgICAgICAgIEdyYXBoaWNzTGF5ZXJzIGFyZSByZWZjb3VudGVkLCBhbmQg dGhlIHNjcm9sbGluZyB0cmVlIGtlZXBzIEdyYXBoaWNzTGF5ZXIgcG9pbnRlcnMsCisgICAgICAg IHNvIGZvciBzYWZlbHkgdGhlIHNjcm9sbGluZyB0cmVlIHNob3VsZCBzdG9yZSBSZWZQdHI8R3Jh cGhpY3NMYXllcj4gaW5zdGVhZC4KKworICAgICAgICBJIHJlbW92ZWQgdGhlIHVuaW9uIChzaW5j ZSBpdCB3b3VsZCBiZSB3ZWlyZCB3aXRoIGEgUmVmUHRyIGFuZCByYXcgcG9pbnRlcikuIFRoaXMg Y29kZQorICAgICAgICBzaG91bGQgcHJvYmFibHkgdXNlIFdURjo6VmFyaWFudDw+IGluIGZ1dHVy ZS4KKworICAgICAgICAqIHBhZ2Uvc2Nyb2xsaW5nL1Njcm9sbGluZ1N0YXRlTm9kZS5oOgorICAg ICAgICAoV2ViQ29yZTo6TGF5ZXJSZXByZXNlbnRhdGlvbjo6TGF5ZXJSZXByZXNlbnRhdGlvbik6 CisgICAgICAgIChXZWJDb3JlOjpMYXllclJlcHJlc2VudGF0aW9uOjpvcGVyYXRvciBHcmFwaGlj c0xheWVyKiBjb25zdCk6CisKIDIwMTktMDMtMTggIFNpbW9uIEZyYXNlciAgPHNpbW9uLmZyYXNl ckBhcHBsZS5jb20+CiAKICAgICAgICAgQ3Jhc2ggd2hlbiByZWxvYWRpbmcgdGVzdCB3aXRoIGFz eW5jIG92ZXJmbG93IHNjcm9sbGluZwpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvcGFnZS9z Y3JvbGxpbmcvU2Nyb2xsaW5nU3RhdGVOb2RlLmggYi9Tb3VyY2UvV2ViQ29yZS9wYWdlL3Njcm9s bGluZy9TY3JvbGxpbmdTdGF0ZU5vZGUuaAppbmRleCAzNTg2NDhmMjNkNDEzNzRkN2Y0MDYzZGU4 MzY0NzZkY2U3YWFjMGZhLi41MzJjMGI3M2Y4NzA0NGFjNzJjZTYwYzI3ZTRmNjgxNjJhYTdlMTdj IDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9wYWdlL3Njcm9sbGluZy9TY3JvbGxpbmdTdGF0 ZU5vZGUuaAorKysgYi9Tb3VyY2UvV2ViQ29yZS9wYWdlL3Njcm9sbGluZy9TY3JvbGxpbmdTdGF0 ZU5vZGUuaApAQCAtNTgsMTEgKzU4LDcgQEAgcHVibGljOgogICAgICAgICBQbGF0Zm9ybUxheWVy SURSZXByZXNlbnRhdGlvbgogICAgIH07CiAKLSAgICBMYXllclJlcHJlc2VudGF0aW9uKCkKLSAg ICAgICAgOiBtX2dyYXBoaWNzTGF5ZXIobnVsbHB0cikKLSAgICAgICAgLCBtX2xheWVySUQoMCkK LSAgICAgICAgLCBtX3JlcHJlc2VudGF0aW9uKEVtcHR5UmVwcmVzZW50YXRpb24pCi0gICAgeyB9 CisgICAgTGF5ZXJSZXByZXNlbnRhdGlvbigpID0gZGVmYXVsdDsKIAogICAgIExheWVyUmVwcmVz ZW50YXRpb24oR3JhcGhpY3NMYXllciogZ3JhcGhpY3NMYXllcikKICAgICAgICAgOiBtX2dyYXBo aWNzTGF5ZXIoZ3JhcGhpY3NMYXllcikKQEAgLTcyLDE1ICs2OCwxMyBAQCBwdWJsaWM6CiAKICAg ICBMYXllclJlcHJlc2VudGF0aW9uKFBsYXRmb3JtTGF5ZXIqIHBsYXRmb3JtTGF5ZXIpCiAgICAg ICAgIDogbV90eXBlbGVzc1BsYXRmb3JtTGF5ZXIobWFrZVBsYXRmb3JtTGF5ZXJUeXBlbGVzcyhw bGF0Zm9ybUxheWVyKSkKLSAgICAgICAgLCBtX2xheWVySUQoMCkKICAgICAgICAgLCBtX3JlcHJl c2VudGF0aW9uKFBsYXRmb3JtTGF5ZXJSZXByZXNlbnRhdGlvbikKICAgICB7CiAgICAgICAgIHJl dGFpblBsYXRmb3JtTGF5ZXIobV90eXBlbGVzc1BsYXRmb3JtTGF5ZXIpOwogICAgIH0KIAogICAg IExheWVyUmVwcmVzZW50YXRpb24oR3JhcGhpY3NMYXllcjo6UGxhdGZvcm1MYXllcklEIGxheWVy SUQpCi0gICAgICAgIDogbV9ncmFwaGljc0xheWVyKG51bGxwdHIpCi0gICAgICAgICwgbV9sYXll cklEKGxheWVySUQpCisgICAgICAgIDogbV9sYXllcklEKGxheWVySUQpCiAgICAgICAgICwgbV9y ZXByZXNlbnRhdGlvbihQbGF0Zm9ybUxheWVySURSZXByZXNlbnRhdGlvbikKICAgICB7CiAgICAg fQpAQCAtMTAzLDcgKzk3LDcgQEAgcHVibGljOgogICAgIG9wZXJhdG9yIEdyYXBoaWNzTGF5ZXIq KCkgY29uc3QKICAgICB7CiAgICAgICAgIEFTU0VSVChtX3JlcHJlc2VudGF0aW9uID09IEdyYXBo aWNzTGF5ZXJSZXByZXNlbnRhdGlvbik7Ci0gICAgICAgIHJldHVybiBtX2dyYXBoaWNzTGF5ZXI7 CisgICAgICAgIHJldHVybiBtX2dyYXBoaWNzTGF5ZXIuZ2V0KCk7CiAgICAgfQogCiAgICAgb3Bl cmF0b3IgUGxhdGZvcm1MYXllciooKSBjb25zdApAQCAtMTc5LDEzICsxNzMsMTAgQEAgcHJpdmF0 ZToKICAgICBXRUJDT1JFX0VYUE9SVCBzdGF0aWMgUGxhdGZvcm1MYXllciogbWFrZVBsYXRmb3Jt TGF5ZXJUeXBlZCh2b2lkKiB0eXBlbGVzc1BsYXRmb3JtTGF5ZXIpOwogICAgIFdFQkNPUkVfRVhQ T1JUIHN0YXRpYyB2b2lkKiBtYWtlUGxhdGZvcm1MYXllclR5cGVsZXNzKFBsYXRmb3JtTGF5ZXIq KTsKIAotICAgIHVuaW9uIHsKLSAgICAgICAgR3JhcGhpY3NMYXllciogbV9ncmFwaGljc0xheWVy OwotICAgICAgICB2b2lkKiBtX3R5cGVsZXNzUGxhdGZvcm1MYXllcjsKLSAgICB9OwotCi0gICAg R3JhcGhpY3NMYXllcjo6UGxhdGZvcm1MYXllcklEIG1fbGF5ZXJJRDsKLSAgICBUeXBlIG1fcmVw cmVzZW50YXRpb247CisgICAgUmVmUHRyPEdyYXBoaWNzTGF5ZXI+IG1fZ3JhcGhpY3NMYXllcjsK KyAgICB2b2lkKiBtX3R5cGVsZXNzUGxhdGZvcm1MYXllciB7IG51bGxwdHIgfTsKKyAgICBHcmFw aGljc0xheWVyOjpQbGF0Zm9ybUxheWVySUQgbV9sYXllcklEIHsgMCB9OworICAgIFR5cGUgbV9y ZXByZXNlbnRhdGlvbiB7IEVtcHR5UmVwcmVzZW50YXRpb24gfTsKIH07CiAKIGNsYXNzIFNjcm9s bGluZ1N0YXRlTm9kZSA6IHB1YmxpYyBSZWZDb3VudGVkPFNjcm9sbGluZ1N0YXRlTm9kZT4gewo=