commit 95c9734bac87ca43e8bec0cdfcf69e1d3cda661b
author Mike Reed <reed@google.com> Mon Jul 26 15:33:30 2021 -0400
committer SkCQ <skcq-be@skia-corp.google.com.iam.gserviceaccount.com> Fri Aug 20 19:35:57 2021 +0000
tree ab949eb6cee8b5dc06b9eb4da1f9067cebf4cbb2
parent edad81e7c509823b5cf6f9b081793d17bc3a71ef

Reland "In SkCanvas destructor, discard (rather than blit) unbalanced layers"

This is a reland of 879b2f2e6ebd7fb465c0679e22c48a0cae58c61e

Now includes a test that demonstrates the bug found by Chrome's fuzzers,
and a different (safer) implementation.

Original change's description:
> In SkCanvas destructor, discard (rather than blit) unbalanced layers
>
> Bug: skia:12267
> Change-Id: I6808f62b2385a3466b1a93db905041a6529f58cb
> Reviewed-on: https://skia-review.googlesource.com/c/skia/+/433360
> Commit-Queue: Brian Osman <brianosman@google.com>
> Reviewed-by: Florin Malita <fmalita@google.com>
> Reviewed-by: Brian Salomon <bsalomon@google.com>
> Reviewed-by: Mike Reed <reed@google.com>

Bug: skia:12267
Change-Id: Ide7dc61b054761826faa5bca3eec6be2fc63c83a
Reviewed-on: https://skia-review.googlesource.com/c/skia/+/440977
Reviewed-by: Brian Salomon <bsalomon@google.com>
Commit-Queue: Brian Osman <brianosman@google.com>

include/core/SkCanvas.h

diff --git a/include/core/SkCanvas.h b/include/core/SkCanvas.h
index 34efd72..56bd689 100644
--- a/include/core/SkCanvas.h
+++ b/include/core/SkCanvas.h
@@ -2296,6 +2296,7 @@
         sk_sp<SkBaseDevice>  fDevice;
         sk_sp<SkImageFilter> fImageFilter; // applied to layer *before* being drawn by paint
         SkPaint              fPaint;
+        bool                 fDiscard;
 
         Layer(sk_sp<SkBaseDevice> device, sk_sp<SkImageFilter> imageFilter, const SkPaint& paint);
     };

src/core/SkCanvas.cpp

diff --git a/src/core/SkCanvas.cpp b/src/core/SkCanvas.cpp
index 39fd744..99e86d0 100644
--- a/src/core/SkCanvas.cpp
+++ b/src/core/SkCanvas.cpp
@@ -194,7 +194,10 @@
 SkCanvas::Layer::Layer(sk_sp<SkBaseDevice> device,
                        sk_sp<SkImageFilter> imageFilter,
                        const SkPaint& paint)
-        : fDevice(std::move(device)), fImageFilter(std::move(imageFilter)), fPaint(paint) {
+        : fDevice(std::move(device))
+        , fImageFilter(std::move(imageFilter))
+        , fPaint(paint)
+        , fDiscard(false) {
     SkASSERT(fDevice);
     // Any image filter should have been pulled out and stored in 'imageFilter' so that 'paint'
     // can be used as-is to draw the result of the filter to the dst device.
@@ -461,6 +464,19 @@
 #endif
 
 SkCanvas::~SkCanvas() {
+    // Mark all pending layers to be discarded during restore (rather than drawn)
+    SkDeque::Iter iter(fMCStack, SkDeque::Iter::kFront_IterStart);
+    for (;;) {
+        MCRec* rec = (MCRec*)iter.next();
+        if (!rec) {
+            break;
+        }
+        if (rec->fLayer) {
+            rec->fLayer->fDiscard = true;
+        }
+    }
+
+    // free up the contents of our deque
     this->restoreToCount(1);    // restore everything but the last
     this->internalRestore();    // restore the last, since we're going away
 
@@ -1153,7 +1169,7 @@
 
     // Draw the layer's device contents into the now-current older device. We can't call public
     // draw functions since we don't want to record them.
-    if (layer && !layer->fDevice->isNoPixelsDevice()) {
+    if (layer && !layer->fDevice->isNoPixelsDevice() && !layer->fDiscard) {
         layer->fDevice->setImmutable();
 
         // Don't go through AutoLayerForImageFilter since device draws are so closely tied to

tests/CanvasTest.cpp

diff --git a/tests/CanvasTest.cpp b/tests/CanvasTest.cpp
index 0f41c7d..65ad343 100644
--- a/tests/CanvasTest.cpp
+++ b/tests/CanvasTest.cpp
@@ -752,36 +752,48 @@
         const SkPMColor pmc = SkPreMultiplyColor(expected);
         for (int y = 0; y < pm.info().height(); ++y) {
             for (int x = 0; x < pm.info().width(); ++x) {
-                REPORTER_ASSERT(reporter, *pm.addr32(x, y) == pmc);
+                if (*pm.addr32(x, y) != pmc) {
+                    ERRORF(reporter, "check_pixels_failed");
+                    return;
+                }
             }
         }
     };
 
-    auto do_test = [&](bool doRestore) {
+    auto do_test = [&](int saveCount, int restoreCount) {
+        SkASSERT(restoreCount <= saveCount);
+
         auto surf = SkSurface::MakeRasterDirect(pm);
         auto canvas = surf->getCanvas();
 
         canvas->clear(SK_ColorRED);
         check_pixels(SK_ColorRED);
 
-        canvas->saveLayer(nullptr, nullptr);
+        for (int i = 0; i < saveCount; ++i) {
+            canvas->saveLayer(nullptr, nullptr);
+        }
+
         canvas->clear(SK_ColorBLUE);
-        // so far, we still expect to see the red
+        // so far, we still expect to see the red, since the blue was drawn in a layer
         check_pixels(SK_ColorRED);
 
-        if (doRestore) {
+        for (int i = 0; i < restoreCount; ++i) {
             canvas->restore();
         }
         // by returning, we are implicitly deleting the surface, and its associated canvas
     };
 
-    do_test(true);
+    do_test(1, 1);
     // since we called restore, we expect to see now see blue
     check_pixels(SK_ColorBLUE);
 
-    // Now we're repeat that, but delete the canvas before we restore it
-    do_test(false);
-    // *if* we restore the layers in the destructor, we expect to see blue, even though
-    // we didn't call restore() as a client.
-    check_pixels(SK_ColorBLUE);
+    // Now repeat that, but delete the canvas before we restore it
+    do_test(1, 0);
+    // We don't blit the unbalanced saveLayers, so we expect to see red (not the layer's blue)
+    check_pixels(SK_ColorRED);
+
+    // Finally, test with multiple unbalanced saveLayers. This led to a crash in an earlier
+    // implementation (crbug.com/1238731)
+    do_test(2, 0);
+    check_pixels(SK_ColorRED);
 }