Summary: | Frame flattening ASSERT(!needsLayout()) in FrameView::paintContents() | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | zalan <zalan> | ||||||||
Component: | Layout and Rendering | Assignee: | zalan <zalan> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | kenneth, koivisto, mrobinson, simon.fraser, webkit.review.bot, yael | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Attachments: |
|
Description
zalan
2012-03-02 05:49:19 PST
Created attachment 129891 [details]
test case
also repro on wired.com the test case has 1, iframe which does not get flattened (fixed width/height + no scrolling) 2, offsetHeight which triggers sync layout Assert happens, because the iframe remains dirty even after a few layout cycles on both the main and the child frame. What happens here is 1, first layout cycle triggered by document.body.offsetHeight offsetHeight -> FrameView::layout(mainframe) -> RenderIFrame::layout(no frame flattening, so child FrameView::layout() is not called) -> FrameView::performPostLayoutTasks() -> RenderWidget::updateWidgetPosition() -> FrameView::layout(childframe) -> (nothing to layout yet) (after return back from layout(childframe) a recursive layout call on the mainframe leaves pending post layout tasks.) 2, second layout cycle triggered by finished parsing Document::implicitClose() -> FrameView::layout(mainframe) -> FrameView::performPostLayoutTasks(pending post layout from 1st cycle) -> RenderWidget::updateWidgetPosition() -> FrameView::layout(childframe) -> nested layout call? no -> start layout from parent frame -> FrameView::layout(mainframe) -> RenderIFrame::layout(no frame flattening, so child FrameView::layout() is not called) -> FrameView::layout(childframe) early returns and it leaves the child frame dirty. 3, third layout cycle triggered by paint paint -> layoutIfNeeded() -> FrameView::layout(mainframe) -> FrameView::performPostLayoutTasks(pending post layout from 2nd cycle) -> RenderWidget::updateWidgetPosition() -> FrameView::layout(childframe) -> nested layout call? no -> start layout from parent frame -> FrameView::layout(mainframe) -> RenderIFrame::layout(no frame flattening, so child FrameView::layout() is not called) -> FrameView::layout(childframe) early returns and it leaves the child frame dirty. ->assert in paintContents() of the child FrameView() This bug occurs, when m_postLayoutTasksTimer has no time to hit, before FrameView::layout() is called. Calling postlayout task from FrameView::layout() ends up calling FrameView::layout(childframe) with a state (m_nestedLayoutCount == 0) where child frameview always initiates the layout from the top and since the RenderIFrame is not calling back to the child FrameView::layout(), child frame remains dirty. (The forced sync layout with offsetHeight at the beginning is needed in order to start the 2nd cycle with pending postlayout tasks.) There seems to be 2 issues with frame flattening code in this context: 1, FrameView::layout(childframe) wrongly assumes that if layout is re-initiated from parent (parentView->layout(allowSubtree);), it will eventually come back to the child frame to finished off layouting. If the associated iframe is not getting flattened, it is absolutely unnecessary to call on parentView->layout() for layouting purposes. (and it still leaves childframe dirty as FrameView::layout(childframe) is not getting called from RenderIFrame::layout()). The flattening code should be smarter to not to initiate parent layouts, if possible. 2, Good amount of the FrameView::layout(childframe) calls are originated from parentView->performPostLayoutTasks(). Now, performPostLayoutTasks() is called from layout() at multiple places with different nesting state, depending whether this is a pending postlayouttask to be executed before root->layout() (no nesting), or normal sync postlayout after root->layout() (yes nesting). layout(childframe) relies on the parent's nesting state (m_nestedLayoutCount) to decide, if layout should be re-initiated from parent. In both cases, we are in parent's layout, and still child FrameView::layout() behaves differently. btw, this is not reproducible on mac port all the time due to timing issues. (m_postLayoutTasksTimer hits before the 2nd layout is initiated by documentClose()) Created attachment 131255 [details]
Patch
Comment on attachment 131255 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131255&action=review r=me > Source/WebCore/ChangeLog:9 > + This patch ensures that iframes only start layouting from the topmost parent, > + when the they are going to be flattened. It would be good to explain how it ensures it. Comment on attachment 131255 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=131255&action=review > LayoutTests/fast/frames/flattening/iframe-flattening-fixed-width-and-height-no-scrolling-with-js-forced-layout.html:5 > +<html> > +<head> > + <script type="text/javascript"> > + if (window.layoutTestController) { > + layoutTestController.waitUntilDone(); This should use layoutTestController.dumpAsText(). The point is to (not) hit the assert, the output is not important. This would probably not be cross-platform. *** Bug 65530 has been marked as a duplicate of this bug. *** Created attachment 131604 [details]
Patch
Comment on attachment 131604 [details]
Patch
r=me
Comment on attachment 131604 [details] Patch Clearing flags on attachment: 131604 Committed r110738: <http://trac.webkit.org/changeset/110738> All reviewed patches have been landed. Closing bug. (In reply to comment #10) > Created an attachment (id=131604) [details] > Patch Zalan, it makes gardening a bit easier when you unskip tests upon fixing a bug. I'll unskip the faililng tests for GTK+ now. (In reply to comment #14) > (In reply to comment #10) > > Created an attachment (id=131604) [details] [details] > > Patch > > Zalan, it makes gardening a bit easier when you unskip tests upon fixing a bug. I'll unskip the faililng tests for GTK+ now. Yeah, sorry. Thanks for doing it. (In reply to comment #15) > > Zalan, it makes gardening a bit easier when you unskip tests upon fixing a bug. I'll unskip the faililng tests for GTK+ now. > > Yeah, sorry. Thanks for doing it. Not a problem. Thanks for fixing the bug! |