Reland Don't assert rect mapping result if impossible

The original CL was reverted because it broke some perf bots. The reason is that the slow path misses ancestor overflow clipping for some absolute-position object whose paint invalidation container is under the absolute-position object's container. This CL reland the original CL but doesn't enable CHECK_FAST_PATH_SLOW_PATH_EQUALITY by default. Avoid rect mapping result checking in the following cases: - Any rect may contain saturated values; - If a fixed-position ancestor disabled clipping. BUG=591199 CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_dbg,mac_blink_dbg,win_blink_dbg [email protected] Review-Url: https://codereview.chromium.org/1859833002 Cr-Commit-Position: refs/heads/master@{#390814} Review-Url: https://codereview.chromium.org/1936413002 Cr-Commit-Position: refs/heads/master@{#391145}
tonikitoo · May 3, 2016 · 0ddd48d · 0ddd48d
1 parent a85b86d
commit 0ddd48d
Show file tree

Hide file tree

Showing 3 changed files with 82 additions and 56 deletions.
diff --git a/third_party/WebKit/Source/core/layout/LayoutView.cpp b/third_party/WebKit/Source/core/layout/LayoutView.cpp
@@ -486,9 +486,6 @@ bool LayoutView::mapToVisualRectInAncestorSpace(const LayoutBoxModelObject* ance
 
 bool LayoutView::mapToVisualRectInAncestorSpace(const LayoutBoxModelObject* ancestor, LayoutRect& rect, MapCoordinatesFlags mode, VisualRectFlags visualRectFlags) const
 {
-    if (document().printing())
-        return true;
-
     // Convert the rect into the physical coordinates space of this LayoutView.
     flipForWritingMode(rect);
 

diff --git a/third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp b/third_party/WebKit/Source/core/layout/PaintInvalidationState.cpp
@@ -14,54 +14,8 @@
 #include "core/layout/svg/SVGLayoutSupport.h"
 #include "core/paint/PaintLayer.h"
 
-// We can't enable this by default because saturated operations of LayoutUnit
-// don't conform commutative law for overflowing results, preventing us from
-// making fast path and slow path always return the same result.
-#define ASSERT_SAME_RESULT_SLOW_AND_FAST_PATH (0 && ENABLE(ASSERT))
-
 namespace blink {
 
-#if ASSERT_SAME_RESULT_SLOW_AND_FAST_PATH
-// Make sure that the fast path and the slow path generate the same rect.
-void assertRectsEqual(const LayoutObject& object, const LayoutBoxModelObject& ancestor, const LayoutRect& rect, const LayoutRect& slowPathRect)
-{
-    // TODO(wangxianzhu): This is for cases that a sub-frame creates a root PaintInvalidationState
-    // which doesn't inherit clip from ancestor frames.
-    // Remove the condition when we eliminate the latter case of PaintInvalidationState(const LayoutView&, ...).
-    if (object.isLayoutView())
-        return;
-
-    // We ignore ancestor clipping for FixedPosition in fast path.
-    if (object.styleRef().position() == FixedPosition)
-        return;
-
-    // TODO(crbug.com/597903): Fast path and slow path should generate equal empty rects.
-    if (rect.isEmpty() && slowPathRect.isEmpty())
-        return;
-
-    if (rect == slowPathRect)
-        return;
-
-    // Tolerate the difference between the two paths when crossing frame boundaries.
-    if (object.view() != ancestor.view()) {
-        LayoutRect inflatedRect = rect;
-        inflatedRect.inflate(1);
-        if (inflatedRect.contains(slowPathRect))
-            return;
-        LayoutRect inflatedSlowPathRect = slowPathRect;
-        inflatedSlowPathRect.inflate(1);
-        if (inflatedSlowPathRect.contains(rect))
-            return;
-    }
-
-#ifndef NDEBUG
-    WTFLogAlways("Fast path paint invalidation rect differs from slow path: %s vs %s", rect.toString().ascii().data(), slowPathRect.toString().ascii().data());
-    showLayoutTree(&object);
-#endif
-    ASSERT_NOT_REACHED();
-}
-#endif
-
 static bool supportsCachedOffsets(const LayoutObject& object)
 {
     return !object.hasTransformRelatedProperty()
@@ -89,6 +43,9 @@ PaintInvalidationState::PaintInvalidationState(const LayoutView& layoutView, Vec
 #if ENABLE(ASSERT)
     , m_didUpdateForChildren(false)
 #endif
+#ifdef CHECK_FAST_PATH_SLOW_PATH_EQUALITY
+    , m_canCheckFastPathSlowPathEquality(layoutView == m_paintInvalidationContainer)
+#endif
 {
     if (!supportsCachedOffsets(layoutView)) {
         m_cachedOffsetsEnabled = false;
@@ -121,6 +78,9 @@ PaintInvalidationState::PaintInvalidationState(const PaintInvalidationState& par
 #if ENABLE(ASSERT)
     , m_didUpdateForChildren(false)
 #endif
+#ifdef CHECK_FAST_PATH_SLOW_PATH_EQUALITY
+    , m_canCheckFastPathSlowPathEquality(parentState.m_canCheckFastPathSlowPathEquality)
+#endif
 {
     if (currentObject == parentState.m_currentObject) {
         // Sometimes we create a new PaintInvalidationState from parentState on the same object
@@ -201,6 +161,9 @@ PaintInvalidationState::PaintInvalidationState(const PaintInvalidationState& par
 
         m_clipped = false; // Will be updated in updateForChildren().
         m_paintOffset = LayoutSize();
+#ifdef CHECK_FAST_PATH_SLOW_PATH_EQUALITY
+        m_canCheckFastPathSlowPathEquality = true;
+#endif
         return;
     }
 
@@ -235,7 +198,12 @@ void PaintInvalidationState::updateForCurrentObject(const PaintInvalidationState
         // In the above way to get paint offset, we can't get accurate clip rect, so just assume no clip.
         // Clip on fixed-position is rare, in case that paintInvalidationContainer crosses frame boundary
         // and the LayoutView is clipped by something in owner document.
-        m_clipped = false;
+        if (m_clipped) {
+            m_clipped = false;
+#ifdef CHECK_FAST_PATH_SLOW_PATH_EQUALITY
+            m_canCheckFastPathSlowPathEquality = false;
+#endif
+        }
         return;
     }
 
@@ -348,7 +316,7 @@ LayoutPoint PaintInvalidationState::computePositionFromPaintInvalidationBacking(
             if (m_currentObject.isSVG() && !m_currentObject.isSVGRoot())
                 point = m_svgTransform.mapPoint(point);
             point += FloatPoint(m_paintOffset);
-#if ASSERT_SAME_RESULT_SLOW_AND_FAST_PATH
+#ifdef CHECK_FAST_PATH_SLOW_PATH_EQUALITY
             // TODO(wangxianzhu): We can't enable this ASSERT for now because of crbug.com/597745.
             // ASSERT(point == slowLocalOriginToAncestorPoint(m_currentObject, m_paintInvalidationContainer, FloatPoint());
 #endif
@@ -384,9 +352,9 @@ LayoutRect PaintInvalidationState::computePaintInvalidationRectInBackingForSVG()
         rect.move(m_paintOffset);
         if (m_clipped)
             rect.intersect(m_clipRect);
-#if ASSERT_SAME_RESULT_SLOW_AND_FAST_PATH
+#ifdef CHECK_FAST_PATH_SLOW_PATH_EQUALITY
         LayoutRect slowPathRect = SVGLayoutSupport::clippedOverflowRectForPaintInvalidation(m_currentObject, *m_paintInvalidationContainer);
-        assertRectsEqual(m_currentObject, m_paintInvalidationContainer, rect, slowPathRect);
+        assertFastPathAndSlowPathRectsEqual(rect, slowPathRect);
 #endif
     } else {
         // TODO(wangxianzhu): Sometimes m_cachedOffsetsEnabled==false doesn't mean we can't use cached
@@ -412,15 +380,15 @@ void PaintInvalidationState::mapLocalRectToPaintInvalidationContainer(LayoutRect
     ASSERT(!m_didUpdateForChildren);
 
     if (m_cachedOffsetsEnabled) {
-#if ASSERT_SAME_RESULT_SLOW_AND_FAST_PATH
+#ifdef CHECK_FAST_PATH_SLOW_PATH_EQUALITY
         LayoutRect slowPathRect(rect);
         slowMapToVisualRectInAncestorSpace(m_currentObject, *m_paintInvalidationContainer, slowPathRect);
 #endif
         rect.move(m_paintOffset);
         if (m_clipped)
             rect.intersect(m_clipRect);
-#if ASSERT_SAME_RESULT_SLOW_AND_FAST_PATH
-        assertRectsEqual(m_currentObject, *m_paintInvalidationContainer, rect, slowPathRect);
+#ifdef CHECK_FAST_PATH_SLOW_PATH_EQUALITY
+        assertFastPathAndSlowPathRectsEqual(rect, slowPathRect);
 #endif
     } else {
         slowMapToVisualRectInAncestorSpace(m_currentObject, *m_paintInvalidationContainer, rect);
@@ -455,4 +423,56 @@ PaintLayer& PaintInvalidationState::enclosingSelfPaintingLayer(const LayoutObjec
     return m_enclosingSelfPaintingLayer;
 }
 
+#ifdef CHECK_FAST_PATH_SLOW_PATH_EQUALITY
+
+static bool mayHaveBeenSaturated(LayoutUnit value)
+{
+    // This is not accurate, just to avoid too big values.
+    return value.abs() >= LayoutUnit::max() / 2;
+}
+
+static bool mayHaveBeenSaturated(const LayoutRect& rect)
+{
+    return mayHaveBeenSaturated(rect.x()) || mayHaveBeenSaturated(rect.y()) || mayHaveBeenSaturated(rect.width()) || mayHaveBeenSaturated(rect.height());
+}
+
+void PaintInvalidationState::assertFastPathAndSlowPathRectsEqual(const LayoutRect& fastPathRect, const LayoutRect& slowPathRect) const
+{
+    if (!m_canCheckFastPathSlowPathEquality)
+        return;
+
+    // TODO(crbug.com/597903): Fast path and slow path should generate equal empty rects.
+    if (fastPathRect.isEmpty() && slowPathRect.isEmpty())
+        return;
+
+    if (fastPathRect == slowPathRect)
+        return;
+
+    // LayoutUnit uses saturated arithmetic operations. If any interim or final result is saturated,
+    // the same operations in different order produce different results. Don't compare results
+    // if any of them may have been saturated.
+    if (mayHaveBeenSaturated(fastPathRect) || mayHaveBeenSaturated(slowPathRect))
+        return;
+
+    // Tolerate the difference between the two paths when crossing frame boundaries.
+    if (m_currentObject.view() != m_paintInvalidationContainer->view()) {
+        LayoutRect inflatedFastPathRect = fastPathRect;
+        inflatedFastPathRect.inflate(1);
+        if (inflatedFastPathRect.contains(slowPathRect))
+            return;
+        LayoutRect inflatedSlowPathRect = slowPathRect;
+        inflatedSlowPathRect.inflate(1);
+        if (inflatedSlowPathRect.contains(fastPathRect))
+            return;
+    }
+
+    WTFLogAlways("Fast path paint invalidation rect differs from slow path: fast: %s vs slow: %s",
+        fastPathRect.toString().ascii().data(), slowPathRect.toString().ascii().data());
+    showLayoutTree(&m_currentObject);
+
+    ASSERT_NOT_REACHED();
+}
+
+#endif // CHECK_FAST_PATH_SLOW_PATH_EQUALITY
+
 } // namespace blink
diff --git a/third_party/WebKit/Source/core/layout/PaintInvalidationState.h b/third_party/WebKit/Source/core/layout/PaintInvalidationState.h
@@ -142,6 +142,15 @@ class CORE_EXPORT PaintInvalidationState {
 #if ENABLE(ASSERT)
     bool m_didUpdateForChildren;
 #endif
+
+#if ENABLE(ASSERT) && !defined(NDEBUG)
+// #define CHECK_FAST_PATH_SLOW_PATH_EQUALITY
+#endif
+
+#ifdef CHECK_FAST_PATH_SLOW_PATH_EQUALITY
+    void assertFastPathAndSlowPathRectsEqual(const LayoutRect& fastPathRect, const LayoutRect& slowPathRect) const;
+    bool m_canCheckFastPathSlowPathEquality;
+#endif
 };
 
 } // namespace blink