FrameView::paintContents() asserts when frame flattening is on with the following simplified test case (originally is seen on nytimes.com) <html><head></head><body><iframe src="data:text/html;charset=utf-8,<html><body>test</body></html>" scrolling="no" height="50" width="50"></iframe><script>document.body.offsetHeight</script></body></html>
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!