Bug 195844 – Scrolling state nodes should hold references to GraphicsLayers

• Status: RESOLVED
• Resolution: FIXED
• Component: Scrolling

[Simon Fraser (smfr)]

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

[Radar WebKit Bug Importer]

<rdar://problem/48949634>

[Simon Fraser (smfr)]

Created attachment 365105 [details]
Patch

[Simon Fraser (smfr)]

https://trac.webkit.org/r243126

[Simon Fraser (smfr)]

Followup in https://trac.webkit.org/changeset/243129/webkit

[Antti Koivisto]

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.

[Simon Fraser (smfr)]

(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.