Fix false-positive of under-invalidation checking

If a subsequence was fully painted the last time, we don't repaint the
subsequence on interest rect change because the fully painted result is
still useable. However this triggers false-positive of under-
invalidation checking which strictly matches new and cached
subsequences.

Bug: 755478
Cq-Include-Trybots: master.tryserver.chromium.linux:linux_layout_tests_slimming_paint_v2
Change-Id: I24f8fa9c2e3167ff3c54954bf5e994a74ef70056
Reviewed-on: https://chromium-review.googlesource.com/661379
Commit-Queue: Xianzhu Wang <wangxianzhu@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#501161}

third_party/WebKit/LayoutTests/FlagExpectations/enable-slimming-paint-v2

@@ -385,7 +385,7 @@ Bug(none) compositing/squashing/repaint-child-of-squashed.html [ Failure ]
 Bug(none) compositing/squashing/selection-repaint-with-gaps.html [ Failure ]
 Bug(none) compositing/squashing/squash-above-fixed-1.html [ Failure ]
 Bug(none) compositing/squashing/squash-above-fixed-2.html [ Crash Failure ]
-Bug(none) compositing/squashing/squash-above-fixed-3.html [ Failure ]
+Bug(none) compositing/squashing/squash-above-fixed-3.html [ Crash Failure ]
 Bug(none) compositing/squashing/squash-compositing-hover.html [ Failure ]
 Bug(none) compositing/squashing/squash-onto-distant-relative.html [ Failure ]
 Bug(none) compositing/squashing/squash-onto-nephew.html [ Failure ]

third_party/WebKit/Source/core/paint/PaintLayerPainter.cpp

@@ -230,7 +230,12 @@ static bool ShouldRepaintSubsequence(
 
   // Repaint if previously the layer might be clipped by paintDirtyRect and
   // paintDirtyRect changes.
-  if (paint_layer.PreviousPaintResult() == kMayBeClippedByPaintDirtyRect &&
+  if ((paint_layer.PreviousPaintResult() == kMayBeClippedByPaintDirtyRect ||
+       // When PaintUnderInvalidationChecking is enabled, always repaint the
+       // subsequence when the paint rect changes because we will strictly match
+       // new and cached subsequences. Normally we can reuse the cached fully
+       // painted subsequence even if we would partially paint this time.
+       RuntimeEnabledFeatures::PaintUnderInvalidationCheckingEnabled()) &&
       paint_layer.PreviousPaintDirtyRect() != painting_info.paint_dirty_rect) {
     needs_repaint = true;
     should_clear_empty_paint_phase_flags = true;

third_party/WebKit/Source/core/paint/PaintLayerPainterTest.cpp

@@ -271,6 +271,29 @@ TEST_P(PaintLayerPainterTest, CachedSubsequenceOnInterestRectChange) {
       TestDisplayItem(content2b, kBackgroundType));
 }
 
+TEST_P(PaintLayerPainterTest,
+       CachedSubsequenceOnInterestRectChangeUnderInvalidationChecking) {
+  ScopedPaintUnderInvalidationCheckingForTest under_invalidation_checking(true);
+
+  SetBodyInnerHTML(
+      "<style>p { width: 200px; height: 50px; background: green }</style>"
+      "<div id='target' style='position: relative; z-index: 1'>"
+      "  <p></p><p></p><p></p><p></p>"
+      "</div>");
+  RootPaintController().InvalidateAll();
+
+  // |target| will be fully painted.
+  GetDocument().View()->UpdateAllLifecyclePhasesExceptPaint();
+  IntRect interest_rect(0, 0, 400, 300);
+  Paint(&interest_rect);
+
+  // |target| will be partially painted. Should not trigger under-invalidation
+  // checking DCHECKs.
+  GetDocument().View()->UpdateAllLifecyclePhasesExceptPaint();
+  IntRect new_interest_rect(0, 100, 300, 1000);
+  Paint(&new_interest_rect);
+}
+
 TEST_P(PaintLayerPainterTest,
        CachedSubsequenceOnStyleChangeWithInterestRectClipping) {
   SetBodyInnerHTML(

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

@@ -173,18 +173,9 @@ size_t PaintController::BeginSubsequence() {
 void PaintController::EndSubsequence(const DisplayItemClient& client,
                                      size_t start) {
   size_t end = new_display_item_list_.size();
-  if (start == end) {
-    // Omit the empty subsequence. The forcing-new-chunk flag set by
-    // BeginSubsequence() still applies, but this not a big deal because empty
-    // subsequences are not common. Also we should not clear the flag because
-    // there might be unhandled flag that was set before this empty subsequence.
-    return;
-  }
-
-  // Force new paint chunk which is required for subsequence caching.
-  new_paint_chunks_.ForceNewChunk();
 
-  if (IsCheckingUnderInvalidation()) {
+  if (RuntimeEnabledFeatures::PaintUnderInvalidationCheckingEnabled() &&
+      IsCheckingUnderInvalidation()) {
     SubsequenceMarkers* markers = GetSubsequenceMarkers(client);
     if (!markers) {
       ShowSequenceUnderInvalidationError(
@@ -197,8 +188,20 @@ void PaintController::EndSubsequence(const DisplayItemClient& client,
           end);
       CHECK(false);
     }
+    under_invalidation_checking_end_ = 0;
   }
 
+  if (start == end) {
+    // Omit the empty subsequence. The forcing-new-chunk flag set by
+    // BeginSubsequence() still applies, but this not a big deal because empty
+    // subsequences are not common. Also we should not clear the flag because
+    // there might be unhandled flag that was set before this empty subsequence.
+    return;
+  }
+
+  // Force new paint chunk which is required for subsequence caching.
+  new_paint_chunks_.ForceNewChunk();
+
   DCHECK(new_cached_subsequences_.find(&client) ==
          new_cached_subsequences_.end());
 

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

@@ -306,9 +306,8 @@ class PLATFORM_EXPORT PaintController {
 
   void CheckUnderInvalidation();
   bool IsCheckingUnderInvalidation() const {
-    return under_invalidation_checking_end_ -
-               under_invalidation_checking_begin_ >
-           0;
+    return under_invalidation_checking_end_ >
+           under_invalidation_checking_begin_;
   }
 
   struct SubsequenceMarkers {

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

@@ -2387,6 +2387,25 @@ class PaintControllerUnderInvalidationTest
     }
     GetPaintController().CommitNewDisplayItems();
   }
+
+  void TestSubsequenceBecomesEmpty() {
+    FakeDisplayItemClient target("target");
+    GraphicsContext context(GetPaintController());
+
+    {
+      SubsequenceRecorder r(context, target);
+      DrawRect(context, target, kBackgroundDrawingType,
+               FloatRect(100, 100, 300, 300));
+    }
+    GetPaintController().CommitNewDisplayItems();
+
+    {
+      EXPECT_FALSE(
+          SubsequenceRecorder::UseCachedSubsequenceIfPossible(context, target));
+      SubsequenceRecorder r(context, target);
+    }
+    GetPaintController().CommitNewDisplayItems();
+  }
 };
 
 TEST_F(PaintControllerUnderInvalidationTest, ChangeDrawing) {
@@ -2436,6 +2455,10 @@ TEST_F(PaintControllerUnderInvalidationTest, InvalidationInSubsequence) {
   TestInvalidationInSubsequence();
 }
 
+TEST_F(PaintControllerUnderInvalidationTest, SubsequenceBecomesEmpty) {
+  EXPECT_DEATH(TestSubsequenceBecomesEmpty(), "");
+}
+
 #endif  // defined(GTEST_HAS_DEATH_TEST) && !defined(OS_ANDROID)
 
 }  // namespace blink