RESOLVED FIXED 61491
Frame flattening is broken with nested frames
https://bugs.webkit.org/show_bug.cgi?id=61491
Summary Frame flattening is broken with nested frames
Simon Fraser (smfr)
Reported 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.
Attachments
Testcase (3.00 KB, application/octet-stream)
2011-05-25 17:37 PDT, Simon Fraser (smfr)
no flags
Patch. (29.56 KB, patch)
2011-05-29 15:17 PDT, Yael
no flags
Patch. (29.56 KB, patch)
2011-05-31 06:09 PDT, Yael
no flags
Patch. (52.77 KB, patch)
2011-05-31 18:49 PDT, Yael
no flags
Patch. (52.77 KB, patch)
2011-05-31 18:56 PDT, Yael
no flags
Patch. (81.84 KB, patch)
2011-06-01 15:37 PDT, Yael
no flags
Patch. (111.07 KB, patch)
2011-06-02 06:27 PDT, Yael
kenneth: review+
Patch (111.09 KB, patch)
2011-06-03 06:17 PDT, Yael
no flags
Simon Fraser (smfr)
Comment 1 2011-05-25 17:37:18 PDT
Created attachment 94888 [details] Testcase
Simon Fraser (smfr)
Comment 2 2011-05-25 17:56:22 PDT
Seems to be an issue when the intermediate iframe has "scrolling=no"
Simon Fraser (smfr)
Comment 3 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.
Kenneth Rohde Christiansen
Comment 4 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?
Simon Fraser (smfr)
Comment 5 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.
Kenneth Rohde Christiansen
Comment 6 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;
Kenneth Rohde Christiansen
Comment 7 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 :-)
Simon Fraser (smfr)
Comment 8 2011-05-26 08:13:52 PDT
Right, I think the isScrollable check is the real source of the bug.
Kenneth Rohde Christiansen
Comment 9 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.
Kenneth Rohde Christiansen
Comment 10 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.
Simon Fraser (smfr)
Comment 11 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.
Simon Fraser (smfr)
Comment 12 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?
Simon Fraser (smfr)
Comment 13 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.
Simon Fraser (smfr)
Comment 14 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.
Yael
Comment 15 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.
Yael
Comment 16 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.
Kenneth Rohde Christiansen
Comment 17 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?
Yael
Comment 18 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.
Yael
Comment 19 2011-05-31 06:09:15 PDT
Created attachment 95425 [details] Patch. Put Simon Fraser's name in the LayoutTests Changelog.
Kenneth Rohde Christiansen
Comment 20 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?
Yael
Comment 21 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().
Antonio Gomes
Comment 22 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
WebKit Commit Bot
Comment 23 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>
WebKit Commit Bot
Comment 24 2011-05-31 07:20:26 PDT
All reviewed patches have been landed. Closing bug.
Yael
Comment 25 2011-05-31 08:39:18 PDT
We should also fix the issue described in comment #5.
Ademar Reis
Comment 26 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>
Yael
Comment 27 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.
Yael
Comment 28 2011-05-31 18:56:39 PDT
Created attachment 95532 [details] Patch. One more try. Patches with images are hard to manage :)
Simon Fraser (smfr)
Comment 29 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,
Yael
Comment 30 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.
Kenneth Rohde Christiansen
Comment 31 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?
Yael
Comment 32 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.
Kenneth Rohde Christiansen
Comment 33 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
Yael
Comment 34 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.
Kenneth Rohde Christiansen
Comment 35 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)
Yael
Comment 36 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 :)
Yael
Comment 37 2011-06-02 06:27:56 PDT
Created attachment 95757 [details] Patch. Additional test that was requested by comment #35.
Kenneth Rohde Christiansen
Comment 38 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
Yael
Comment 39 2011-06-03 06:17:31 PDT
Created attachment 95906 [details] Patch Updated comments per Kenneth's request.
WebKit Commit Bot
Comment 40 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>
WebKit Commit Bot
Comment 41 2011-06-03 06:47:27 PDT
All reviewed patches have been landed. Closing bug.
Oleg Romashin (:romaxa)
Comment 42 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?
Oleg Romashin (:romaxa)
Comment 43 2011-07-12 14:32:42 PDT
Simple testcase: <iframe frameborder="0" style="width: 0px; height: 0px;" src="data:text/html"</iframe>
Simon Fraser (smfr)
Comment 44 2011-07-12 14:38:43 PDT
Please file a new bug if you think there's an outstanding issue.
Yael
Comment 45 2011-07-12 14:51:48 PDT
http://trac.webkit.org/changeset/87853 Fixed the issue of 0 size iframes. Can you try that?
Oleg Romashin (:romaxa)
Comment 46 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
Yael
Comment 47 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.
Ademar Reis
Comment 48 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>
Note You need to log in before you can comment on or make changes to this bug.