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.
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.
Created attachment 324391 [details]
Comment on attachment 324391 [details]
Change looks right, but I think this needs tests.
(In reply to Simon Fraser (smfr) from comment #3)
> Comment on attachment 324391 [details]
> 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).
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.
+ if (frameView.frame().isMainFrame())
That would allow to save some calculation and to have one case less to consider for bug 176914.
Created attachment 325001 [details]
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 on attachment 325001 [details]
Looks good to me, r+.
Committed r224914: <https://trac.webkit.org/changeset/224914>