Bug 80155

Summary:

Frame flattening ASSERT(!needsLayout()) in FrameView::paintContents()

Product:

WebKit

Reporter:

alan <zalan>

Component:

Layout and Rendering

Assignee:

alan <zalan>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

kenneth, koivisto, mrobinson, simon.fraser, webkit.review.bot, yael

Priority:

Version:

528+ (Nightly build)

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
test case	none
Patch	none
Patch	none

alan

Reported 2012-03-02 05:49:19 PST

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>

Attachments
test case (204 bytes, text/html) 2012-03-02 05:49 PST, alan	no flags	Details
Patch (8.86 KB, patch) 2012-03-11 12:39 PDT, alan	no flags	Details Formatted Diff Diff
Patch (11.98 KB, patch) 2012-03-13 07:02 PDT, alan	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

alan

Comment 1 2012-03-02 05:49:58 PST

Created attachment 129891 [details] test case

alan

Comment 2 2012-03-02 05:52:43 PST

also repro on wired.com

alan

Comment 3 2012-03-02 07:15:30 PST

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

alan

Comment 4 2012-03-03 12:37:41 PST

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.

alan

Comment 5 2012-03-03 12:47:07 PST

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

alan

Comment 6 2012-03-11 12:39:25 PDT

Created attachment 131255 [details] Patch

Antti Koivisto

Comment 7 2012-03-12 09:20:07 PDT

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.

Antti Koivisto

Comment 8 2012-03-12 09:23:33 PDT

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.

Philippe Normand

Comment 9 2012-03-12 09:52:17 PDT

*** Bug 65530 has been marked as a duplicate of this bug. ***

alan

Comment 10 2012-03-13 07:02:41 PDT

Created attachment 131604 [details] Patch

Antti Koivisto

Comment 11 2012-03-14 09:58:24 PDT

Comment on attachment 131604 [details] Patch r=me

WebKit Review Bot

Comment 12 2012-03-14 13:23:10 PDT

Comment on attachment 131604 [details] Patch Clearing flags on attachment: 131604 Committed r110738: <http://trac.webkit.org/changeset/110738>

WebKit Review Bot

Comment 13 2012-03-14 13:23:15 PDT

All reviewed patches have been landed. Closing bug.

Martin Robinson

Comment 14 2012-04-05 18:19:08 PDT

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

alan

Comment 15 2012-04-05 18:25:12 PDT

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

Martin Robinson

Comment 16 2012-04-05 18:27:45 PDT

(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!

Note You need to log in before you can comment on or make changes to this bug.