Check return values for Sk[I]Rect::intersect (#54577)

The `SkRect::intersect` and `SkIRect::intersect` methods return values that must be handled or the calling code might end up with a non-empty answer for non-intersecting rectangles. There were 3 or 4 places in our code that we weren't doing this so now we check them all, in some cases even if we believe that the answer will not be empty.

This is prep work so that Skia can add `[[nodiscard]]` directives to these 2 methods so that nobody else makes that mistake.

Author: flar

display_list/display_list_unittests.cc

@@ -4119,6 +4119,32 @@ TEST_F(DisplayListTest, SaveLayerBoundsClipDetectionSimpleClippedRect) {
   EXPECT_TRUE(expector.all_bounds_checked());
 }
 
+TEST_F(DisplayListTest, DisjointSaveLayerBoundsProduceEmptySuppliedBounds) {
+  // This test was added when we fixed the Builder code to check the
+  // return value of the SkRect::intersect method, but it turns out
+  // that the indicated case never happens in practice due to the
+  // internal culling during the recording process. It actually passes
+  // both before and after the fix, but is here to ensure the right
+  // behavior does not regress.
+
+  SkRect layer_bounds = SkRect::MakeLTRB(10.0f, 10.0f, 20.0f, 20.0f);
+  SkRect draw_rect = SkRect::MakeLTRB(50.0f, 50.0f, 100.0f, 100.0f);
+  ASSERT_FALSE(layer_bounds.intersects(draw_rect));
+  ASSERT_FALSE(layer_bounds.isEmpty());
+  ASSERT_FALSE(draw_rect.isEmpty());
+
+  DisplayListBuilder builder;
+  builder.SaveLayer(&layer_bounds, nullptr);
+  builder.DrawRect(draw_rect, DlPaint());
+  builder.Restore();
+  auto display_list = builder.Build();
+
+  SaveLayerBoundsExpector expector;
+  expector.addSuppliedExpectation(SkRect::MakeEmpty(), false);
+  display_list->Dispatch(expector);
+  EXPECT_TRUE(expector.all_bounds_checked());
+}
+
 class DepthExpector : public virtual DlOpReceiver,
                       virtual IgnoreAttributeDispatchHelper,
                       virtual IgnoreTransformDispatchHelper,

display_list/dl_builder.cc

@@ -617,7 +617,12 @@ void DisplayListBuilder::RestoreLayer() {
   if (layer_op->options.bounds_from_caller()) {
     if (!content_bounds.isEmpty() && !layer_op->rect.contains(content_bounds)) {
       layer_op->options = layer_op->options.with_content_is_clipped();
-      content_bounds.intersect(layer_op->rect);
+      if (!content_bounds.intersect(layer_op->rect)) {
+        // Should never happen because we prune ops that don't intersect the
+        // supplied bounds so content_bounds would already be empty and we
+        // wouldn't come into this control block due to the empty test above.
+        content_bounds.setEmpty();
+      }
     }
   }
   layer_op->rect = content_bounds;

display_list/geometry/dl_region.cc

@@ -499,8 +499,8 @@ DlRegion DlRegion::MakeIntersection(const DlRegion& a, const DlRegion& b) {
     return DlRegion();
   } else if (a.isSimple() && b.isSimple()) {
     SkIRect r(a.bounds_);
+    [[maybe_unused]]
     auto res = r.intersect(b.bounds_);
-    (void)res;  // Suppress unused variable warning in release builds.
     FML_DCHECK(res);
     return DlRegion(r);
   } else if (a.isSimple() && a.bounds_.contains(b.bounds_)) {

flow/diff_context.cc

@@ -139,8 +139,12 @@ Damage DiffContext::ComputeDamage(const SkIRect& accumulated_buffer_damage,
   frame_damage.roundOut(&res.frame_damage);
 
   SkIRect frame_clip = SkIRect::MakeSize(frame_size_);
-  res.buffer_damage.intersect(frame_clip);
-  res.frame_damage.intersect(frame_clip);
+  if (!res.buffer_damage.intersect(frame_clip)) {
+    res.buffer_damage.setEmpty();
+  }
+  if (!res.frame_damage.intersect(frame_clip)) {
+    res.frame_damage.setEmpty();
+  }
 
   if (horizontal_clip_alignment > 1 || vertical_clip_alignment > 1) {
     AlignRect(res.buffer_damage, horizontal_clip_alignment,

flow/diff_context_unittests.cc

@@ -32,5 +32,41 @@ TEST_F(DiffContextTest, ClipAlignment) {
   EXPECT_EQ(damage.buffer_damage, SkIRect::MakeLTRB(16, 16, 64, 64));
 }
 
+TEST_F(DiffContextTest, DisjointDamage) {
+  SkISize frame_size = SkISize::Make(90, 90);
+  auto in_bounds_dl = CreateDisplayList(SkRect::MakeLTRB(30, 30, 50, 50));
+  auto out_bounds_dl = CreateDisplayList(SkRect::MakeLTRB(100, 100, 120, 120));
+
+  // We need both DisplayLists to be non-empty.
+  ASSERT_FALSE(in_bounds_dl->bounds().isEmpty());
+  ASSERT_FALSE(out_bounds_dl->bounds().isEmpty());
+
+  // We need the in_bounds DisplayList to be inside the frame size.
+  // We need the out_bounds DisplayList to be completely outside the frame.
+  ASSERT_TRUE(SkRect::Make(frame_size).contains(in_bounds_dl->bounds()));
+  ASSERT_FALSE(SkRect::Make(frame_size).intersects(out_bounds_dl->bounds()));
+
+  MockLayerTree t1(frame_size);
+  t1.root()->Add(CreateDisplayListLayer(in_bounds_dl));
+
+  MockLayerTree t2(frame_size);
+  // Include previous
+  t2.root()->Add(CreateDisplayListLayer(in_bounds_dl));
+  // Add a new layer that is out of frame bounds
+  t2.root()->Add(CreateDisplayListLayer(out_bounds_dl));
+
+  // Cannot use DiffLayerTree because it implicitly adds a clip layer
+  // around the tree, but we want the out of bounds dl to not be pruned
+  // to test the intersection code inside layer::Diff/ComputeDamage
+  // damage = DiffLayerTree(t2, t1, SkIRect::MakeEmpty(), 0, 0);
+
+  DiffContext dc(frame_size, t2.paint_region_map(), t1.paint_region_map(), true,
+                 false);
+  t2.root()->Diff(&dc, t1.root());
+  auto damage = dc.ComputeDamage(SkIRect::MakeEmpty(), 0, 0);
+  EXPECT_EQ(damage.frame_damage, SkIRect::MakeEmpty());
+  EXPECT_EQ(damage.buffer_damage, SkIRect::MakeEmpty());
+}
+
 }  // namespace testing
 }  // namespace flutter

flow/view_slicer.cc

@@ -89,11 +89,19 @@ std::unordered_map<int64_t, SkRect> SliceViews(
       }
 
       // Get the intersection rect with the `current_view_rect`,
-      partial_joined_rect.intersect(SkRect::Make(current_view_rect.roundOut()));
-
-      // Join the `partial_joined_rect` into `full_joined_rect` to get the rect
-      // above the current `slice`
-      full_joined_rect.join(partial_joined_rect);
+      if (partial_joined_rect.intersect(
+              SkRect::Make(current_view_rect.roundOut()))) {
+        // Join the `partial_joined_rect` into `full_joined_rect` to get the
+        // rect above the current `slice`, only if it intersects the indicated
+        // view. This should always be the case because we just deleted any
+        // rects that don't intersect the "rounded-in" view, so they must
+        // all intersect the "rounded-out" view (or the partial join could
+        // be empty in which case this would be a NOP). Either way, the
+        // penalty for not checking the return value of the intersect method
+        // would be to join a non-overlapping rectangle into the overlay
+        // bounds - if the above implementation ever changes - so we check it.
+        full_joined_rect.join(partial_joined_rect);
+      }
     }
 
     if (!full_joined_rect.isEmpty()) {