Fix clip serialization crash

Can't safely rewind clip allocations, since those pointers are cached by
ClipArea. Instead add early rejection to handle most cases, and update
tests.

Change-Id: Ic32f95cf95602f427f25761a8da1583c4495f36d

4 files changed, 29 insertions, 26 deletions

diff --git a/libs/hwui/BakedOpRenderer.cpp b/libs/hwui/BakedOpRenderer.cpp
index b9c13e6cf847..c1f19a341956 100644
--- a/libs/hwui/BakedOpRenderer.cpp
+++ b/libs/hwui/BakedOpRenderer.cpp
@@ -201,6 +201,7 @@ void BakedOpRenderer::setupStencilQuads(std::vector<Vertex>& quadVertices,
 }
 
 void BakedOpRenderer::setupStencilRectList(const ClipBase* clip) {
+    LOG_ALWAYS_FATAL_IF(clip->mode != ClipMode::RectangleList, "can't rectlist clip without rectlist");
     auto&& rectList = reinterpret_cast<const ClipRectList*>(clip)->rectList;
     int quadCount = rectList.getTransformedRectanglesCount();
     std::vector<Vertex> rectangleVertices;
@@ -234,6 +235,7 @@ void BakedOpRenderer::setupStencilRectList(const ClipBase* clip) {
 }
 
 void BakedOpRenderer::setupStencilRegion(const ClipBase* clip) {
+    LOG_ALWAYS_FATAL_IF(clip->mode != ClipMode::Region, "can't region clip without region");
     auto&& region = reinterpret_cast<const ClipRegion*>(clip)->region;
 
     std::vector<Vertex> regionVertices;
diff --git a/libs/hwui/BakedOpState.cpp b/libs/hwui/BakedOpState.cpp
index f1cc846593db..f0406fa13a3d 100644
--- a/libs/hwui/BakedOpState.cpp
+++ b/libs/hwui/BakedOpState.cpp
@@ -48,10 +48,10 @@ ResolvedRenderState::ResolvedRenderState(LinearAllocator& allocator, Snapshot& s
     const Rect& clipRect = clipState->rect;
     if (CC_UNLIKELY(clipRect.isEmpty() || !clippedBounds.intersects(clipRect))) {
         // Rejected based on either empty clip, or bounds not intersecting with clip
-        if (clipState) {
-            allocator.rewindIfLastAlloc(clipState);
-            clipState = nullptr;
-        }
+
+        // Note: we could rewind the clipState object in situations where the clipRect is empty,
+        // but *only* if the caching logic within ClipArea was aware of the rewind.
+        clipState = nullptr;
         clippedBounds.setEmpty();
     } else {
         // Not rejected! compute true clippedBounds and clipSideFlags
diff --git a/libs/hwui/BakedOpState.h b/libs/hwui/BakedOpState.h
index 5c7b43f5a034..3db28c982469 100644
--- a/libs/hwui/BakedOpState.h
+++ b/libs/hwui/BakedOpState.h
@@ -99,6 +99,7 @@ class BakedOpState {
 public:
     static BakedOpState* tryConstruct(LinearAllocator& allocator,
             Snapshot& snapshot, const RecordedOp& recordedOp) {
+        if (CC_UNLIKELY(snapshot.getRenderTargetClip().isEmpty())) return nullptr;
         BakedOpState* bakedState = new (allocator) BakedOpState(
                 allocator, snapshot, recordedOp, false);
         if (bakedState->computedState.clippedBounds.isEmpty()) {
@@ -118,6 +119,7 @@ public:
 
     static BakedOpState* tryStrokeableOpConstruct(LinearAllocator& allocator,
             Snapshot& snapshot, const RecordedOp& recordedOp, StrokeBehavior strokeBehavior) {
+        if (CC_UNLIKELY(snapshot.getRenderTargetClip().isEmpty())) return nullptr;
         bool expandForStroke = (strokeBehavior == StrokeBehavior::StyleDefined)
                 ? (recordedOp.paint && recordedOp.paint->getStyle() != SkPaint::kFill_Style)
                 : true;
@@ -126,6 +128,7 @@ public:
                 allocator, snapshot, recordedOp, expandForStroke);
         if (bakedState->computedState.clippedBounds.isEmpty()) {
             // bounds are empty, so op is rejected
+            // NOTE: this won't succeed if a clip was allocated
             allocator.rewindIfLastAlloc(bakedState);
             return nullptr;
         }
@@ -134,7 +137,7 @@ public:
 
     static BakedOpState* tryShadowOpConstruct(LinearAllocator& allocator,
             Snapshot& snapshot, const ShadowOp* shadowOpPtr) {
-        if (snapshot.getRenderTargetClip().isEmpty()) return nullptr;
+        if (CC_UNLIKELY(snapshot.getRenderTargetClip().isEmpty())) return nullptr;
 
         // clip isn't empty, so construct the op
         return new (allocator) BakedOpState(allocator, snapshot, shadowOpPtr);
diff --git a/libs/hwui/tests/unit/BakedOpStateTests.cpp b/libs/hwui/tests/unit/BakedOpStateTests.cpp
index 3fd822d71310..b3506869e0dc 100644
--- a/libs/hwui/tests/unit/BakedOpStateTests.cpp
+++ b/libs/hwui/tests/unit/BakedOpStateTests.cpp
@@ -176,29 +176,26 @@ TEST(ResolvedRenderState, construct_expandForStroke) {
 }
 
 TEST(BakedOpState, tryConstruct) {
-    LinearAllocator allocator;
-
     Matrix4 translate100x0;
     translate100x0.loadTranslate(100, 0, 0);
 
     SkPaint paint;
     ClipRect clip(Rect(100, 200));
-    {
-        RectOp rejectOp(Rect(30, 40, 100, 200), translate100x0, &clip, &paint);
-        auto snapshot = TestUtils::makeSnapshot(Matrix4::identity(), Rect(100, 200));
-        BakedOpState* bakedState = BakedOpState::tryConstruct(allocator, *snapshot, rejectOp);
-
-        EXPECT_EQ(nullptr, bakedState); // rejected by clip, so not constructed
-        EXPECT_GT(8u, allocator.usedSize()); // no significant allocation space used for rejected op
-    }
-    {
-        RectOp successOp(Rect(30, 40, 100, 200), Matrix4::identity(), &clip, &paint);
-        auto snapshot = TestUtils::makeSnapshot(Matrix4::identity(), Rect(100, 200));
-        BakedOpState* bakedState = BakedOpState::tryConstruct(allocator, *snapshot, successOp);
 
-        EXPECT_NE(nullptr, bakedState); // NOT rejected by clip, so will be constructed
-        EXPECT_LE(64u, allocator.usedSize()); // relatively large alloc for non-rejected op
-    }
+    LinearAllocator allocator;
+    RectOp successOp(Rect(30, 40, 100, 200), Matrix4::identity(), &clip, &paint);
+    auto snapshot = TestUtils::makeSnapshot(Matrix4::identity(), Rect(100, 200));
+    EXPECT_NE(nullptr, BakedOpState::tryConstruct(allocator, *snapshot, successOp))
+            << "successOp NOT rejected by clip, so should be constructed";
+    size_t successAllocSize = allocator.usedSize();
+    EXPECT_LE(64u, successAllocSize) << "relatively large alloc for non-rejected op";
+
+    RectOp rejectOp(Rect(30, 40, 100, 200), translate100x0, &clip, &paint);
+    EXPECT_EQ(nullptr, BakedOpState::tryConstruct(allocator, *snapshot, rejectOp))
+            << "rejectOp rejected by clip, so should not be constructed";
+
+    // NOTE: this relies on the clip having already been serialized by the op above
+    EXPECT_EQ(successAllocSize, allocator.usedSize()) << "no extra allocation used for rejected op";
 }
 
 TEST(BakedOpState, tryShadowOpConstruct) {
@@ -207,15 +204,16 @@ TEST(BakedOpState, tryShadowOpConstruct) {
         auto snapshot = TestUtils::makeSnapshot(Matrix4::identity(), Rect()); // Note: empty clip
         BakedOpState* bakedState = BakedOpState::tryShadowOpConstruct(allocator, *snapshot, (ShadowOp*)0x1234);
 
-        EXPECT_EQ(nullptr, bakedState); // rejected by clip, so not constructed
-        EXPECT_GT(8u, allocator.usedSize()); // no significant allocation space used for rejected op
+        EXPECT_EQ(nullptr, bakedState) << "op should be rejected by clip, so not constructed";
+        EXPECT_EQ(0u, allocator.usedSize()) << "no serialization, even for clip,"
+                "since op is quick rejected based on snapshot clip";
     }
     {
         auto snapshot = TestUtils::makeSnapshot(Matrix4::identity(), Rect(100, 200));
         BakedOpState* bakedState = BakedOpState::tryShadowOpConstruct(allocator, *snapshot, (ShadowOp*)0x1234);
 
-        ASSERT_NE(nullptr, bakedState); // NOT rejected by clip, so will be constructed
-        EXPECT_LE(64u, allocator.usedSize()); // relatively large alloc for non-rejected op
+        ASSERT_NE(nullptr, bakedState) << "NOT rejected by clip, so op should be constructed";
+        EXPECT_LE(64u, allocator.usedSize()) << "relatively large alloc for non-rejected op";
     }
 }