Bug 81114

Summary: ASSERT(!needsLayout()) in FrameView::paintContents() with multiple nested iframes, when frame flattening is on
Product: WebKit Reporter: zalan <zalan>
Component: Layout and RenderingAssignee: zalan <zalan>
Status: RESOLVED FIXED    
Severity: Normal CC: dglazkov, kenneth, koivisto, max.hong.shen, webkit.review.bot, yael
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
URL: http://www.iol.ie/~mariah/dbrooks/delon.html
Attachments:
Description Flags
Patch
none
Archive of layout-test-results from ec2-cr-linux-01
none
Patch
none
Patch
koivisto: review+, koivisto: commit-queue-
Patch none

Description zalan 2012-03-14 08:41:13 PDT
In case of multiple nested iframes, when the bottom most iframe has a blocking resource and there's a call to paint the tree, before the blocking resource arrives, FrameView::paintContens() asserts with !needsLayout().

this behavior can be seen at www.iol.ie/~mariah/dbrooks/delon.html when clicking on Tag Board link.

simplified test case:
<html><body><iframe src="data:text/html,<html><body><iframe src='http://dl.dropbox.com/u/36138203/cbox.html'></iframe></body></html>"></iframe></body></html>
where cbox.html is a simple page with a blocking resource.
<html><head><script src="notfound.js" type="text/javascript"></script></head><body></body></html>
Comment 1 zalan 2012-03-14 09:43:12 PDT
steps leading to assertion:

1, When the bottommost iframes loads, FrameView/RenderView are constructed and get marked dirty. (markContainingBlocksForLayout )
2, Parser stops at the script element.
3, Viewport requests painting.
5, updateLayoutAndStyleIfNeededRecursive(mainframe) is called to clean the render tree recursively before performing painting.
4, At this point only the newly constructed, bottommost FrameView is dirty. Both its parent and its parent's parent (mainframe) are clean (due to previous layouts)
5, layout() is called on the bottommost FrameView, which in turn re-initiates the layout flow from the topmost FrameView and expects that the layouting will reach itself.
6, Since parent FrameViews are clean, layouting stops at the topmost frame.
7, Bottommost FrameView's layout returns with dirty tree.

No assertion happens:
1, When the blocking resource is not present and parsing hits <body>.
HTMLBodyElement::insertedIntoDocument() schedules relayout on the current (bottommost) FrameView, which in turn marks its parent dirty.

2, When there's one less nested iframe.
The layout works accidentally in this case. At step 6, when layout(mainframe) performs post layout tasks, it calls updateWidgetPositions(), which eventually calls layout on direct (dirty)child frames. So getting the bottommost tree clean does not happen as the result of the normal layout flow (FrameView(mainframe)->RenderView->...->RenderIFrame->FrameView(child)), but as the result of perform post layout task.
Comment 2 zalan 2012-03-26 11:43:57 PDT
Created attachment 133856 [details]
Patch
Comment 3 WebKit Review Bot 2012-03-26 12:56:15 PDT
Comment on attachment 133856 [details]
Patch

Attachment 133856 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12145013

New failing tests:
http/tests/misc/iframe-flattening-3level-nesting-with-blocking-resource.html
Comment 4 WebKit Review Bot 2012-03-26 12:56:22 PDT
Created attachment 133871 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
Comment 5 zalan 2012-03-27 08:09:22 PDT
Created attachment 134066 [details]
Patch
Comment 6 zalan 2012-03-27 08:10:45 PDT
"CONSOLE MESSAGE: line 7: Uncaught TypeError: Object [object Object] has no method 'setFrameFlatteningEnabled'"
chromium-ews failed on setFrameFlatteningEnabled(). Added to skip list similarly to all the other frame flattening tests.
Comment 7 zalan 2012-04-03 08:52:12 PDT
Created attachment 135337 [details]
Patch
Comment 8 Antti Koivisto 2012-04-03 12:28:55 PDT
Comment on attachment 135337 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=135337&action=review

> Source/WebCore/ChangeLog:9
> +        When frame flattening is on, childframes try to mark ancestors dirty to be able to restart
> +        layout from mainframe. When marking ancestor fails, childframe layout needs to continue normally.

You should explain what the case where marking ancestors fails is and why it is not an error.

> LayoutTests/platform/chromium/test_expectations.txt:724
>  WONTFIX SKIP : fast/frames/flattening = TIMEOUT
> +WONTFIX SKIP : http/tests/misc/iframe-flattening-3level-nesting-with-blocking-resource.html = TIMEOUT

Please check that other platforms are ok with this when landing.
Comment 9 zalan 2012-04-03 13:47:02 PDT
(In reply to comment #8)
> (From update of attachment 135337 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=135337&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        When frame flattening is on, childframes try to mark ancestors dirty to be able to restart
> > +        layout from mainframe. When marking ancestor fails, childframe layout needs to continue normally.
> 
> You should explain what the case where marking ancestors fails is and why it is not an error.
> 
> > LayoutTests/platform/chromium/test_expectations.txt:724
> >  WONTFIX SKIP : fast/frames/flattening = TIMEOUT
> > +WONTFIX SKIP : http/tests/misc/iframe-flattening-3level-nesting-with-blocking-resource.html = TIMEOUT
> 
> Please check that other platforms are ok with this when landing.
other platforms do implement layoutTestController.setFrameFlatteningEnabled
Comment 10 zalan 2012-04-03 13:57:29 PDT
Created attachment 135418 [details]
Patch
Comment 11 WebKit Review Bot 2012-04-03 14:35:25 PDT
Comment on attachment 135418 [details]
Patch

Clearing flags on attachment: 135418

Committed r113091: <http://trac.webkit.org/changeset/113091>
Comment 12 WebKit Review Bot 2012-04-03 14:35:43 PDT
All reviewed patches have been landed.  Closing bug.