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

RESOLVED FIXED Bug 80155

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

https://bugs.webkit.org/show_bug.cgi?id=80155

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

zalan

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, zalan	no flags	Details
Patch (8.86 KB, patch) 2012-03-11 12:39 PDT, zalan	no flags	Details Formatted Diff Diff
Patch (11.98 KB, patch) 2012-03-13 07:02 PDT, zalan	no flags	Details Formatted Diff Diff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.

zalan

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

Created attachment 129891 [details] test case

zalan

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

also repro on wired.com

zalan

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

zalan

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.

zalan

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

zalan

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

zalan

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.

zalan

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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version 528+ (Nightly build)

Hardware Unspecified

OS Unspecified

Product WebKit

Component Layout and Rendering

Assignee

zalan

Reported

2012-03-02 05:49 PST

Modified

2012-04-05 18:27 PDT History

CC List

6 users Show

URL

Keywords

Depends on

Blocks