Bug 195844 - Scrolling state nodes should hold references to GraphicsLayers
Summary: Scrolling state nodes should hold references to GraphicsLayers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Scrolling (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-03-15 22:03 PDT by Simon Fraser (smfr)
Modified: 2019-03-19 09:05 PDT (History)
10 users (show)

See Also:


Attachments
Patch (3.23 KB, patch)
2019-03-18 19:00 PDT, Simon Fraser (smfr)
thorton: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2019-03-15 22:03:19 PDT
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
Comment 1 Radar WebKit Bug Importer 2019-03-15 22:23:51 PDT
<rdar://problem/48949634>
Comment 2 Simon Fraser (smfr) 2019-03-18 19:00:37 PDT
Created attachment 365105 [details]
Patch
Comment 3 Simon Fraser (smfr) 2019-03-18 19:06:01 PDT
https://trac.webkit.org/r243126
Comment 4 Simon Fraser (smfr) 2019-03-18 21:14:07 PDT
Followup in https://trac.webkit.org/changeset/243129/webkit
Comment 5 Antti Koivisto 2019-03-19 06:39:29 PDT
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.
Comment 6 Simon Fraser (smfr) 2019-03-19 09:05:41 PDT
(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.