change SkCanvasStack to take ownership of its subcanvases

Inspired by https://bugs.chromium.org/p/chromium/issues/detail?id=663959

GOLD_TRYBOT_URL= https://gold.skia.org/search?issue=4799

Change-Id: I69f7ac73386bb7ca96778e2fec4cb2757b982a52
Reviewed-on: https://skia-review.googlesource.com/4799
Commit-Queue: Mike Reed <reed@google.com>
Reviewed-by: Florin Malita <fmalita@chromium.org>

src/utils/SkCanvasStack.cpp

@@ -13,18 +13,19 @@ SkCanvasStack::~SkCanvasStack() {
     this->removeAll();
 }
 
-void SkCanvasStack::pushCanvas(SkCanvas* canvas, const SkIPoint& origin) {
+void SkCanvasStack::pushCanvas(std::unique_ptr<SkCanvas> canvas, const SkIPoint& origin) {
     if (canvas) {
         // compute the bounds of this canvas
         const SkIRect canvasBounds = SkIRect::MakeSize(canvas->getDeviceSize());
 
         // push the canvas onto the stack
-        this->INHERITED::addCanvas(canvas);
+        this->INHERITED::addCanvas(canvas.get());
 
         // push the canvas data onto the stack
         CanvasData* data = &fCanvasData.push_back();
         data->origin = origin;
         data->requiredClip.setRect(canvasBounds);
+        data->ownedCanvas = std::move(canvas);
 
         // subtract this region from the canvas objects already on the stack.
         // This ensures they do not draw into the space occupied by the layers
@@ -41,8 +42,8 @@ void SkCanvasStack::pushCanvas(SkCanvas* canvas, const SkIPoint& origin) {
 }
 
 void SkCanvasStack::removeAll() {
+    this->INHERITED::removeAll();   // call the baseclass *before* we actually delete the canvases
     fCanvasData.reset();
-    this->INHERITED::removeAll();
 }
 
 /**

src/utils/SkCanvasStack.h

@@ -11,12 +11,18 @@
 #include "SkNWayCanvas.h"
 #include "SkTArray.h"
 
+/**
+ *  Like NWayCanvas, in that it forwards all canvas methods to each sub-canvas that is "pushed".
+ *
+ *  Unlike NWayCanvas, this takes ownership of each subcanvas, and deletes them when this canvas
+ *  is deleted.
+ */
 class SkCanvasStack : public SkNWayCanvas {
 public:
     SkCanvasStack(int width, int height);
     virtual ~SkCanvasStack();
 
-    void pushCanvas(SkCanvas* canvas, const SkIPoint& origin);
+    void pushCanvas(std::unique_ptr<SkCanvas>, const SkIPoint& origin);
     void removeAll() override;
 
     /*
@@ -42,6 +48,7 @@ private:
     struct CanvasData {
         SkIPoint origin;
         SkRegion requiredClip;
+        std::unique_ptr<SkCanvas> ownedCanvas;
     };
 
     SkTArray<CanvasData> fCanvasData;

src/utils/SkCanvasStateUtils.cpp

@@ -337,8 +337,8 @@ std::unique_ptr<SkCanvas> SkCanvasStateUtils::MakeFromCanvasState(const SkCanvas
         if (!canvasLayer.get()) {
             return nullptr;
         }
-        canvas->pushCanvas(canvasLayer.get(), SkIPoint::Make(state_v1->layers[i].x,
+        canvas->pushCanvas(std::move(canvasLayer), SkIPoint::Make(state_v1->layers[i].x,
-                                                             state_v1->layers[i].y));
+                                                                  state_v1->layers[i].y));
     }
 
     return std::move(canvas);

tests/CanvasTest.cpp

@@ -734,3 +734,71 @@ DEF_TEST(DeferredCanvas, r) {
     canvas.restore();
 }
 
+///////////////////////////////////////////////////////////////////////////////////////////////////
+
+#include "SkCanvasStack.h"
+#include "SkNWayCanvas.h"
+
+// Subclass that takes a bool*, which it updates in its construct (true) and destructor (false)
+// to allow the caller to know how long the object is alive.
+class LifeLineCanvas : public SkCanvas {
+    bool*   fLifeLine;
+public:
+    LifeLineCanvas(int w, int h, bool* lifeline) : SkCanvas(w, h), fLifeLine(lifeline) {
+        *fLifeLine = true;
+    }
+    ~LifeLineCanvas() {
+        *fLifeLine = false;
+    }
+};
+
+// Check that NWayCanvas does NOT try to manage the lifetime of its sub-canvases
+DEF_TEST(NWayCanvas, r) {
+    const int w = 10;
+    const int h = 10;
+    bool life[2];
+    {
+        LifeLineCanvas c0(w, h, &life[0]);
+        REPORTER_ASSERT(r, life[0]);
+    }
+    REPORTER_ASSERT(r, !life[0]);
+
+
+    std::unique_ptr<SkCanvas> c0 = std::unique_ptr<SkCanvas>(new LifeLineCanvas(w, h, &life[0]));
+    std::unique_ptr<SkCanvas> c1 = std::unique_ptr<SkCanvas>(new LifeLineCanvas(w, h, &life[1]));
+    REPORTER_ASSERT(r, life[0]);
+    REPORTER_ASSERT(r, life[1]);
+
+    {
+        SkNWayCanvas nway(w, h);
+        nway.addCanvas(c0.get());
+        nway.addCanvas(c1.get());
+        REPORTER_ASSERT(r, life[0]);
+        REPORTER_ASSERT(r, life[1]);
+    }
+    // Now assert that the death of the nway has NOT also killed the sub-canvases
+    REPORTER_ASSERT(r, life[0]);
+    REPORTER_ASSERT(r, life[1]);
+}
+
+// Check that CanvasStack DOES manage the lifetime of its sub-canvases
+DEF_TEST(CanvasStack, r) {
+    const int w = 10;
+    const int h = 10;
+    bool life[2];
+    std::unique_ptr<SkCanvas> c0 = std::unique_ptr<SkCanvas>(new LifeLineCanvas(w, h, &life[0]));
+    std::unique_ptr<SkCanvas> c1 = std::unique_ptr<SkCanvas>(new LifeLineCanvas(w, h, &life[1]));
+    REPORTER_ASSERT(r, life[0]);
+    REPORTER_ASSERT(r, life[1]);
+
+    {
+        SkCanvasStack stack(w, h);
+        stack.pushCanvas(std::move(c0), {0,0});
+        stack.pushCanvas(std::move(c1), {0,0});
+        REPORTER_ASSERT(r, life[0]);
+        REPORTER_ASSERT(r, life[1]);
+    }
+    // Now assert that the death of the canvasstack has also killed the sub-canvases
+    REPORTER_ASSERT(r, !life[0]);
+    REPORTER_ASSERT(r, !life[1]);
+}