Bug 178508 - Consider non-main frames for AsyncScrollingCoordinator::frameViewRootLayerDidChange
Summary: Consider non-main frames for AsyncScrollingCoordinator::frameViewRootLayerDid...
Alias: None
Product: WebKit
Classification: Unclassified
Component: Frames (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Frédéric Wang (:fredw)
Keywords: InRadar
Depends on:
Blocks: 176914
  Show dependency treegraph
Reported: 2017-10-19 02:16 PDT by Frédéric Wang (:fredw)
Modified: 2017-11-16 05:50 PST (History)
8 users (show)

See Also:

Patch (5.42 KB, patch)
2017-10-20 06:50 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Alternative Patch (3.13 KB, patch)
2017-10-26 06:16 PDT, Frédéric Wang (:fredw)
tonikitoo: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Frédéric Wang (:fredw) 2017-10-19 02:16:22 PDT
AsyncScrollingCoordinator::frameViewRootLayerDidChange seems to assume that frameView is always a main-frame:

* It calls ensureRootStateNodeForFrameView, which always attaches a frame node with parentID = 0.
* It has an ASSERT to check m_scrollingStateTree->rootStateNode(), instead m_scrollingStateTree->stateNodeForID(frameView.scrollLayerID()).

It seems that the only place where AsyncScrollingCoordinator::frameViewRootLayerDidChange can be called with a non-main frame is RenderLayerCompositor::updateBacking.

I guess we should either ASSERT that frameView is a main frame, or update the function to consider non-main frames. In the latter case, I guess we should re-use the work of RenderLayerCompositor::scrollCoordinatedAncestorInParentOfFrame to calculate the parentID.

I'm making this blocking bug 176914, just because for non-main frames we might also need to calculate a childIndex or similar if we want to ensure z-index ordering.

@Simon: WDYT?
Comment 1 Frédéric Wang (:fredw) 2017-10-20 03:22:34 PDT
I just debugged it now with a simple test case. The iframe node is first created with nonzero parentID via:


After that, attachNode is called again with a wrong parentID == 0 value via:


It turns out that nodeTypeAndParentMatch always returns true when parentID == 0 so at the end attachToStateTree just returns the existing node id for the iframe and does not modify the state tree.

If the second stack trace was done before the scrolling node for the iframe is created, then ScrollingStateTree::attachNode would incorrectly set it as the new root. Also, if for some reason the iframe node was re-parented before that call, the state tree would not be updated. I'm not sure such bugs happen, but in any case, it is wrong for ensureRootStateNodeForFrameView to pass parentID = 0 for non-main frames.
Comment 2 Frédéric Wang (:fredw) 2017-10-20 06:50:52 PDT
Created attachment 324391 [details]
Comment 3 Simon Fraser (smfr) 2017-10-20 10:25:10 PDT
Comment on attachment 324391 [details]

Change looks right, but I think this needs tests.
Comment 4 Frédéric Wang (:fredw) 2017-10-20 12:03:04 PDT
(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 324391 [details]
> Patch
> Change looks right, but I think this needs tests.

Unfortunately, as I said in comment 1, the bad parendID = 0 does not seem to affect the scrolling tree. I tried with both static testcase and dynamic testcase (iframe inserted).
Comment 5 Frédéric Wang (:fredw) 2017-10-23 09:51:05 PDT
So checking again https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/rendering/RenderLayerCompositor.cpp#L1023

IIUC, updateScrollCoordinatedStatus will perform the first trace of comment 1 and hence creates the node while frameViewRootLayerDidChange will follow the second trace and hence just retrieve the existing node.

So I don't think it is possible to write a testcase that exhibits the issue. But apparently RenderLayerCompositor::updateBacking is the only place where frameViewRootLayerDidChange is called for non-main frames, so maybe an alternative option is to not call ensureRootStateNodeForFrameView at all for non-main frame:

@@ -219,8 +219,9 @@ void AsyncScrollingCoordinator::frameViewRootLayerDidChange(FrameView& frameView
     // If the root layer does not have a ScrollingStateNode, then we should create one.
-    ensureRootStateNodeForFrameView(frameView);
-    ASSERT(m_scrollingStateTree->rootStateNode());
+    if (frameView.frame().isMainFrame())
+        ensureRootStateNodeForFrameView(frameView);
+    ASSERT(m_scrollingStateTree->stateNodeForID(frameView.scrollLayerID()));
That would allow to save some calculation and to have one case less to consider for bug 176914.
Comment 6 Frédéric Wang (:fredw) 2017-10-26 06:16:13 PDT
Created attachment 325001 [details]
Alternative Patch

This is another version that just skips the call to attachToStateTree when the node already exists, which is the case for non-main frames.
Comment 7 Antonio Gomes 2017-11-16 04:12:47 PST
Comment on attachment 325001 [details]
Alternative Patch

Looks good to me, r+.
Comment 8 Frédéric Wang (:fredw) 2017-11-16 05:49:08 PST
Committed r224914: <https://trac.webkit.org/changeset/224914>
Comment 9 Radar WebKit Bug Importer 2017-11-16 05:50:47 PST