Bug 61491

Summary: Frame flattening is broken with nested frames
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: FramesAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: ademar, commit-queue, jesus, kenneth, koivisto, mtutunik, romaxa, simon.fraser, tonikitoo, yael, zalan
Priority: P1    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=184665
Attachments:
Description Flags
Testcase
none
Patch.
none
Patch.
none
Patch.
none
Patch.
none
Patch.
none
Patch.
kenneth: review+
Patch none

Description Simon Fraser (smfr) 2011-05-25 17:36:40 PDT
Frame flattening is totally broken if there are nested frames.  The inner frame ends up with size 0x0. There are no tests for this.
Comment 1 Simon Fraser (smfr) 2011-05-25 17:37:18 PDT
Created attachment 94888 [details]
Testcase
Comment 2 Simon Fraser (smfr) 2011-05-25 17:56:22 PDT
Seems to be an issue when the intermediate iframe has "scrolling=no"
Comment 3 Simon Fraser (smfr) 2011-05-25 18:16:10 PDT
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.
Comment 4 Kenneth Rohde Christiansen 2011-05-26 07:20:05 PDT
(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?
Comment 5 Simon Fraser (smfr) 2011-05-26 08:08:38 PDT
I'm not sure that's the source of the bug, but doing flattening based on a bogus visibility check certainly seems wrong.
Comment 6 Kenneth Rohde Christiansen 2011-05-26 08:09:57 PDT
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;
Comment 7 Kenneth Rohde Christiansen 2011-05-26 08:10:34 PDT
(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 :-)
Comment 8 Simon Fraser (smfr) 2011-05-26 08:13:52 PDT
Right, I think the isScrollable check is the real source of the bug.
Comment 9 Kenneth Rohde Christiansen 2011-05-26 08:31:55 PDT
(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.
Comment 10 Kenneth Rohde Christiansen 2011-05-26 08:49:26 PDT
@@ -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.
Comment 11 Simon Fraser (smfr) 2011-05-26 09:10:33 PDT
Don't we call updateWidgetPositions() after layout anyway? I'm not sure why frame flattening has to call it explicitly.
Comment 12 Simon Fraser (smfr) 2011-05-26 10:42:06 PDT
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?
Comment 13 Simon Fraser (smfr) 2011-05-26 10:46:59 PDT
performPostLayoutTasks() is being skipped because inSubframeLayoutWithFrameFlattening is true. So we never update the widget inside the flattened iframe.
Comment 14 Simon Fraser (smfr) 2011-05-26 10:54:36 PDT
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.
Comment 15 Yael 2011-05-26 18:14:10 PDT
(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.
Comment 16 Yael 2011-05-29 15:17:55 PDT
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.
Comment 17 Kenneth Rohde Christiansen 2011-05-30 01:11:21 PDT
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
> +2011-05-29  Yael Aharon  <yael.aharon@nokia.com>
> +
> +        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?
Comment 18 Yael 2011-05-31 06:01:02 PDT
(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.
Comment 19 Yael 2011-05-31 06:09:15 PDT
Created attachment 95425 [details]
Patch.

Put Simon Fraser's name in the LayoutTests Changelog.
Comment 20 Kenneth Rohde Christiansen 2011-05-31 06:12:34 PDT
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?
Comment 21 Yael 2011-05-31 06:49:31 PDT
(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().
Comment 22 Antonio Gomes 2011-05-31 07:02:53 PDT
> > > 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
Comment 23 WebKit Commit Bot 2011-05-31 07:20:19 PDT
Comment on attachment 95425 [details]
Patch.

Clearing flags on attachment: 95425

Committed r87726: <http://trac.webkit.org/changeset/87726>
Comment 24 WebKit Commit Bot 2011-05-31 07:20:26 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 Yael 2011-05-31 08:39:18 PDT
We should also fix the issue described in comment #5.
Comment 26 Ademar Reis 2011-05-31 12:18:50 PDT
Revision r87726 cherry-picked into qtwebkit-2.2 with commit de5d05b <http://gitorious.org/webkit/qtwebkit/commit/de5d05b>
Comment 27 Yael 2011-05-31 18:49:18 PDT
Created attachment 95530 [details]
Patch.

Do not flatten offscreen iframes during frame flattening, if flattening will make them visible.
Comment 28 Yael 2011-05-31 18:56:39 PDT
Created attachment 95532 [details]
Patch.

One more try. Patches with images are hard to manage :)
Comment 29 Simon Fraser (smfr) 2011-05-31 20:04:54 PDT
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,
Comment 30 Yael 2011-06-01 15:37:18 PDT
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.
Comment 31 Kenneth Rohde Christiansen 2011-06-01 15:50:32 PDT
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?
Comment 32 Yael 2011-06-01 16:37:03 PDT
(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.
Comment 33 Kenneth Rohde Christiansen 2011-06-01 17:08:30 PDT
(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
Comment 34 Yael 2011-06-01 17:20:35 PDT
(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.
Comment 35 Kenneth Rohde Christiansen 2011-06-01 17:22:59 PDT
(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)
Comment 36 Yael 2011-06-01 17:36:00 PDT
(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 :)
Comment 37 Yael 2011-06-02 06:27:56 PDT
Created attachment 95757 [details]
Patch.

Additional test that was requested by comment #35.
Comment 38 Kenneth Rohde Christiansen 2011-06-03 01:10:22 PDT
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
Comment 39 Yael 2011-06-03 06:17:31 PDT
Created attachment 95906 [details]
Patch

Updated comments per Kenneth's request.
Comment 40 WebKit Commit Bot 2011-06-03 06:47:18 PDT
Comment on attachment 95906 [details]
Patch

Clearing flags on attachment: 95906

Committed r88011: <http://trac.webkit.org/changeset/88011>
Comment 41 WebKit Commit Bot 2011-06-03 06:47:27 PDT
All reviewed patches have been landed.  Closing bug.
Comment 42 Oleg Romashin (:romaxa) 2011-07-12 14:25:38 PDT
-    // 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?
Comment 43 Oleg Romashin (:romaxa) 2011-07-12 14:32:42 PDT
Simple testcase:
<iframe frameborder="0" style="width: 0px; height: 0px;" src="data:text/html"</iframe>
Comment 44 Simon Fraser (smfr) 2011-07-12 14:38:43 PDT
Please file a new bug if you think there's an outstanding issue.
Comment 45 Yael 2011-07-12 14:51:48 PDT
http://trac.webkit.org/changeset/87853 Fixed the issue of 0 size iframes.
Can you try that?
Comment 46 Oleg Romashin (:romaxa) 2011-07-12 15:41:35 PDT
(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
Comment 47 Yael 2011-07-12 18:52:27 PDT
(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.
Comment 48 Ademar Reis 2011-07-13 06:09:27 PDT
Revision r88011 cherry-picked into qtwebkit-2.2 with commit 3b8d474 <http://gitorious.org/webkit/qtwebkit/commit/3b8d474>