[CSS Regions] Assertion in RenderFlowThread::removeRenderBoxRegionInfo
authorabucur@adobe.com <abucur@adobe.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Feb 2013 13:44:42 +0000 (13:44 +0000)
committerabucur@adobe.com <abucur@adobe.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 19 Feb 2013 13:44:42 +0000 (13:44 +0000)
https://bugs.webkit.org/show_bug.cgi?id=109914

Reviewed by David Hyatt.

Source/WebCore:

This patch moves a part of the invalidation operations inside the RenderFlowThread::invalidateRegions call. The maps
are cleared anyway at layout time but doing this earlier makes sure the flow thread is in a more consistent state
(the RenderFlowThread object has both the region chain invalidated and the regions information cleared).

RenderFlowThread::removeRenderBoxRegionInfo will check if the region chain is invalidated. If true, it means the
flow thread has a layout scheduled and the regions information is not yet reliable. In this case we just return from the
function and wait for the layout to cleanup the box information.

Test: fast/regions/remove-box-info-assert.html

* rendering/RenderFlowThread.cpp:
(WebCore::RenderFlowThread::removeRegionFromThread):
(WebCore::RenderFlowThread::invalidateRegions):
(WebCore):
(WebCore::RenderFlowThread::layout):
(WebCore::RenderFlowThread::removeRenderBoxRegionInfo):
* rendering/RenderFlowThread.h:
* rendering/RenderNamedFlowThread.cpp:
(WebCore::RenderNamedFlowThread::removeRegionFromThread):

LayoutTests:

The test creates a flow thread with two regions and a content node. We remove the region and the node from the flow thread
at the same time. When the style is updated, the region is first removed from the chain and range information for boxes
is cleared from the flow thread. When the node is removed from the flow thread it tries to delete its region box information.
The range information on the flow thread is gone so the range for the node's box is zero - nothing gets deleted. Afterwards,
an ASSERT is triggered because there's leftover box information inside the region chain.

* fast/regions/remove-box-info-assert-expected.txt: Added.
* fast/regions/remove-box-info-assert.html: Added.

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

LayoutTests/ChangeLog
LayoutTests/fast/regions/remove-box-info-assert-expected.txt [new file with mode: 0644]
LayoutTests/fast/regions/remove-box-info-assert.html [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/rendering/RenderFlowThread.cpp
Source/WebCore/rendering/RenderFlowThread.h
Source/WebCore/rendering/RenderNamedFlowThread.cpp

index a2cb5b6..dfba7a9 100644 (file)
@@ -1,3 +1,19 @@
+2013-02-19  Andrei Bucur  <abucur@adobe.com>
+
+        [CSS Regions] Assertion in RenderFlowThread::removeRenderBoxRegionInfo
+        https://bugs.webkit.org/show_bug.cgi?id=109914
+
+        Reviewed by David Hyatt.
+
+        The test creates a flow thread with two regions and a content node. We remove the region and the node from the flow thread
+        at the same time. When the style is updated, the region is first removed from the chain and range information for boxes
+        is cleared from the flow thread. When the node is removed from the flow thread it tries to delete its region box information.
+        The range information on the flow thread is gone so the range for the node's box is zero - nothing gets deleted. Afterwards,
+        an ASSERT is triggered because there's leftover box information inside the region chain.
+
+        * fast/regions/remove-box-info-assert-expected.txt: Added.
+        * fast/regions/remove-box-info-assert.html: Added.
+
 2013-02-19  Stephen White  <senorblanco@chromium.org>
 
         [chromium] Unreviewed gardening.
diff --git a/LayoutTests/fast/regions/remove-box-info-assert-expected.txt b/LayoutTests/fast/regions/remove-box-info-assert-expected.txt
new file mode 100644 (file)
index 0000000..7d7c885
--- /dev/null
@@ -0,0 +1,5 @@
+Bug 109914: [CSS Regions] Assertion in RenderFlowThread::removeRenderBoxRegionInfo
+
+This test PASSES if it does not CRASH or ASSERT.
+
+
diff --git a/LayoutTests/fast/regions/remove-box-info-assert.html b/LayoutTests/fast/regions/remove-box-info-assert.html
new file mode 100644 (file)
index 0000000..ec798d9
--- /dev/null
@@ -0,0 +1,35 @@
+<html>
+       <head>
+               <style>
+               .content {
+                       -webkit-flow-into: flow;
+               }
+
+               .region {
+                       -webkit-flow-from: flow;
+                       width: 300px;
+                       height: 300px;
+               }
+       </style>
+       </head>
+<body>
+       <p>Bug <a href="https://bugs.webkit.org/show_bug.cgi?id=109914">109914</a>: [CSS Regions] Assertion in RenderFlowThread::removeRenderBoxRegionInfo</p>
+       <p>This test PASSES if it does not CRASH or ASSERT.</p>
+       <!-- We need two regions to prevent optimizations from hiding the assertion-->
+       <div class="region"></div>
+       <div class="region" id="region"></div>
+       <div class="content"><div id="content">X</div></div>
+       <script type="text/javascript">
+       if (window.testRunner)
+        window.testRunner.dumpAsText();
+       document.body.offsetTop;
+       var region = document.getElementById("region");
+       var content = document.getElementById("content");
+       region.style.webkitFlowFrom = "change_flow";
+       content.style.webkitFlowInto = "change_flow";
+       document.body.offsetTop;
+       content.style.display = "none";
+       region.style.display = "none";
+       </script>
+</body>
+</html>
\ No newline at end of file
index 0bcfad0..407611b 100644 (file)
@@ -1,3 +1,30 @@
+2013-02-19  Andrei Bucur  <abucur@adobe.com>
+
+        [CSS Regions] Assertion in RenderFlowThread::removeRenderBoxRegionInfo
+        https://bugs.webkit.org/show_bug.cgi?id=109914
+
+        Reviewed by David Hyatt.
+
+        This patch moves a part of the invalidation operations inside the RenderFlowThread::invalidateRegions call. The maps
+        are cleared anyway at layout time but doing this earlier makes sure the flow thread is in a more consistent state
+        (the RenderFlowThread object has both the region chain invalidated and the regions information cleared).
+
+        RenderFlowThread::removeRenderBoxRegionInfo will check if the region chain is invalidated. If true, it means the
+        flow thread has a layout scheduled and the regions information is not yet reliable. In this case we just return from the
+        function and wait for the layout to cleanup the box information.
+
+        Test: fast/regions/remove-box-info-assert.html
+
+        * rendering/RenderFlowThread.cpp:
+        (WebCore::RenderFlowThread::removeRegionFromThread):
+        (WebCore::RenderFlowThread::invalidateRegions):
+        (WebCore):
+        (WebCore::RenderFlowThread::layout):
+        (WebCore::RenderFlowThread::removeRenderBoxRegionInfo):
+        * rendering/RenderFlowThread.h:
+        * rendering/RenderNamedFlowThread.cpp:
+        (WebCore::RenderNamedFlowThread::removeRegionFromThread):
+
 2013-02-19  Alberto Garcia  <agarcia@igalia.com>
 
         Fix build broekn by r142988.
index e9a1679..612590d 100644 (file)
@@ -105,12 +105,26 @@ void RenderFlowThread::addRegionToThread(RenderRegion* renderRegion)
 void RenderFlowThread::removeRegionFromThread(RenderRegion* renderRegion)
 {
     ASSERT(renderRegion);
-    m_regionRangeMap.clear();
     m_regionList.remove(renderRegion);
     invalidateRegions();
     checkRegionsWithStyling();
 }
 
+void RenderFlowThread::invalidateRegions()
+{
+    if (m_regionsInvalidated) {
+        ASSERT(selfNeedsLayout());
+        return;
+    }
+
+    m_regionRangeMap.clear();
+    m_breakBeforeToRegionMap.clear();
+    m_breakAfterToRegionMap.clear();
+    setNeedsLayout(true);
+
+    m_regionsInvalidated = true;
+}
+
 class CurrentRenderFlowThreadDisabler {
     WTF_MAKE_NONCOPYABLE(CurrentRenderFlowThreadDisabler);
 public:
@@ -141,9 +155,6 @@ void RenderFlowThread::layout()
         m_regionsInvalidated = false;
         m_regionsHaveUniformLogicalWidth = true;
         m_regionsHaveUniformLogicalHeight = true;
-        m_regionRangeMap.clear();
-        m_breakBeforeToRegionMap.clear();
-        m_breakAfterToRegionMap.clear();
 
         LayoutUnit previousRegionLogicalWidth = 0;
         LayoutUnit previousRegionLogicalHeight = 0;
@@ -152,7 +163,7 @@ void RenderFlowThread::layout()
             for (RenderRegionList::iterator iter = m_regionList.begin(); iter != m_regionList.end(); ++iter) {
                 RenderRegion* region = *iter;
                 ASSERT(!region->needsLayout());
-                
+
                 region->deleteAllRenderBoxRegionInfo();
 
                 // In the normal layout phase we need to initialize the overrideLogicalContentHeight for auto-height regions.
@@ -436,6 +447,12 @@ void RenderFlowThread::removeRenderBoxRegionInfo(RenderBox* box)
     if (!hasRegions())
         return;
 
+    // If the region chain was invalidated the next layout will clear the box information from all the regions.
+    if (m_regionsInvalidated) {
+        ASSERT(selfNeedsLayout());
+        return;
+    }
+
     RenderRegion* startRegion;
     RenderRegion* endRegion;
     getRegionRangeForBox(box, startRegion, endRegion);
index 2f5f76e..3fa87db 100644 (file)
@@ -84,7 +84,7 @@ public:
     bool hasRegionsWithStyling() const { return m_hasRegionsWithStyling; }
     void checkRegionsWithStyling();
 
-    void invalidateRegions() { m_regionsInvalidated = true; setNeedsLayout(true); }
+    void invalidateRegions();
     bool hasValidRegionInfo() const { return !m_regionsInvalidated && !m_regionList.isEmpty(); }
 
     static PassRefPtr<RenderStyle> createFlowThreadStyle(RenderStyle* parentStyle);
index e8d7d00..3cce9df 100644 (file)
@@ -250,7 +250,6 @@ void RenderNamedFlowThread::addRegionToThread(RenderRegion* renderRegion)
 void RenderNamedFlowThread::removeRegionFromThread(RenderRegion* renderRegion)
 {
     ASSERT(renderRegion);
-    m_regionRangeMap.clear();
 
     if (renderRegion->parentNamedFlowThread()) {
         if (!renderRegion->isValid()) {