Bug 227738 - Defend against stack overflow in GraphicsLayerCA::recursiveCommitChanges
Summary: Defend against stack overflow in GraphicsLayerCA::recursiveCommitChanges
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Compositing (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Cameron McCormack (:heycam)
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-07-06 23:39 PDT by Cameron McCormack (:heycam)
Modified: 2021-07-08 15:17 PDT (History)
2 users (show)

See Also:


Attachments
Patch (8.20 KB, patch)
2021-07-07 19:01 PDT, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff
Patch (8.41 KB, patch)
2021-07-07 23:28 PDT, Cameron McCormack (:heycam)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Cameron McCormack (:heycam) 2021-07-06 23:39:31 PDT
It looks like GraphicsLayerCA::recursiveCommitChanges can cause a stack overflow in some configurations.  We already have CommitState::treeDepth, which we use to avoid creating deep CALayer trees, so we should be able to stop the recursion at the same level.
Comment 1 Cameron McCormack (:heycam) 2021-07-06 23:39:57 PDT
<rdar://42584284>
Comment 2 Cameron McCormack (:heycam) 2021-07-06 23:49:02 PDT
All CommitState::treeDepth does currently is avoid hooking up the too-deep PlatformCALayers to their intended parents.  I'm slightly concerned with leaving other aspects of the PlatformCALayers out of date, though most of the LayerChange reasons are just about rendering, geometry, etc., and shouldn't be a big deal, especially when not hooked up.  ScrollingNodeChanged is a bit different, but we tend to check for scrolling node ID validity when we use them.

Another concern is that the treeDepth counts structural layers, and so a given GraphicsLayerCA may be too deep in one call to recursiveCommitChanges, and not be too deep the next.  In such a case we should then continue to process the old m_uncommittedChanges on the children, so that should be OK?
Comment 3 Cameron McCormack (:heycam) 2021-07-07 19:01:53 PDT
Created attachment 433107 [details]
Patch
Comment 4 Cameron McCormack (:heycam) 2021-07-07 23:28:55 PDT
Created attachment 433120 [details]
Patch
Comment 5 EWS 2021-07-08 15:17:47 PDT
Committed r279756 (239529@main): <https://commits.webkit.org/239529@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 433120 [details].