Bug 61491 – Frame flattening is broken with nested frames

• Component: Frames
• Status: RESOLVED FIXED

[Simon Fraser (smfr)]

Frame flattening is totally broken if there are nested frames.  The inner frame ends up with size 0x0. There are no tests for this.

[Simon Fraser (smfr)]

Created attachment 94888 [details]
Testcase

[Simon Fraser (smfr)]

Seems to be an issue when the intermediate iframe has "scrolling=no"

[Simon Fraser (smfr)]

Seems like RenderIFrame::flattenFrame() decides not to flatten the first time around because the absoluteBoundingBoxRect().intersects() tests fails, because view->contentsSize() is 0,0 at this point (we haven't finished the first layout). That test is bogus, and nothing later comes back to cause us to re-evaluate that decision.

[Kenneth Rohde Christiansen]

(In reply to comment #3)
> Seems like RenderIFrame::flattenFrame() decides not to flatten the first time around because the absoluteBoundingBoxRect().intersects() tests fails, because view->contentsSize() is 0,0 at this point (we haven't finished the first layout). That test is bogus, and nothing later comes back to cause us to re-evaluate that decision.

So you suggest that we reevaluate whether we should flatten or not?

[Simon Fraser (smfr)]

I'm not sure that's the source of the bug, but doing flattening based on a bogus visibility check certainly seems wrong.

[Kenneth Rohde Christiansen]

I think the bug is elsewhere. The nested iframe has a fixed size and thus the flattenFrame() method is returning false:

 92     if (!isScrollable && style()->width().isFixed()
 93         && style()->height().isFixed())
             return false;

Regarding the absoluteBoundingBoxRect().intersects() test. Basically to check if something is offscreen, either of the left values of absoluteBoundingBoxRect() needs to be negative.

The code could probably be rewritten to something like:

    // Do not flatten offscreen inner frames during frame flattening.
    IntRect rect = absoluteBoundingBoxRect();
    int rightX = rect.x() + rect.width();
    int rightY = rect.y() + rect.height();

    return rightX > 0 && rightY > 0;

[Kenneth Rohde Christiansen]

(In reply to comment #5)
> I'm not sure that's the source of the bug, but doing flattening based on a bogus visibility check certainly seems wrong.

I totally agree with that :-)

[Simon Fraser (smfr)]

Right, I think the isScrollable check is the real source of the bug.

[Kenneth Rohde Christiansen]

(In reply to comment #8)
> Right, I think the isScrollable check is the real source of the bug.

Well, make it return true to flattenFrame makes it work, but I don't see why it becomes 0x0 size if flattenFrame() is false. Why should it be laid out differently than without frame flattening enabled. The first iframe IS flattened.

[Kenneth Rohde Christiansen]

@@ -115,7 +122,8 @@ void RenderIFrame::layout()
     if (flattenFrame()) {
         layoutWithFlattening(style()->width().isFixed(), style()->height().isFixed());
         return;
-    }
+    } else
+        updateWidgetPosition();

The above makes it work, so apparently updateWidgetPosition() needs to be called when frame flattening is on, but maybe this is not the right place.

[Simon Fraser (smfr)]

Don't we call updateWidgetPositions() after layout anyway? I'm not sure why frame flattening has to call it explicitly.

[Simon Fraser (smfr)]

Oh, updateWidgetPositions() doesn't go into frames/iframes.

But doing layout on a FrameView should update the positions of contained widgets, so why does that not happen in this case?

[Simon Fraser (smfr)]

performPostLayoutTasks() is being skipped because inSubframeLayoutWithFrameFlattening is true. So we never update the widget inside the flattened iframe.

[Simon Fraser (smfr)]

inSubframeLayoutWithFrameFlattening  checks were added in http://trac.webkit.org/changeset/77988 to fix bug 52449, but I don't see how performPostLayoutTasks() gets called at all on nested, flattened frames now.

[Yael]

(In reply to comment #14)
> inSubframeLayoutWithFrameFlattening  checks were added in http://trac.webkit.org/changeset/77988 to fix bug 52449, but I don't see how performPostLayoutTasks() gets called at all on nested, flattened frames now.

performPostLayoutTasks() is called on a timer only for flattened iframes.
That is becuase it calls resizeEvent() and sites like gmail.com would use that event to relayout themselves. That caused the bug 52449.

[Yael]

Created attachment 95303 [details]
Patch.

After r77988, when frame flattening is enabled, performPostLayoutTasks() is called on a timer for iframes.
This causes layout of nested iframes to sometimes happen asynchronously, but WebCore expects layout to always finish synchronously.
Added a call to updateWidgetPosition() for cases that performPostLayoutTasks() is now happening asynchronously.

[Kenneth Rohde Christiansen]

Comment on attachment 95303 [details]
Patch.

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

> Source/WebCore/page/FrameView.cpp:1041
>      if (!m_hasPendingPostLayoutTasks) {
> -        if (!m_inSynchronousPostLayout && !inSubframeLayoutWithFrameFlattening) {
> -            m_inSynchronousPostLayout = true;
> -            // Calls resumeScheduledEvents()
> -            performPostLayoutTasks();
> -            m_inSynchronousPostLayout = false;
> +        if (!m_inSynchronousPostLayout) {

who not do

if (!m_hasPendingPostLayoutTasks && !m_inSynchronousPostLayout) ?

> LayoutTests/ChangeLog:7
> [phone]  Yael Aharon  <[e-mail]>
> +
> +        Reviewed by NOBODY (OOPS!).
> +
> +        Frame flattening is broken with nested frames
> +        https://bugs.webkit.org/show_bug.cgi?id=61491
> +

Shouldn't you give some credit to Simon Fraser for the test case?

[Yael]

(In reply to comment #17)
> (From update of attachment 95303 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95303&action=review
> 
> > Source/WebCore/page/FrameView.cpp:1041
> >      if (!m_hasPendingPostLayoutTasks) {
> > -        if (!m_inSynchronousPostLayout && !inSubframeLayoutWithFrameFlattening) {
> > -            m_inSynchronousPostLayout = true;
> > -            // Calls resumeScheduledEvents()
> > -            performPostLayoutTasks();
> > -            m_inSynchronousPostLayout = false;
> > +        if (!m_inSynchronousPostLayout) {
> 
> who not do
> 
> if (!m_hasPendingPostLayoutTasks && !m_inSynchronousPostLayout) ?
> 
The above condition is not correct for the second half of the if statement.

[Yael]

Created attachment 95425 [details]
Patch.

Put Simon Fraser's name in the LayoutTests Changelog.

[Kenneth Rohde Christiansen]

Comment on attachment 95425 [details]
Patch.

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

> Source/WebCore/page/FrameView.cpp:1043
> +                m_frame->contentRenderer()->updateWidgetPositions();

Are we ensured to always have a contentRenderer?

[Yael]

(In reply to comment #20)
> (From update of attachment 95425 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95425&action=review
> 
> > Source/WebCore/page/FrameView.cpp:1043
> > +                m_frame->contentRenderer()->updateWidgetPositions();
> 
> Are we ensured to always have a contentRenderer?

Yes, we are.
When layout starts, we check for a layout root and return if there is no root.
If m_layoutRoot exists, that is out root, otherwise, the root is the same as contentRenderer().

[Antonio Gomes]

> > > Source/WebCore/page/FrameView.cpp:1043
> > > +                m_frame->contentRenderer()->updateWidgetPositions();
> > 
> > Are we ensured to always have a contentRenderer?
> 
> Yes, we are.
> When layout starts, we check for a layout root and return if there is no root.
> If m_layoutRoot exists, that is out root, otherwise, the root is the same as contentRenderer().

An assert is highly recommended in such cases, then

[WebKit Commit Bot]

Comment on attachment 95425 [details]
Patch.

Clearing flags on attachment: 95425

Committed r87726: <http://trac.webkit.org/changeset/87726>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Yael]

We should also fix the issue described in comment #5.

[Ademar Reis]

Revision r87726 cherry-picked into qtwebkit-2.2 with commit de5d05b <http://gitorious.org/webkit/qtwebkit/commit/de5d05b>

[Yael]

Created attachment 95530 [details]
Patch.

Do not flatten offscreen iframes during frame flattening, if flattening will make them visible.

[Yael]

Created attachment 95532 [details]
Patch.

One more try. Patches with images are hard to manage :)

[Simon Fraser (smfr)]

Comment on attachment 95532 [details]
Patch.

I think you should test what happens when the iframes are moved after the first layout, so the invisible one becomes visible and vice versa. A good test would also include nested iframes,

[Yael]

Created attachment 95675 [details]
Patch.

Added a second test that has a nested iframe and scrolls the main frame so that the iframes become visible.

[Kenneth Rohde Christiansen]

Comment on attachment 95675 [details]
Patch.

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

> LayoutTests/fast/frames/flattening/iframe-flattening-out-of-view-and-scroll.html:12
> +                scrollTo(1200, 0);

shouldnt you do something to invalidate the layout and force relayout?

[Yael]

(In reply to comment #31)
> (From update of attachment 95675 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95675&action=review
> 
> > LayoutTests/fast/frames/flattening/iframe-flattening-out-of-view-and-scroll.html:12
> > +                scrollTo(1200, 0);
> 
> shouldnt you do something to invalidate the layout and force relayout?

Not sure what you mean?
As asked in comment #29, I scroll the frame into the view so one can see that the iframes have been flattened.

[Kenneth Rohde Christiansen]

(In reply to comment #32)
> (In reply to comment #31)
> > (From update of attachment 95675 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=95675&action=review
> > 
> > > LayoutTests/fast/frames/flattening/iframe-flattening-out-of-view-and-scroll.html:12
> > > +                scrollTo(1200, 0);
> > 
> > shouldnt you do something to invalidate the layout and force relayout?
> 
> Not sure what you mean?
> As asked in comment #29, I scroll the frame into the view so one can see that the iframes have been flattened.

But that doesnt mean that it is going to get re-laid out. There is a trick to touch some css property and it will invalidate and relayout... I think you will find that in some of the frame flattening tests

[Yael]

(In reply to comment #33)
> (In reply to comment #32)
> > (In reply to comment #31)
> > > (From update of attachment 95675 [details] [details] [details])
> > > View in context: https://bugs.webkit.org/attachment.cgi?id=95675&action=review
> > > 
> > > > LayoutTests/fast/frames/flattening/iframe-flattening-out-of-view-and-scroll.html:12
> > > > +                scrollTo(1200, 0);
> > > 
> > > shouldnt you do something to invalidate the layout and force relayout?
> > 
> > Not sure what you mean?
> > As asked in comment #29, I scroll the frame into the view so one can see that the iframes have been flattened.
> 
> But that doesnt mean that it is going to get re-laid out. There is a trick to touch some css property and it will invalidate and relayout... I think you will find that in some of the frame flattening tests

The idea is that it was flattened while off screen, and not re-laid out after scrolling into view.

[Kenneth Rohde Christiansen]

(In reply to comment #34)
> (In reply to comment #33)
> > (In reply to comment #32)
> > > (In reply to comment #31)
> > > > (From update of attachment 95675 [details] [details] [details] [details])
> > > > View in context: https://bugs.webkit.org/attachment.cgi?id=95675&action=review
> > > > 
> > > > > LayoutTests/fast/frames/flattening/iframe-flattening-out-of-view-and-scroll.html:12
> > > > > +                scrollTo(1200, 0);
> > > > 
> > > > shouldnt you do something to invalidate the layout and force relayout?
> > > 
> > > Not sure what you mean?
> > > As asked in comment #29, I scroll the frame into the view so one can see that the iframes have been flattened.
> > 
> > But that doesnt mean that it is going to get re-laid out. There is a trick to touch some css property and it will invalidate and relayout... I think you will find that in some of the frame flattening tests
> 
> The idea is that it was flattened while off screen, and not re-laid out after scrolling into view.

But if a relayout happens now that it is visible, you need to make sure it is still flattened right. So maybe that is yet another test.

You can force a relayout by doing the following:

var ignoreMe = document.body.offsetWidth;

Just accessing that value should force a relayout (grep in other tests for more examples)

[Yael]

(In reply to comment #35)
> (In reply to comment #34)
> > (In reply to comment #33)
> > > (In reply to comment #32)
> > > > (In reply to comment #31)
> > > > > (From update of attachment 95675 [details] [details] [details] [details] [details])
> > > > > View in context: https://bugs.webkit.org/attachment.cgi?id=95675&action=review
> > > > > 
> > > > > > LayoutTests/fast/frames/flattening/iframe-flattening-out-of-view-and-scroll.html:12
> > > > > > +                scrollTo(1200, 0);
> > > > > 
> > > > > shouldnt you do something to invalidate the layout and force relayout?
> > > > 
> > > > Not sure what you mean?
> > > > As asked in comment #29, I scroll the frame into the view so one can see that the iframes have been flattened.
> > > 
> > > But that doesnt mean that it is going to get re-laid out. There is a trick to touch some css property and it will invalidate and relayout... I think you will find that in some of the frame flattening tests
> > 
> > The idea is that it was flattened while off screen, and not re-laid out after scrolling into view.
> 
> But if a relayout happens now that it is visible, you need to make sure it is still flattened right. So maybe that is yet another test.
> 
> You can force a relayout by doing the following:
> 
> var ignoreMe = document.body.offsetWidth;
> 
> Just accessing that value should force a relayout (grep in other tests for more examples)
I can add another test to do that :)

[Yael]

Created attachment 95757 [details]
Patch.

Additional test that was requested by comment #35.

[Kenneth Rohde Christiansen]

Comment on attachment 95757 [details]
Patch.

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

> Source/WebCore/ChangeLog:8
> +        Do not flatten offscreen iframes during frame flattening, if flattening will make them visible.

as flattening might make them visible

> Source/WebCore/rendering/RenderIFrame.cpp:104
> +    // Do not flatten offscreen inner frames during frame flattening, if flattening will make them visible.

same here

[Yael]

Created attachment 95906 [details]
Patch

Updated comments per Kenneth's request.

[WebKit Commit Bot]

Comment on attachment 95906 [details]
Patch

Clearing flags on attachment: 95906

Committed r88011: <http://trac.webkit.org/changeset/88011>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Oleg Romashin (:romaxa)]

-    // Do not flatten offscreen inner frames during frame flattening.
-    return absoluteBoundingBoxRect().intersects(IntRect(IntPoint(0, 0), view->contentsSize()));
+    // Do not flatten offscreen inner frames during frame flattening, as flattening might make them visible.
+    IntRect boundingRect = absoluteBoundingBoxRect();
+    return boundingRect.maxX() > 0 && boundingRect.maxY() > 0;

this change seems making flattenFrame() return true for 0 size frames with x && y position != 0... is it expected?

[Oleg Romashin (:romaxa)]

Simple testcase:
<iframe frameborder="0" style="width: 0px; height: 0px;" src="data:text/html"</iframe>

[Simon Fraser (smfr)]

Please file a new bug if you think there's an outstanding issue.

[Yael]

http://trac.webkit.org/changeset/87853 Fixed the issue of 0 size iframes.
Can you try that?

[Oleg Romashin (:romaxa)]

(In reply to comment #45)
> http://trac.webkit.org/changeset/87853 Fixed the issue of 0 size iframes.
> Can you try that?

yep, that works fine, thanks... probably it make sense to set proper dependency between these two bugs...
and make this bug depends from 61831

[Yael]

(In reply to comment #46)
> (In reply to comment #45)
> > http://trac.webkit.org/changeset/87853 Fixed the issue of 0 size iframes.
> > Can you try that?
> 
> yep, that works fine, thanks... probably it make sense to set proper dependency between these two bugs...
> and make this bug depends from 61831
It was a mere coincidence that these 2 bugs were fixed at the same time. I did not know about the other issue until later. (No, I did not introduce the problem :)
Anyways, I am glad it works now.

[Ademar Reis]

Revision r88011 cherry-picked into qtwebkit-2.2 with commit 3b8d474 <http://gitorious.org/webkit/qtwebkit/commit/3b8d474>