Changeset 142982 in webkit

Timestamp:

Feb 15, 2013 4:28:19 AM (11 years ago)

Author:

commit-queue@webkit.org

Message:

[CSS Regions][Mac] fast/regions/full-screen-video-from-region.html hits an assertion in RenderFlowThread::removeRenderBoxRegionInfo
​https://bugs.webkit.org/show_bug.cgi?id=106075

Patch by Andrei Bucur <​abucur@adobe.com> on 2013-02-15
Reviewed by Tony Chang.

Source/WebCore:

The crash is caused by two issues.

The first problem is how a block inside a flow thread determines if the children needs relayout or not.
When the region chain is invalidated, the information is lost so we need to return true, even for the
enclosing RenderFlowThread. Because the video renderer is the first child of the flow thread this doesn't
happen.

The patch implements this behaviour by inspecting both if the region chain has changed and
if the block has no range computed yet.

The second problem is RenderMedia not inheriting from RenderBlock. The logic of child relayout doesn't apply
to it. In the test case, when the full screen button is pressed, the region changes width to fill the viewport,
the chain is invalidated and the box info hash map is cleared. When the video is laid out again (after fixing
the first issue) it has the same size so the controls don't do a layout. They remain without box info inside
the flow thread, thus causing the assertion.

The patch forces the controls to relayout if the region chain was invalidated. We can't use the
logicalWidthChangedInRegions method because it is block specific. This will be fixed in a later patch.

Tests: No new tests. fast/regions/full-screen-video-from-region.html no longer crashes.

• rendering/RenderBlock.cpp:

(WebCore::RenderBlock::checkForPaginationLogicalHeightChange):

• rendering/RenderFlowThread.cpp:

(WebCore::RenderFlowThread::RenderFlowThread):
(WebCore::RenderFlowThread::layout):
(WebCore::RenderFlowThread::logicalWidthChangedInRegions):

• rendering/RenderFlowThread.h: Renamed pageLogicalHeightChanged to pageLogicalSizeChanged.
• rendering/RenderMedia.cpp:

(WebCore::RenderMedia::layout):

LayoutTests:

Removed the crash/fail expectation for fast/regions/full-screen-video-from-region.html.

• platform/mac/TestExpectations:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/platform/mac/TestExpectations (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/rendering/RenderBlock.cpp (modified) (1 diff)
• Source/WebCore/rendering/RenderFlowThread.cpp (modified) (5 diffs)
• Source/WebCore/rendering/RenderFlowThread.h (modified) (2 diffs)
• Source/WebCore/rendering/RenderMedia.cpp (modified) (2 diffs)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+2013-02-15  Andrei Bucur  <abucur@adobe.com>
+
+        [CSS Regions][Mac] fast/regions/full-screen-video-from-region.html hits an assertion in RenderFlowThread::removeRenderBoxRegionInfo
+        https://bugs.webkit.org/show_bug.cgi?id=106075
+
+        Reviewed by Tony Chang.
+
+        Removed the crash/fail expectation for fast/regions/full-screen-video-from-region.html.
+
+        * platform/mac/TestExpectations:
+
 2013-02-15  Andrew Wilson  <atwilson@chromium.org>
 

trunk/LayoutTests/platform/mac/TestExpectations

@@ -1351 +1351 @@
 webkit.org/b/105999 [ Lion ] fast/canvas/canvas-composite-image.html [ Failure ]
 
-webkit.org/b/106075 [ Debug ] fast/regions/full-screen-video-from-region.html [ Crash ]
-
 webkit.org/b/106151 [ Lion ] http/tests/misc/link-rel-icon-beforeload.html [ Pass Failure ]
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2013-02-15  Andrei Bucur  <abucur@adobe.com>
+
+        [CSS Regions][Mac] fast/regions/full-screen-video-from-region.html hits an assertion in RenderFlowThread::removeRenderBoxRegionInfo
+        https://bugs.webkit.org/show_bug.cgi?id=106075
+
+        Reviewed by Tony Chang.
+
+        The crash is caused by two issues.
+
+        The first problem is how a block inside a flow thread determines if the children needs relayout or not.
+        When the region chain is invalidated, the information is lost so we need to return true, even for the
+        enclosing RenderFlowThread. Because the video renderer is the first child of the flow thread this doesn't
+        happen.
+
+        The patch implements this behaviour by inspecting both if the region chain has changed and
+        if the block has no range computed yet.
+
+        The second problem is RenderMedia not inheriting from RenderBlock. The logic of child relayout doesn't apply
+        to it. In the test case, when the full screen button is pressed, the region changes width to fill the viewport,
+        the chain is invalidated and the box info hash map is cleared. When the video is laid out again (after fixing
+        the first issue) it has the same size so the controls don't do a layout. They remain without box info inside
+        the flow thread, thus causing the assertion.
+
+        The patch forces the controls to relayout if the region chain was invalidated. We can't use the
+        logicalWidthChangedInRegions method because it is block specific. This will be fixed in a later patch.
+
+        Tests: No new tests. fast/regions/full-screen-video-from-region.html no longer crashes.
+
+        * rendering/RenderBlock.cpp:
+        (WebCore::RenderBlock::checkForPaginationLogicalHeightChange):
+        * rendering/RenderFlowThread.cpp:
+        (WebCore::RenderFlowThread::RenderFlowThread):
+        (WebCore::RenderFlowThread::layout):
+        (WebCore::RenderFlowThread::logicalWidthChangedInRegions):
+        * rendering/RenderFlowThread.h: Renamed pageLogicalHeightChanged to pageLogicalSizeChanged.
+        * rendering/RenderMedia.cpp:
+        (WebCore::RenderMedia::layout):
+
 2013-02-13  Allan Sandfeld Jensen  <allan.jensen@digia.com>
 

trunk/Source/WebCore/rendering/RenderBlock.cpp

@@ -1474 +1474 @@
     } else if (isRenderFlowThread()) {
         pageLogicalHeight = 1; // This is just a hack to always make sure we have a page logical height.
-        pageLogicalHeightChanged = toRenderFlowThread(this)->pageLogicalHeightChanged();
+        pageLogicalHeightChanged = toRenderFlowThread(this)->pageLogicalSizeChanged();
     }
 }

trunk/Source/WebCore/rendering/RenderFlowThread.cpp

@@ -57 +57 @@
     , m_hasRegionsWithStyling(false)
     , m_dispatchRegionLayoutUpdateEvent(false)
-    , m_pageLogicalHeightChanged(false)
+    , m_pageLogicalSizeChanged(false)
 {
     ASSERT(document->cssRegionsEnabled());
@@ -137 +137 @@
     StackStats::LayoutCheckPoint layoutCheckPoint;
 
-    m_pageLogicalHeightChanged = m_regionsInvalidated && everHadLayout();
+    m_pageLogicalSizeChanged = m_regionsInvalidated && everHadLayout();
     if (m_regionsInvalidated) {
         m_regionsInvalidated = false;
@@ -188 +188 @@
     RenderBlock::layout();
 
-    m_pageLogicalHeightChanged = false;
+    m_pageLogicalSizeChanged = false;
 
     if (lastRegion())
@@ -461 +461 @@
 bool RenderFlowThread::logicalWidthChangedInRegions(const RenderBlock* block, LayoutUnit offsetFromLogicalTopOfFirstPage)
 {
-    if (!hasRegions() || block == this) // Not necessary, since if any region changes, we do a full pagination relayout anyway.
+    if (!hasRegions())
         return false;
 
@@ -468 +468 @@
     getRegionRangeForBox(block, startRegion, endRegion);
 
-    // If the block doesn't have a startRegion (and implicitly a region range) it's safe to assume the width in regions has changed (e.g. the region chain was invalidated).
-    if (!startRegion)
+    // When the region chain is invalidated the box information is discarded so we must assume the width has changed.
+    if (m_pageLogicalSizeChanged && !startRegion)
         return true;
+
+    // Not necessary for the flow thread, since we already computed the correct info for it.
+    if (block == this)
+        return false;
 
     for (RenderRegionList::iterator iter = m_regionList.find(startRegion); iter != m_regionList.end(); ++iter) {

trunk/Source/WebCore/rendering/RenderFlowThread.h

@@ -134 +134 @@
     bool addForcedRegionBreak(LayoutUnit, RenderObject* breakChild, bool isBefore, LayoutUnit* offsetBreakAdjustment = 0);
 
-    bool pageLogicalHeightChanged() const { return m_pageLogicalHeightChanged; }
+    bool pageLogicalSizeChanged() const { return m_pageLogicalSizeChanged; }
 
     bool hasAutoLogicalHeightRegions() const { ASSERT(isAutoLogicalHeightRegionsCountConsistent()); return m_autoLogicalHeightRegionsCount; }
@@ -205 +205 @@
     bool m_hasRegionsWithStyling : 1;
     bool m_dispatchRegionLayoutUpdateEvent : 1;
-    bool m_pageLogicalHeightChanged : 1;
+    bool m_pageLogicalSizeChanged : 1;
 };
 

trunk/Source/WebCore/rendering/RenderMedia.cpp

@@ -30 +30 @@
 
 #include "HTMLMediaElement.h"
+#include "RenderFlowThread.h"
 #include "RenderView.h"
 
@@ -67 +68 @@
         return;
 
+    bool controlsNeedLayout = controlsRenderer->needsLayout();
+    // If the region chain has changed we also need to relayout the controls to update the region box info.
+    // FIXME: We can do better once we compute region box info for RenderReplaced, not only for RenderBlock.
+    if (inRenderFlowThread() && !controlsNeedLayout) {
+        const RenderFlowThread* flowThread = enclosingRenderFlowThread();
+        ASSERT(flowThread);
+        if (flowThread->pageLogicalSizeChanged())
+            controlsNeedLayout = true;
+    }
+
     LayoutSize newSize = contentBoxRect().size();
-    if (newSize == oldSize && !controlsRenderer->needsLayout())
+    if (newSize == oldSize && !controlsNeedLayout)
         return;