Consider non-main frames for AsyncScrollingCoordinator::frameViewRootLayerDidChange

• Component: Frames
• Status: RESOLVED FIXED

[Frédéric Wang (:fredw)]

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?

[Frédéric Wang (:fredw)]

I just debugged it now with a simple test case. The iframe node is first created with nonzero parentID via:

ScrollingStateTree::attachNode
AsyncScrollingCoordinator::attachToStateTree
RenderLayerCompositor::attachScrollingNode
RenderLayerCompositor::updateScrollCoordinationForThisFrame
RenderLayerCompositor::updateScrollCoordinatedLayer

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

ScrollingStateTree::attachNode
AsyncScrollingCoordinator::attachToStateTree
AsyncScrollingCoordinator::ensureRootStateNodeForFrameView
AsyncScrollingCoordinator::frameViewRootLayerDidChange
RenderLayerCompositor::updateBacking

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.

[Frédéric Wang (:fredw)]

Created attachment 324391 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 324391 [details]
Patch

Change looks right, but I think this needs tests.

[Frédéric Wang (:fredw)]

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

[Frédéric Wang (:fredw)]

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
         return;
     
     // 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.

[Frédéric Wang (:fredw)]

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.

[Antonio Gomes]

Comment on attachment 325001 [details]
Alternative Patch

Looks good to me, r+.

[Frédéric Wang (:fredw)]

Committed r224914: <https://trac.webkit.org/changeset/224914>

[Radar WebKit Bug Importer]

<rdar://problem/35587294>