Bug 178508

Summary: Consider non-main frames for AsyncScrollingCoordinator::frameViewRootLayerDidChange
Product: WebKit Reporter: Frédéric Wang (:fredw) <fred.wang>
Component: FramesAssignee: Frédéric Wang (:fredw) <fred.wang>
Status: RESOLVED FIXED    
Severity: Normal CC: buildbot, cmarcelo, fred.wang, jamesr, luiz, simon.fraser, tonikitoo, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 176914    
Attachments:
Description Flags
Patch
none
Alternative Patch tonikitoo: review+

Frédéric Wang (:fredw)
Reported 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?
Attachments
Patch (5.42 KB, patch)
2017-10-20 06:50 PDT, Frédéric Wang (:fredw) 		no flags
DetailsFormatted DiffDiff
Alternative Patch (3.13 KB, patch)
2017-10-26 06:16 PDT, Frédéric Wang (:fredw) 		tonikitoo: review+
DetailsFormatted DiffDiff
View All Add attachment proposed patch, testcase, etc.
Frédéric Wang (:fredw)
Comment 1 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: 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)
Comment 2 2017-10-20 06:50:52 PDT
Created attachment 324391 [details] Patch
Simon Fraser (smfr)
Comment 3 2017-10-20 10:25:10 PDT
Comment on attachment 324391 [details] Patch Change looks right, but I think this needs tests.
Frédéric Wang (:fredw)
Comment 4 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).
Frédéric Wang (:fredw)
Comment 5 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 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)
Comment 6 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.
Antonio Gomes
Comment 7 2017-11-16 04:12:47 PST
Comment on attachment 325001 [details] Alternative Patch Looks good to me, r+.
Frédéric Wang (:fredw)
Comment 8 2017-11-16 05:49:08 PST
Committed r224914: <https://trac.webkit.org/changeset/224914>
Radar WebKit Bug Importer
Comment 9 2017-11-16 05:50:47 PST
<rdar://problem/35587294>
Note You need to log in before you can comment on or make changes to this bug.