Crash when subtree layout is set on FrameView while auto size mode is enabled.
authorzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 8 Nov 2015 00:50:44 +0000 (00:50 +0000)
committerzalan@apple.com <zalan@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Sun, 8 Nov 2015 00:50:44 +0000 (00:50 +0000)
https://bugs.webkit.org/show_bug.cgi?id=150995
rdar://problem/22785262

Reviewed by Beth Dakin.

Autosizing initiates multiple synchronous layouts to calculate preferred view width for current content.
FrameView::autoSizeIfEnabled() is called from FrameView::layout() while we are in InPreLayout state.
It is safe to do during full layout.
However, since we setup the subtree state just before the autoSizeIfEnabled() call, reentering it with
a newly issued layout confuses SubtreeLayoutStateMaintainer.

This patch reverses the order of autoSizeIfEnabled() call and the subtree layout state setup.
It also ensures that the first layout requested by autoSizeIfEnabled() always runs on the whole tree.

Source/WebCore:

Test: fast/dynamic/crash-subtree-layout-when-auto-size-enabled.html

* page/FrameView.cpp:
(WebCore::FrameView::layout):
(WebCore::FrameView::convertSubtreeLayoutToFullLayout):
(WebCore::FrameView::scheduleRelayout):
(WebCore::FrameView::scheduleRelayoutOfSubtree):
(WebCore::FrameView::autoSizeIfEnabled):
* page/FrameView.h:
* testing/Internals.cpp:
(WebCore::Internals::enableAutoSizeMode):
* testing/Internals.h:
* testing/Internals.idl:

LayoutTests:

* fast/dynamic/crash-subtree-layout-when-auto-size-enabled-expected.txt: Added.
* fast/dynamic/crash-subtree-layout-when-auto-size-enabled.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/dynamic/crash-subtree-layout-when-auto-size-enabled-expected.txt [new file with mode: 0644]
LayoutTests/fast/dynamic/crash-subtree-layout-when-auto-size-enabled.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/page/FrameView.cpp
Source/WebCore/page/FrameView.h
Source/WebCore/testing/Internals.cpp
Source/WebCore/testing/Internals.h
Source/WebCore/testing/Internals.idl

index 782e618..1d77c82 100644 (file)
@@ -1,3 +1,23 @@
+2015-11-07  Zalan Bujtas  <zalan@apple.com>
+
+        Crash when subtree layout is set on FrameView while auto size mode is enabled.
+        https://bugs.webkit.org/show_bug.cgi?id=150995
+        rdar://problem/22785262
+
+        Reviewed by Beth Dakin.
+
+        Autosizing initiates multiple synchronous layouts to calculate preferred view width for current content.
+        FrameView::autoSizeIfEnabled() is called from FrameView::layout() while we are in InPreLayout state.
+        It is safe to do during full layout.
+        However, since we setup the subtree state just before the autoSizeIfEnabled() call, reentering it with
+        a newly issued layout confuses SubtreeLayoutStateMaintainer.
+
+        This patch reverses the order of autoSizeIfEnabled() call and the subtree layout state setup.
+        It also ensures that the first layout requested by autoSizeIfEnabled() always runs on the whole tree.  
+
+        * fast/dynamic/crash-subtree-layout-when-auto-size-enabled-expected.txt: Added.
+        * fast/dynamic/crash-subtree-layout-when-auto-size-enabled.html: Added.
+
 2015-11-07  Chris Dumez  <cdumez@apple.com>
 
         embed element without src and type attributes should represent nothing
diff --git a/LayoutTests/fast/dynamic/crash-subtree-layout-when-auto-size-enabled-expected.txt b/LayoutTests/fast/dynamic/crash-subtree-layout-when-auto-size-enabled-expected.txt
new file mode 100644 (file)
index 0000000..166765e
--- /dev/null
@@ -0,0 +1 @@
+Pass if no crash or assert.
diff --git a/LayoutTests/fast/dynamic/crash-subtree-layout-when-auto-size-enabled.html b/LayoutTests/fast/dynamic/crash-subtree-layout-when-auto-size-enabled.html
new file mode 100644 (file)
index 0000000..f928eaf
--- /dev/null
@@ -0,0 +1,32 @@
+<!DOCTYPE HTML>
+<html>
+<head>
+<title>This tests that we don't crash in autosize mode with subtree layout.</title>
+<style>
+    div {
+        position: absolute;
+        overflow: hidden;
+        width: 10px;
+        height: 10px;
+    }
+</style>
+</head>
+<body>
+<div>Pass if no crash or assert.<div id=resizeThis></div></div>
+<script>
+if (window.internals)
+    internals.enableAutoSizeMode(true, 10, 10, 1000, 1000);
+
+if (window.testRunner) {
+    testRunner.waitUntilDone();
+    testRunner.dumpAsText();
+}
+
+setTimeout(function() {
+    document.getElementById("resizeThis").style.width = "20px";
+    if (window.testRunner)
+        testRunner.notifyDone();
+    }, 0);
+</script>
+</body>
+</html>
index 3711332..1708482 100644 (file)
@@ -1,3 +1,34 @@
+2015-11-07  Zalan Bujtas  <zalan@apple.com>
+
+        Crash when subtree layout is set on FrameView while auto size mode is enabled.
+        https://bugs.webkit.org/show_bug.cgi?id=150995
+        rdar://problem/22785262
+
+        Reviewed by Beth Dakin.
+
+        Autosizing initiates multiple synchronous layouts to calculate preferred view width for current content.
+        FrameView::autoSizeIfEnabled() is called from FrameView::layout() while we are in InPreLayout state.
+        It is safe to do during full layout.
+        However, since we setup the subtree state just before the autoSizeIfEnabled() call, reentering it with
+        a newly issued layout confuses SubtreeLayoutStateMaintainer.
+
+        This patch reverses the order of autoSizeIfEnabled() call and the subtree layout state setup.
+        It also ensures that the first layout requested by autoSizeIfEnabled() always runs on the whole tree.  
+
+        Test: fast/dynamic/crash-subtree-layout-when-auto-size-enabled.html
+
+        * page/FrameView.cpp:
+        (WebCore::FrameView::layout):
+        (WebCore::FrameView::convertSubtreeLayoutToFullLayout):
+        (WebCore::FrameView::scheduleRelayout):
+        (WebCore::FrameView::scheduleRelayoutOfSubtree):
+        (WebCore::FrameView::autoSizeIfEnabled):
+        * page/FrameView.h:
+        * testing/Internals.cpp:
+        (WebCore::Internals::enableAutoSizeMode):
+        * testing/Internals.h:
+        * testing/Internals.idl:
+
 2015-11-07  Chris Dumez  <cdumez@apple.com>
 
         embed element without src and type attributes should represent nothing
index 122964a..8fa0fe4 100644 (file)
@@ -1259,10 +1259,8 @@ void FrameView::layout(bool allowSubtree)
     InspectorInstrumentationCookie cookie = InspectorInstrumentation::willLayout(frame());
     AnimationUpdateBlock animationUpdateBlock(&frame().animation());
     
-    if (!allowSubtree && m_layoutRoot) {
-        m_layoutRoot->markContainingBlocksForLayout(ScheduleRelayout::No);
-        m_layoutRoot = nullptr;
-    }
+    if (!allowSubtree && m_layoutRoot)
+        convertSubtreeLayoutToFullLayout();
 
     ASSERT(frame().view() == this);
     ASSERT(frame().document());
@@ -1270,9 +1268,6 @@ void FrameView::layout(bool allowSubtree)
     Document& document = *frame().document();
     ASSERT(!document.inPageCache());
 
-    bool subtree;
-    RenderElement* root;
-
     {
         TemporaryChange<bool> changeSchedulingEnabled(m_layoutSchedulingEnabled, false);
 
@@ -1302,33 +1297,34 @@ void FrameView::layout(bool allowSubtree)
         // Always ensure our style info is up-to-date. This can happen in situations where
         // the layout beats any sort of style recalc update that needs to occur.
         document.updateStyleIfNeeded();
-        m_layoutPhase = InPreLayout;
-
-        subtree = m_layoutRoot;
-
-        // If there is only one ref to this view left, then its going to be destroyed as soon as we exit, 
+        // If there is only one ref to this view left, then its going to be destroyed as soon as we exit,
         // so there's no point to continuing to layout
         if (hasOneRef())
             return;
 
-        root = subtree ? m_layoutRoot : document.renderView();
-        if (!root) {
-            // FIXME: Do we need to set m_size here?
-            return;
-        }
-
         // Close block here so we can set up the font cache purge preventer, which we will still
         // want in scope even after we want m_layoutSchedulingEnabled to be restored again.
         // The next block sets m_layoutSchedulingEnabled back to false once again.
     }
 
-    RenderLayer* layer;
+    m_layoutPhase = InPreLayout;
+
+    RenderLayer* layer = nullptr;
+    bool subtree = false;
+    RenderElement* root = nullptr;
 
     ++m_nestedLayoutCount;
 
     {
         TemporaryChange<bool> changeSchedulingEnabled(m_layoutSchedulingEnabled, false);
 
+        autoSizeIfEnabled();
+
+        root = m_layoutRoot ? m_layoutRoot : document.renderView();
+        if (!root)
+            return;
+        subtree = m_layoutRoot;
+
         if (!m_layoutRoot) {
             auto* body = document.bodyOrFrameset();
             if (body && body->renderer()) {
@@ -1343,11 +1339,9 @@ void FrameView::layout(bool allowSubtree)
 #ifdef INSTRUMENT_LAYOUT_SCHEDULING
             if (m_firstLayout && !frame().ownerElement())
                 printf("Elapsed time before first layout: %lld\n", document.elapsedTime().count());
-#endif        
+#endif
         }
 
-        autoSizeIfEnabled();
-
         m_needsFullRepaint = !subtree && (m_firstLayout || downcast<RenderView>(*root).printing());
 
         if (!subtree) {
@@ -2567,6 +2561,12 @@ void FrameView::show()
         adjustTiledBackingCoverage();
     }
 }
+void FrameView::convertSubtreeLayoutToFullLayout()
+{
+    ASSERT(m_layoutRoot);
+    m_layoutRoot->markContainingBlocksForLayout(ScheduleRelayout::No);
+    m_layoutRoot = nullptr;
+}
 
 void FrameView::layoutTimerFired()
 {
@@ -2583,10 +2583,8 @@ void FrameView::scheduleRelayout()
     // too many false assertions.  See <rdar://problem/7218118>.
     ASSERT(frame().view() == this);
 
-    if (m_layoutRoot) {
-        m_layoutRoot->markContainingBlocksForLayout(ScheduleRelayout::No);
-        m_layoutRoot = nullptr;
-    }
+    if (m_layoutRoot)
+        convertSubtreeLayoutToFullLayout();
     if (!m_layoutSchedulingEnabled)
         return;
     if (!needsLayout())
@@ -2675,8 +2673,7 @@ void FrameView::scheduleRelayoutOfSubtree(RenderElement& newRelayoutRoot)
     }
 
     // Just do a full relayout.
-    m_layoutRoot->markContainingBlocksForLayout(ScheduleRelayout::No);
-    m_layoutRoot = nullptr;
+    convertSubtreeLayoutToFullLayout();
     newRelayoutRoot.markContainingBlocksForLayout(ScheduleRelayout::No);
     InspectorInstrumentation::didInvalidateLayout(frame());
 }
@@ -3219,6 +3216,8 @@ void FrameView::autoSizeIfEnabled()
     if (!documentView || !documentElement)
         return;
 
+    if (m_layoutRoot)
+        convertSubtreeLayoutToFullLayout();
     // Start from the minimum size and allow it to grow.
     resize(m_minAutoSize.width(), m_minAutoSize.height());
 
index 614ac1b..cc884ce 100644 (file)
@@ -677,6 +677,8 @@ private:
     void removeFromAXObjectCache();
     void notifyWidgets(WidgetNotification);
 
+    void convertSubtreeLayoutToFullLayout();
+
     RenderElement* viewportRenderer() const;
 
     HashSet<Widget*> m_widgetsInRenderTree;
index 77ace2d..4db6f9d 100644 (file)
@@ -2440,6 +2440,14 @@ void Internals::forceReload(bool endToEnd)
     frame()->loader().reload(endToEnd);
 }
 
+void Internals::enableAutoSizeMode(bool enabled, int minimumWidth, int minimumHeight, int maximumWidth, int maximumHeight)
+{
+    Document* document = contextDocument();
+    if (!document || !document->view())
+        return;
+    document->view()->enableAutoSizeMode(enabled, IntSize(minimumWidth, minimumHeight), IntSize(maximumWidth, maximumHeight));
+}
+
 #if ENABLE(ENCRYPTED_MEDIA_V2)
 void Internals::initializeMockCDM()
 {
index 97a2b9f..ea8bdc3 100644 (file)
@@ -336,6 +336,8 @@ public:
 
     void forceReload(bool endToEnd);
 
+    void enableAutoSizeMode(bool enabled, int minimumWidth, int minimumHeight, int maximumWidth, int maximumHeight);
+
 #if ENABLE(ENCRYPTED_MEDIA_V2)
     void initializeMockCDM();
 #endif
index af9c1f4..356e986 100644 (file)
@@ -329,6 +329,8 @@ enum MediaControlEvent {
 
     void forceReload(boolean endToEnd);
 
+    void enableAutoSizeMode(boolean enabled, long minimumWidth, long minimumHeight, long maximumWidth, long maximumHeight);
+
     [Conditional=VIDEO] void simulateAudioInterruption(Node node);
     [Conditional=VIDEO, RaisesException] boolean mediaElementHasCharacteristic(Node node, DOMString characteristic);