Frame flattening ASSERT(!needsLayout()) in FrameView::paintContents()
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 14 Mar 2012 20:22:55 +0000 (20:22 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 14 Mar 2012 20:22:55 +0000 (20:22 +0000)
https://bugs.webkit.org/show_bug.cgi?id=80155

Patch by Zalan Bujtas <zbujtas@gmail.com> on 2012-03-14
Reviewed by Antti Koivisto.

Source/WebCore:

This patch ensures that an iframe only schedules and calls parent's layout,
when it is going to be flattened. Non-flattened iframe does not affect
parent's layout, so normal layout flow applies. isInSubframeLayoutWithFrameFlattening()
function has been added to test whether a particular child frame is changing
parent's layout. This function also ensures that scheduleRelayout() and layout()
are in sync of checking againts frame flattening.

Test: fast/frames/flattening/iframe-flattening-fixed-width-and-height-no-scrolling-with-js-forced-layout.html

* page/FrameView.cpp:
(WebCore::FrameView::avoidScrollbarCreation):
(WebCore::FrameView::layout):
(WebCore::FrameView::scheduleRelayout):
(WebCore::FrameView::isInChildFrameWithFrameFlattening):
(WebCore):
(WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive):
* page/FrameView.h:
(FrameView):
* rendering/RenderIFrame.h:
(RenderIFrame):
(WebCore::RenderIFrame::renderName):

LayoutTests:

* fast/frames/flattening/iframe-flattening-fixed-width-and-height-no-scrolling-with-js-forced-layout-expected.txt: Added.
* fast/frames/flattening/iframe-flattening-fixed-width-and-height-no-scrolling-with-js-forced-layout.html: Added.

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@110738 268f45cc-cd09-0410-ab3c-d52691b4dbfc

LayoutTests/ChangeLog
LayoutTests/fast/frames/flattening/iframe-flattening-fixed-width-and-height-no-scrolling-with-js-forced-layout-expected.txt [new file with mode: 0644]
LayoutTests/fast/frames/flattening/iframe-flattening-fixed-width-and-height-no-scrolling-with-js-forced-layout.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/FrameView.h
Source/WebCore/rendering/RenderIFrame.h

index 6d82383..1c2b37d 100644 (file)
@@ -1,3 +1,13 @@
+2012-03-14  Zalan Bujtas  <zbujtas@gmail.com>
+
+        Frame flattening ASSERT(!needsLayout()) in FrameView::paintContents()
+        https://bugs.webkit.org/show_bug.cgi?id=80155
+
+        Reviewed by Antti Koivisto.
+
+        * fast/frames/flattening/iframe-flattening-fixed-width-and-height-no-scrolling-with-js-forced-layout-expected.txt: Added.
+        * fast/frames/flattening/iframe-flattening-fixed-width-and-height-no-scrolling-with-js-forced-layout.html: Added.
+
 2012-03-14  Ojan Vafai  <ojan@chromium.org>
 
         Add Lion failures in order to green the lion bot.
diff --git a/LayoutTests/fast/frames/flattening/iframe-flattening-fixed-width-and-height-no-scrolling-with-js-forced-layout-expected.txt b/LayoutTests/fast/frames/flattening/iframe-flattening-fixed-width-and-height-no-scrolling-with-js-forced-layout-expected.txt
new file mode 100644 (file)
index 0000000..f4c439d
--- /dev/null
@@ -0,0 +1,3 @@
+This test verifies that nested iframes do not cause assertion failure, when javascript is forcing layout. PASS if no assert in debug.
+
+
diff --git a/LayoutTests/fast/frames/flattening/iframe-flattening-fixed-width-and-height-no-scrolling-with-js-forced-layout.html b/LayoutTests/fast/frames/flattening/iframe-flattening-fixed-width-and-height-no-scrolling-with-js-forced-layout.html
new file mode 100644 (file)
index 0000000..142febb
--- /dev/null
@@ -0,0 +1,33 @@
+<html>
+<head>
+    <script type="text/javascript">
+        if (window.layoutTestController) {
+            layoutTestController.waitUntilDone();
+            layoutTestController.dumpAsText();
+            layoutTestController.setFrameFlatteningEnabled(true);
+        }
+
+        function test()
+        {
+            setTimeout(function() {
+                if (window.layoutTestController)
+                    layoutTestController.notifyDone();
+            }, 0);
+        }
+    </script>
+</head>
+<body onload="test()">
+    <div>
+        <p>This test verifies that nested iframes do not cause assertion failure, when javascript is forcing layout. PASS if no assert in debug.</p>
+    </div>
+    <iframe src="data:text/html,
+            <style>body { background-color: green; }</style>
+            <body>
+            <iframe src='data:text/html,
+                <style>body { background-color: yellow; }</style>
+                <body></body>' scrolling='no' height='50px' width='50px'></iframe>
+            <script>document.body.offsetHeight</script>
+            </body>" scrolling="no" height="100px" width="100px"></iframe>
+     <script>document.body.offsetHeight</script>
+</body>
+</html>
index 1f87c57..789e16c 100644 (file)
@@ -1,3 +1,32 @@
+2012-03-14  Zalan Bujtas  <zbujtas@gmail.com>
+
+        Frame flattening ASSERT(!needsLayout()) in FrameView::paintContents()
+        https://bugs.webkit.org/show_bug.cgi?id=80155
+
+        Reviewed by Antti Koivisto.
+
+        This patch ensures that an iframe only schedules and calls parent's layout,
+        when it is going to be flattened. Non-flattened iframe does not affect
+        parent's layout, so normal layout flow applies. isInSubframeLayoutWithFrameFlattening()
+        function has been added to test whether a particular child frame is changing
+        parent's layout. This function also ensures that scheduleRelayout() and layout()
+        are in sync of checking againts frame flattening.
+
+        Test: fast/frames/flattening/iframe-flattening-fixed-width-and-height-no-scrolling-with-js-forced-layout.html
+
+        * page/FrameView.cpp:
+        (WebCore::FrameView::avoidScrollbarCreation):
+        (WebCore::FrameView::layout):
+        (WebCore::FrameView::scheduleRelayout):
+        (WebCore::FrameView::isInChildFrameWithFrameFlattening):
+        (WebCore):
+        (WebCore::FrameView::updateLayoutAndStyleIfNeededRecursive):
+        * page/FrameView.h:
+        (FrameView):
+        * rendering/RenderIFrame.h:
+        (RenderIFrame):
+        (WebCore::RenderIFrame::renderName):
+
 2012-03-14  Anders Carlsson  <andersca@apple.com>
 
         Don't cap the scroll position if layout happens when a FrameView's overhangAmount is non-zero
index 5ecbca3..2d392de 100644 (file)
@@ -56,6 +56,7 @@
 #include "RenderArena.h"
 #include "RenderEmbeddedObject.h"
 #include "RenderFullScreen.h"
+#include "RenderIFrame.h"
 #include "RenderLayer.h"
 #include "RenderPart.h"
 #include "RenderScrollbar.h"
@@ -436,7 +437,7 @@ bool FrameView::avoidScrollbarCreation() const
     ASSERT(m_frame);
 
     // with frame flattening no subframe can have scrollbars
-    // but we also cannot turn scrollbars of as we determine
+    // but we also cannot turn scrollbars off as we determine
     // our flattening policy using that.
 
     if (!m_frame->ownerElement())
@@ -891,17 +892,19 @@ void FrameView::layout(bool allowSubtree)
     if (m_inLayout)
         return;
 
-    bool inSubframeLayoutWithFrameFlattening = parent() && m_frame->settings() && m_frame->settings()->frameFlatteningEnabled();
+    bool inChildFrameLayoutWithFrameFlattening = isInChildFrameWithFrameFlattening();
 
-    if (inSubframeLayoutWithFrameFlattening) {
-        if (parent()->isFrameView()) {
-            FrameView* parentView =   static_cast<FrameView*>(parent());
-            if (!parentView->m_nestedLayoutCount) {
-                while (parentView->parent() && parentView->parent()->isFrameView())
-                    parentView = static_cast<FrameView*>(parentView->parent());
-                parentView->layout(allowSubtree);
-                return;
-            }
+    if (inChildFrameLayoutWithFrameFlattening) {
+        FrameView* parentView = parentFrameView();
+        if (parentView && !parentView->m_nestedLayoutCount) {
+            while (parentView->parentFrameView())
+                parentView = parentView->parentFrameView();
+
+            parentView->layout(allowSubtree);
+
+            RenderObject* root = m_layoutRoot ? m_layoutRoot : m_frame->document()->renderer();
+            ASSERT_UNUSED(root, !root->needsLayout());
+            return;
         }
     }
 
@@ -940,7 +943,7 @@ void FrameView::layout(bool allowSubtree)
     {
         TemporaryChange<bool> changeSchedulingEnabled(m_layoutSchedulingEnabled, false);
 
-        if (!m_nestedLayoutCount && !m_inSynchronousPostLayout && m_postLayoutTasksTimer.isActive() && !inSubframeLayoutWithFrameFlattening) {
+        if (!m_nestedLayoutCount && !m_inSynchronousPostLayout && m_postLayoutTasksTimer.isActive() && !inChildFrameLayoutWithFrameFlattening) {
             // This is a new top-level layout. If there are any remaining tasks from the previous
             // layout, finish them now.
             m_inSynchronousPostLayout = true;
@@ -1118,7 +1121,7 @@ void FrameView::layout(bool allowSubtree)
 
     if (!m_postLayoutTasksTimer.isActive()) {
         if (!m_inSynchronousPostLayout) {
-            if (inSubframeLayoutWithFrameFlattening) {
+            if (inChildFrameLayoutWithFrameFlattening) {
                 if (RenderView* root = rootRenderer(this))
                     root->updateWidgetPositions();
             } else {
@@ -1129,7 +1132,7 @@ void FrameView::layout(bool allowSubtree)
             }
         }
         
-        if (!m_postLayoutTasksTimer.isActive() && (needsLayout() || m_inSynchronousPostLayout || inSubframeLayoutWithFrameFlattening)) {
+        if (!m_postLayoutTasksTimer.isActive() && (needsLayout() || m_inSynchronousPostLayout || inChildFrameLayoutWithFrameFlattening)) {
             // If we need layout or are already in a synchronous call to postLayoutTasks(), 
             // defer widget updates and event dispatch until after we return. postLayoutTasks()
             // can make us need to update again, and we can get stuck in a nasty cycle unless
@@ -1962,12 +1965,10 @@ void FrameView::scheduleRelayout()
     if (!m_frame->document()->shouldScheduleLayout())
         return;
 
-    // When frame flattening is enabled, the contents of the frame affects layout of the parent frames.
+    // When frame flattening is enabled, the contents of the frame could affect the layout of the parent frames.
     // Also invalidate parent frame starting from the owner element of this frame.
-    if (m_frame->settings() && m_frame->settings()->frameFlatteningEnabled() && m_frame->ownerRenderer()) {
-        if (m_frame->ownerElement()->hasTagName(iframeTag) || m_frame->ownerElement()->hasTagName(frameTag))
-            m_frame->ownerRenderer()->setNeedsLayout(true, true);
-    }
+    if (isInChildFrameWithFrameFlattening() && m_frame->ownerRenderer())
+        m_frame->ownerRenderer()->setNeedsLayout(true, true);
 
     int delay = m_frame->document()->minimumLayoutDelay();
     if (m_layoutTimer.isActive() && m_delayedLayout && !delay)
@@ -2801,6 +2802,25 @@ FrameView* FrameView::parentFrameView() const
     return 0;
 }
 
+bool FrameView::isInChildFrameWithFrameFlattening()
+{
+    if (!parent() || !m_frame->ownerElement() || !m_frame->settings() || !m_frame->settings()->frameFlatteningEnabled())
+        return false;
+
+    // Frame flattening applies when the owner element is either in a frameset or
+    // an iframe with flattening parameters.
+    if (m_frame->ownerElement()->hasTagName(iframeTag)) {
+        RenderIFrame* iframeRenderer = toRenderIFrame(m_frame->ownerElement()->renderPart());
+
+        if (iframeRenderer->flattenFrame())
+            return true;
+
+    } else if (m_frame->ownerElement()->hasTagName(frameTag))
+        return true;
+
+    return false;
+}
+
 void FrameView::updateControlTints()
 {
     // This is called when control tints are changed from aqua/graphite to clear and vice versa.
@@ -3008,6 +3028,11 @@ void FrameView::updateLayoutAndStyleIfNeededRecursive()
     // updateLayoutAndStyleIfNeededRecursive is called when we need to make sure style and layout are up-to-date before
     // painting, so we need to flush out any deferred repaints too.
     flushDeferredRepaints();
+
+    // When frame flattening is on, child frame can mark parent frame dirty. In such case, child frame
+    // needs to call layout on parent frame recursively.
+    // This assert ensures that parent frames are clean, when child frames finished updating layout and style.
+    ASSERT(!needsLayout());
 }
     
 void FrameView::flushDeferredRepaints()
index 1f90bc3..f14a213 100644 (file)
@@ -406,6 +406,8 @@ private:
 
     FrameView* parentFrameView() const;
 
+    bool isInChildFrameWithFrameFlattening();
+
     virtual AXObjectCache* axObjectCache() const;
     void notifyWidgetsInAllFrames(WidgetNotification);
     
index b6b30c9..fe6631e 100644 (file)
@@ -34,6 +34,8 @@ class RenderIFrame : public RenderFrameBase {
 public:
     explicit RenderIFrame(Element*);
 
+    bool flattenFrame();
+
 private:
     virtual void computeLogicalHeight();
     virtual void computeLogicalWidth();
@@ -44,8 +46,6 @@ private:
 
     virtual const char* renderName() const { return "RenderPartObject"; } // Lying for now to avoid breaking tests
 
-    bool flattenFrame();
-
 };
 
 inline RenderIFrame* toRenderIFrame(RenderObject* object)