Handle removed display items in under-invalidation checking in cached subsequences

third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp

Index: third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
diff --git a/third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp b/third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
index f91a64b7198679dcefe1a5c9a6a4ee6c98c41379..d6ff91fa89591b4d024c26b86ad520aa0a82fc0a 100644
--- a/third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
+++ b/third_party/WebKit/Source/platform/graphics/paint/PaintController.cpp
@@ -139,6 +139,15 @@ void PaintController::removeLastDisplayItem()
         if (!indices.isEmpty() && indices.last() == (m_newDisplayItemList.size() - 1))
             indices.removeLast();
     }
+
+    if (RuntimeEnabledFeatures::slimmingPaintUnderInvalidationCheckingEnabled() && isCheckingUnderInvalidation()) {
+        if (m_skippedUnderInvalidationCount) {
+            --m_skippedUnderInvalidationCount;
+        } else {
+            DCHECK(m_underInvalidationCheckingBegin);
+            --m_underInvalidationCheckingBegin;
+        }
+    }
 #endif
     m_newDisplayItemList.removeLast();
 
@@ -401,6 +410,7 @@ void PaintController::resetCurrentListIndices()
 #if DCHECK_IS_ON()
     m_underInvalidationCheckingBegin = 0;
     m_underInvalidationCheckingEnd = 0;
+    m_skippedUnderInvalidationCount = 0;
 #endif
 }
 
@@ -504,24 +514,24 @@ void PaintController::appendDebugDrawingAfterCommit(const DisplayItemClient& dis
 
 #if DCHECK_IS_ON()
 
-void PaintController::showUnderInvalidationError(const char* reason, const DisplayItem& newItem, const DisplayItem& oldItem) const
+void PaintController::showUnderInvalidationError(const char* reason, const DisplayItem& newItem, const DisplayItem* oldItem) const
 {
     LOG(ERROR) << m_underInvalidationMessagePrefix << " " << reason;
 #ifndef NDEBUG
     LOG(ERROR) << "New display item: " << newItem.asDebugString();
-    LOG(ERROR) << "Old display item: " << oldItem.asDebugString();
+    LOG(ERROR) << "Old display item: " << (oldItem ? oldItem->asDebugString() : "None");
 #else
     LOG(ERROR) << "Run debug build to get more details.";
 #endif
     LOG(ERROR) << "See http://crbug.com/619103.";
 
 #ifndef NDEBUG
-    if (newItem.isDrawing()) {
-        RefPtr<const SkPicture> newPicture = static_cast<const DrawingDisplayItem&>(newItem).picture();
-        RefPtr<const SkPicture> oldPicture = static_cast<const DrawingDisplayItem&>(oldItem).picture();
-        LOG(INFO) << "new picture:\n" << (newPicture ? pictureAsDebugString(newPicture.get()) : "None");
-        LOG(INFO) << "old picture:\n" << (oldPicture ? pictureAsDebugString(oldPicture.get()) : "None");
-    }
+    const SkPicture* newPicture = newItem.isDrawing() ? static_cast<const DrawingDisplayItem&>(newItem).picture() : nullptr;
+    const SkPicture* oldPicture = oldItem && oldItem->isDrawing() ? static_cast<const DrawingDisplayItem*>(oldItem)->picture() : nullptr;
+    LOG(INFO) << "new picture:\n" << (newPicture ? pictureAsDebugString(newPicture) : "None");
+    LOG(INFO) << "old picture:\n" << (oldPicture ? pictureAsDebugString(oldPicture) : "None");
+
+    showDebugData();
 #endif // NDEBUG
 }
 
@@ -533,16 +543,37 @@ void PaintController::checkUnderInvalidation()
         return;
 
     const DisplayItem& newItem = m_newDisplayItemList.last();
-    const DisplayItem& oldItem = m_currentPaintArtifact.getDisplayItemList()[m_underInvalidationCheckingBegin++];
+    size_t oldItemIndex = m_underInvalidationCheckingBegin + m_skippedUnderInvalidationCount;
+    const DisplayItem* oldItem = oldItemIndex < m_currentPaintArtifact.getDisplayItemList().size() ? &m_currentPaintArtifact.getDisplayItemList()[oldItemIndex] : nullptr;
 
     if (newItem.isCacheable() && !clientCacheIsValid(newItem.client())) {
         showUnderInvalidationError("under-invalidation of PaintLayer: invalidated in cached subsequence", newItem, oldItem);
         NOTREACHED();
     }
-    if (!newItem.equals(oldItem)) {
-        showUnderInvalidationError("under-invalidation: display item changed", newItem, oldItem);
+
+    bool oldAndNewEqual = oldItem && newItem.equals(*oldItem);
+    if (!oldAndNewEqual) {

[chrishtr, 2016/07/28 16:55:36]

Don't you need to reset m_skippedUnderInvalidationCount to zero as soon as the
pattern of begin/end or compositebegin/drawing/end does *not* happen?

[Xianzhu, 2016/07/28 17:20:48]

No. If the pattern does not happen, non-zero m_skippedUnderInvalidationCount
means real under-invalidation because the mismatching display items actually
exist. Line 568-274 below will report under-invalidation in the case.

[chrishtr, 2016/07/28 17:29:21]

Suppose the display list is this:

BeginCompositing - Drawing - Drawing - EndCompositing

After the first two are processed, won't m_skippedUnderInvalidationCount be 2?
What resets it to 0 when the second Drawing is encountered?

[Xianzhu, 2016/07/28 17:38:40]

It's not reset to 0. It is non-zero only if we have seen mismatching display
items in a subsequence that is supposed to use the cache. It's 2 only if the
BeginCompositing and the first Drawing didn't match the cached display items. At
the second Drawing, we know that the skipped probable under-invalidations are
real under-invalidations and will report the mismatching BeginCompositing as
under-invalidation.

If all the new display items are matching cached ones,
m_skippedUnderInvalidationCount will be always 0.

+        if (newItem.isBegin()) {
+            // Ignore mismatching begin display item which may be removed when we remove a no-op pair.
+            ++m_skippedUnderInvalidationCount;
+            return;
+        }
+        if (newItem.isDrawing() && m_skippedUnderInvalidationCount == 1 && m_newDisplayItemList[m_newDisplayItemList.size() - 2].getType() == DisplayItem::BeginCompositing) {

[chrishtr, 2016/07/28 16:55:36]

Check if the display list has length at least 2.

[Xianzhu, 2016/07/28 17:20:48]

Added a DCHECK. The condition should be always true because
m_newDisplayItemList.size() > m_skippedUnderInvalidationCount.

+            // This might be a drawing item between a pair of begin/end compositing display items that will be folded into a single drawing display item.
+            ++m_skippedUnderInvalidationCount;
+            return;
+        }
+    }
+
+    if (m_skippedUnderInvalidationCount || !oldAndNewEqual) {
+        // If we ever skipped reporting any under-invalidations, report the earliest one.
+        showUnderInvalidationError("under-invalidation: display item changed",
+            m_newDisplayItemList[m_newDisplayItemList.size() - m_skippedUnderInvalidationCount - 1],
+            &m_currentPaintArtifact.getDisplayItemList()[m_underInvalidationCheckingBegin]);
         NOTREACHED();
     }
+
+    ++m_underInvalidationCheckingBegin;
 }
 
 #endif // DCHECK_IS_ON()