Change the meaning of GrBudgetedType::kUnbudgetedUncacheable.

kUnbudgetedCacheable now means that the resource is never purged
until its unique key is removed.

This fixes an issue where a cached texture for a promise image
might get purged by cache pressure. This in turn could cause
Skia to call the promise image's Fulfill proc multiple times with
no intervening Release calls. The balancing Release calls would
occur, but the policy is that each Fulfill should be balanced by
Release *before* another Fulfill.

Update/add unit tests.

Bug: chromium:922851
Change-Id: I6411e413b3104721ca4bb6e7f07b3b73d14cbcf9
Reviewed-on: https://skia-review.googlesource.com/c/186361
Commit-Queue: Brian Salomon <bsalomon@google.com>
Reviewed-by: Robert Phillips <robertphillips@google.com>

14 files changed, 95 insertions, 67 deletions

diff --git a/src/gpu/GrContext.cpp b/src/gpu/GrContext.cpp
index 449fe606df..d122a4d891 100644
--- a/src/gpu/GrContext.cpp
+++ b/src/gpu/GrContext.cpp
@@ -261,7 +261,8 @@ void GrContext::abandonContext() {
 
 bool GrContext::abandoned() const {
     ASSERT_SINGLE_OWNER
-    return fDrawingManager->wasAbandoned();
+    // If called from ~GrContext(), the drawing manager may already be gone.
+    return !fDrawingManager || fDrawingManager->wasAbandoned();
 }
 
 void GrContext::releaseResourcesAndAbandonContext() {
diff --git a/src/gpu/GrGpuResource.cpp b/src/gpu/GrGpuResource.cpp
index c9224be09c..02cc6171f6 100644
--- a/src/gpu/GrGpuResource.cpp
+++ b/src/gpu/GrGpuResource.cpp
@@ -93,6 +93,17 @@ void GrGpuResource::dumpMemoryStatisticsPriv(SkTraceMemoryDump* traceMemoryDump,
     this->setMemoryBacking(traceMemoryDump, resourceName);
 }
 
+bool GrGpuResource::isPurgeable() const {
+    // Resources in the kUnbudgetedCacheable state are never purgeable when they have a unique
+    // key. The key must be removed/invalidated to make them purgeable.
+    return !this->hasRefOrPendingIO() &&
+           !(fBudgetedType == GrBudgetedType::kUnbudgetedCacheable && fUniqueKey.isValid());
+}
+
+bool GrGpuResource::hasRefOrPendingIO() const {
+    return this->internalHasRef() || this->internalHasPendingIO();
+}
+
 SkString GrGpuResource::getResourceName() const {
     // Dump resource as "skia/gpu_resources/resource_#".
     SkString resourceName("skia/gpu_resources/resource_");
@@ -145,6 +156,8 @@ void GrGpuResource::setUniqueKey(const GrUniqueKey& key) {
 }
 
 void GrGpuResource::notifyAllCntsAreZero(CntType lastCntTypeToReachZero) const {
+    GrGpuResource* mutableThis = const_cast<GrGpuResource*>(this);
+    mutableThis->removedLastRefOrPendingIO();
     if (this->wasDestroyed()) {
         // We've already been removed from the cache. Goodbye cruel world!
         delete this;
@@ -154,7 +167,6 @@ void GrGpuResource::notifyAllCntsAreZero(CntType lastCntTypeToReachZero) const {
     // We should have already handled this fully in notifyRefCntIsZero().
     SkASSERT(kRef_CntType != lastCntTypeToReachZero);
 
-    GrGpuResource* mutableThis = const_cast<GrGpuResource*>(this);
     static const uint32_t kFlag =
         GrResourceCache::ResourceAccess::kAllCntsReachedZero_RefNotificationFlag;
     get_resource_cache(fGpu)->resourceAccess().notifyCntReachedZero(mutableThis, kFlag);
@@ -170,6 +182,7 @@ bool GrGpuResource::notifyRefCountIsZero() const {
     uint32_t flags = GrResourceCache::ResourceAccess::kRefCntReachedZero_RefNotificationFlag;
     if (!this->internalHasPendingIO()) {
         flags |= GrResourceCache::ResourceAccess::kAllCntsReachedZero_RefNotificationFlag;
+        mutableThis->removedLastRefOrPendingIO();
     }
     get_resource_cache(fGpu)->resourceAccess().notifyCntReachedZero(mutableThis, flags);
 
diff --git a/src/gpu/GrGpuResourceCacheAccess.h b/src/gpu/GrGpuResourceCacheAccess.h
index 3765c6e82e..e80b9b5451 100644
--- a/src/gpu/GrGpuResourceCacheAccess.h
+++ b/src/gpu/GrGpuResourceCacheAccess.h
@@ -30,17 +30,11 @@ private:
     }
 
     /**
-     * Called by GrResourceCache when a resource becomes purgeable regardless of whether the cache
-     * has decided to keep the resource ot purge it immediately.
-     */
-    void becamePurgeable() { fResource->becamePurgeable(); }
-
-    /**
      * Called by the cache to delete the resource under normal circumstances.
      */
     void release() {
         fResource->release();
-        if (fResource->isPurgeable()) {
+        if (!fResource->hasRefOrPendingIO()) {
             delete fResource;
         }
     }
@@ -50,7 +44,7 @@ private:
      */
     void abandon() {
         fResource->abandon();
-        if (fResource->isPurgeable()) {
+        if (!fResource->hasRefOrPendingIO()) {
             delete fResource;
         }
     }
diff --git a/src/gpu/GrGpuResourcePriv.h b/src/gpu/GrGpuResourcePriv.h
index 75b3531b7f..30f686cd5d 100644
--- a/src/gpu/GrGpuResourcePriv.h
+++ b/src/gpu/GrGpuResourcePriv.h
@@ -72,6 +72,8 @@ public:
 
     bool isPurgeable() const { return fResource->isPurgeable(); }
 
+    bool hasRefOrPendingIO() const { return fResource->hasRefOrPendingIO(); }
+
 protected:
     ResourcePriv(GrGpuResource* resource) : fResource(resource) {   }
     ResourcePriv(const ResourcePriv& that) : fResource(that.fResource) {}
diff --git a/src/gpu/GrProxyProvider.h b/src/gpu/GrProxyProvider.h
index f84cdfc40b..f093416949 100644
--- a/src/gpu/GrProxyProvider.h
+++ b/src/gpu/GrProxyProvider.h
@@ -251,6 +251,7 @@ public:
      */
     sk_sp<GrTextureProxy> testingOnly_createInstantiatedProxy(const GrSurfaceDesc&, GrSurfaceOrigin,
                                                               SkBackingFit, SkBudgeted);
+    sk_sp<GrTextureProxy> testingOnly_createWrapped(sk_sp<GrTexture>, GrSurfaceOrigin);
 
 private:
     friend class GrAHardwareBufferImageGenerator; // for createWrapped
diff --git a/src/gpu/GrResourceCache.cpp b/src/gpu/GrResourceCache.cpp
index 634dac7d2e..dfb1df4344 100644
--- a/src/gpu/GrResourceCache.cpp
+++ b/src/gpu/GrResourceCache.cpp
@@ -187,9 +187,6 @@ void GrResourceCache::abandonAll() {
     while (fNonpurgeableResources.count()) {
         GrGpuResource* back = *(fNonpurgeableResources.end() - 1);
         SkASSERT(!back->wasDestroyed());
-        // If these resources we're relying on a purgeable notification to release something, notify
-        // them now. They aren't in the purgeable queue but they're getting purged anyway.
-        back->cacheAccess().becamePurgeable();
         back->cacheAccess().abandon();
     }
 
@@ -230,9 +227,6 @@ void GrResourceCache::releaseAll() {
     while (fNonpurgeableResources.count()) {
         GrGpuResource* back = *(fNonpurgeableResources.end() - 1);
         SkASSERT(!back->wasDestroyed());
-        // If these resources we're relying on a purgeable notification to release something, notify
-        // them now. They aren't in the purgeable queue but they're getting purged anyway.
-        back->cacheAccess().becamePurgeable();
         back->cacheAccess().release();
     }
 
@@ -319,11 +313,14 @@ void GrResourceCache::removeUniqueKey(GrGpuResource* resource) {
         fUniqueHash.remove(resource->getUniqueKey());
     }
     resource->cacheAccess().removeUniqueKey();
-
     if (resource->resourcePriv().getScratchKey().isValid()) {
         fScratchMap.insert(resource->resourcePriv().getScratchKey(), resource);
     }
 
+    // Removing a unique key from a kUnbudgetedCacheable resource would make the resource
+    // require purging. However, the resource must be ref'ed to get here and therefore can't
+    // be purgeable. We'll purge it when the refs reach zero.
+    SkASSERT(!resource->resourcePriv().isPurgeable());
     this->validate();
 }
 
@@ -340,7 +337,8 @@ void GrResourceCache::changeUniqueKey(GrGpuResource* resource, const GrUniqueKey
                 old->resourcePriv().isPurgeable()) {
                 old->cacheAccess().release();
             } else {
-                this->removeUniqueKey(old);
+                // removeUniqueKey expects an external owner of the resource.
+                this->removeUniqueKey(sk_ref_sp(old).get());
             }
         }
         SkASSERT(nullptr == fUniqueHash.find(newKey));
@@ -412,7 +410,11 @@ void GrResourceCache::notifyCntReachedZero(GrGpuResource* resource, uint32_t fla
         return;
     }
 
-    SkASSERT(resource->resourcePriv().isPurgeable());
+    if (!resource->resourcePriv().isPurgeable()) {
+        this->validate();
+        return;
+    }
+
     this->removeFromNonpurgeableArray(resource);
     fPurgeableQueue.insert(resource);
     resource->cacheAccess().setTimeWhenResourceBecomePurgeable();
@@ -420,35 +422,33 @@ void GrResourceCache::notifyCntReachedZero(GrGpuResource* resource, uint32_t fla
 
     bool hasUniqueKey = resource->getUniqueKey().isValid();
 
-    {
-        SkScopeExit notifyPurgeable([resource] { resource->cacheAccess().becamePurgeable(); });
-
-        auto budgetedType = resource->resourcePriv().budgetedType();
-        if (budgetedType == GrBudgetedType::kBudgeted) {
-            // Purge the resource immediately if we're over budget
-            // Also purge if the resource has neither a valid scratch key nor a unique key.
-            bool hasKey = resource->resourcePriv().getScratchKey().isValid() || hasUniqueKey;
-            if (!this->overBudget() && hasKey) {
-                return;
-            }
-        } else {
-            // We keep unbudgeted resources with a unique key in the purgable queue of the cache so
-            // they can be reused again by the image connected to the unique key.
-            if (hasUniqueKey && budgetedType == GrBudgetedType::kUnbudgetedCacheable) {
+    GrBudgetedType budgetedType = resource->resourcePriv().budgetedType();
+
+    if (budgetedType == GrBudgetedType::kBudgeted) {
+        // Purge the resource immediately if we're over budget
+        // Also purge if the resource has neither a valid scratch key nor a unique key.
+        bool hasKey = resource->resourcePriv().getScratchKey().isValid() || hasUniqueKey;
+        if (!this->overBudget() && hasKey) {
+            return;
+        }
+    } else {
+        // We keep unbudgeted resources with a unique key in the purgeable queue of the cache so
+        // they can be reused again by the image connected to the unique key.
+        if (hasUniqueKey && budgetedType == GrBudgetedType::kUnbudgetedCacheable) {
+            return;
+        }
+        // Check whether this resource could still be used as a scratch resource.
+        if (!resource->resourcePriv().refsWrappedObjects() &&
+            resource->resourcePriv().getScratchKey().isValid()) {
+            // We won't purge an existing resource to make room for this one.
+            if (fBudgetedCount < fMaxCount &&
+                fBudgetedBytes + resource->gpuMemorySize() <= fMaxBytes) {
+                resource->resourcePriv().makeBudgeted();
                 return;
             }
-            // Check whether this resource could still be used as a scratch resource.
-            if (!resource->resourcePriv().refsWrappedObjects() &&
-                resource->resourcePriv().getScratchKey().isValid()) {
-                // We won't purge an existing resource to make room for this one.
-                if (fBudgetedCount < fMaxCount &&
-                    fBudgetedBytes + resource->gpuMemorySize() <= fMaxBytes) {
-                    resource->resourcePriv().makeBudgeted();
-                    return;
-                }
-            }
         }
     }
+
     SkDEBUGCODE(int beforeCount = this->getResourceCount();)
     resource->cacheAccess().release();
     // We should at least free this resource, perhaps dependent resources as well.
@@ -462,8 +462,12 @@ void GrResourceCache::didChangeBudgetStatus(GrGpuResource* resource) {
     SkASSERT(this->isInCache(resource));
 
     size_t size = resource->gpuMemorySize();
-
-    if (GrBudgetedType::kBudgeted == resource->resourcePriv().budgetedType()) {
+    // Changing from BudgetedType::kUnbudgetedCacheable to another budgeted type could make
+    // resource become purgeable. However, we should never allow that transition. Wrapped
+    // resources are the only resources that can be in that state and they aren't allowed to
+    // transition from one budgeted state to another.
+    SkDEBUGCODE(bool wasPurgeable = resource->resourcePriv().isPurgeable());
+    if (resource->resourcePriv().budgetedType() == GrBudgetedType::kBudgeted) {
         ++fBudgetedCount;
         fBudgetedBytes += size;
 #if GR_CACHE_STATS
@@ -472,9 +476,11 @@ void GrResourceCache::didChangeBudgetStatus(GrGpuResource* resource) {
 #endif
         this->purgeAsNeeded();
     } else {
+        SkASSERT(resource->resourcePriv().budgetedType() != GrBudgetedType::kUnbudgetedCacheable);
         --fBudgetedCount;
         fBudgetedBytes -= size;
     }
+    SkASSERT(wasPurgeable == resource->resourcePriv().isPurgeable());
     TRACE_COUNTER2("skia.gpu.cache", "skia budget", "used",
                    fBudgetedBytes, "free", fMaxBytes - fBudgetedBytes);
 
diff --git a/src/gpu/gl/GrGLTexture.h b/src/gpu/gl/GrGLTexture.h
index e8ca83cee2..9383f11089 100644
--- a/src/gpu/gl/GrGLTexture.h
+++ b/src/gpu/gl/GrGLTexture.h
@@ -137,7 +137,7 @@ private:
         fReleaseHelper.reset();
     }
 
-    void becamePurgeable() override {
+    void removedLastRefOrPendingIO() override {
         if (fIdleProc) {
             fIdleProc(fIdleProcContext);
             fIdleProc = nullptr;
diff --git a/src/gpu/mock/GrMockTexture.h b/src/gpu/mock/GrMockTexture.h
index 26741be1d9..c44809abda 100644
--- a/src/gpu/mock/GrMockTexture.h
+++ b/src/gpu/mock/GrMockTexture.h
@@ -77,7 +77,7 @@ protected:
 
     // protected so that GrMockTextureRenderTarget can call this to avoid "inheritance via
     // dominance" warning.
-    void becamePurgeable() override {
+    void removedLastRefOrPendingIO() override {
         if (fIdleProc) {
             fIdleProc(fIdleProcContext);
             fIdleProc = nullptr;
@@ -196,7 +196,7 @@ private:
     }
 
     // We implement this to avoid the inheritance via dominance warning.
-    void becamePurgeable() override { GrMockTexture::becamePurgeable(); }
+    void removedLastRefOrPendingIO() override { GrMockTexture::removedLastRefOrPendingIO(); }
 
     size_t onGpuMemorySize() const override {
         int numColorSamples = this->numColorSamples();
diff --git a/src/gpu/mtl/GrMtlTexture.h b/src/gpu/mtl/GrMtlTexture.h
index 87599eab15..a01bfa49e3 100644
--- a/src/gpu/mtl/GrMtlTexture.h
+++ b/src/gpu/mtl/GrMtlTexture.h
@@ -76,7 +76,7 @@ private:
         fReleaseHelper.reset();
     }
 
-    void becamePurgeable() override {
+    void removedLastRefOrPendingIO() override {
         if (fIdleProc) {
             fIdleProc(fIdleProcContext);
             fIdleProc = nullptr;
diff --git a/src/gpu/vk/GrVkImage.cpp b/src/gpu/vk/GrVkImage.cpp
index ead8d8dd44..755ccabcc6 100644
--- a/src/gpu/vk/GrVkImage.cpp
+++ b/src/gpu/vk/GrVkImage.cpp
@@ -262,7 +262,7 @@ void GrVkImage::Resource::notifyRemovedFromCommandBuffer() const {
     if (--fNumCommandBufferOwners || !fIdleProc) {
         return;
     }
-    if (fOwningTexture && !fOwningTexture->resourcePriv().isPurgeable()) {
+    if (fOwningTexture && fOwningTexture->resourcePriv().hasRefOrPendingIO()) {
         return;
     }
     fIdleProc(fIdleProcContext);
diff --git a/src/gpu/vk/GrVkImage.h b/src/gpu/vk/GrVkImage.h
index 0ce223c571..dab3a756a1 100644
--- a/src/gpu/vk/GrVkImage.h
+++ b/src/gpu/vk/GrVkImage.h
@@ -139,6 +139,7 @@ public:
 protected:
     void releaseImage(GrVkGpu* gpu);
     void abandonImage();
+    bool hasResource() const { return fResource; }
 
     GrVkImageInfo          fInfo;
     uint32_t               fInitialQueueFamily;
diff --git a/src/gpu/vk/GrVkTexture.cpp b/src/gpu/vk/GrVkTexture.cpp
index bdd8db4325..6e8fe67fd1 100644
--- a/src/gpu/vk/GrVkTexture.cpp
+++ b/src/gpu/vk/GrVkTexture.cpp
@@ -120,10 +120,13 @@ GrVkTexture::~GrVkTexture() {
 }
 
 void GrVkTexture::onRelease() {
-    // When there is an idle proc, the Resource will call the proc in releaseImage() so
-    // we clear it here.
-    fIdleProc = nullptr;
-    fIdleProcContext = nullptr;
+    // We're about to be severed from our GrVkResource. If there is an idle proc we have to decide
+    // who will handle it. If the resource is still tied to a command buffer we let it handle it.
+    // Otherwise, we handle it.
+    if (this->hasResource() && this->resource()->isOwnedByCommandBuffer()) {
+        fIdleProc = nullptr;
+        fIdleProcContext = nullptr;
+    }
 
     // we create this and don't hand it off, so we should always destroy it
     if (fTextureView) {
@@ -137,10 +140,14 @@ void GrVkTexture::onRelease() {
 }
 
 void GrVkTexture::onAbandon() {
-    // When there is an idle proc, the Resource will call the proc in abandonImage() so
-    // we clear it here.
-    fIdleProc = nullptr;
-    fIdleProcContext = nullptr;
+    // We're about to be severed from our GrVkResource. If there is an idle proc we have to decide
+    // who will handle it. If the resource is still tied to a command buffer we let it handle it.
+    // Otherwise, we handle it.
+    if (this->hasResource() && this->resource()->isOwnedByCommandBuffer()) {
+        fIdleProc = nullptr;
+        fIdleProcContext = nullptr;
+    }
+
     // we create this and don't hand it off, so we should always destroy it
     if (fTextureView) {
         fTextureView->unrefAndAbandon();
@@ -172,19 +179,20 @@ void GrVkTexture::setIdleProc(IdleProc proc, void* context) {
     }
 }
 
-void GrVkTexture::becamePurgeable() {
+void GrVkTexture::removedLastRefOrPendingIO() {
     if (!fIdleProc) {
         return;
     }
     // This is called when the GrTexture is purgeable. However, we need to check whether the
     // Resource is still owned by any command buffers. If it is then it will call the proc.
-    auto* resource = this->resource();
-    SkASSERT(resource);
-    if (resource->isOwnedByCommandBuffer()) {
+    auto* resource = this->hasResource() ? this->resource() : nullptr;
+    if (resource && resource->isOwnedByCommandBuffer()) {
         return;
     }
     fIdleProc(fIdleProcContext);
     fIdleProc = nullptr;
     fIdleProcContext = nullptr;
-    resource->setIdleProc(nullptr, nullptr, nullptr);
+    if (resource) {
+        resource->setIdleProc(nullptr, nullptr, nullptr);
+    }
 }
diff --git a/src/gpu/vk/GrVkTexture.h b/src/gpu/vk/GrVkTexture.h
index 9005b42356..97d7f06690 100644
--- a/src/gpu/vk/GrVkTexture.h
+++ b/src/gpu/vk/GrVkTexture.h
@@ -69,7 +69,7 @@ private:
                 const GrVkImageView*, GrMipMapsStatus, GrBackendObjectOwnership, GrWrapCacheable,
                 GrIOType);
 
-    void becamePurgeable() override;
+    void removedLastRefOrPendingIO() override;
 
     const GrVkImageView* fTextureView;
     GrTexture::IdleProc* fIdleProc = nullptr;
diff --git a/src/gpu/vk/GrVkTextureRenderTarget.h b/src/gpu/vk/GrVkTextureRenderTarget.h
index 1538287883..ecd634319a 100644
--- a/src/gpu/vk/GrVkTextureRenderTarget.h
+++ b/src/gpu/vk/GrVkTextureRenderTarget.h
@@ -42,13 +42,15 @@ public:
 
 protected:
     void onAbandon() override {
-        GrVkRenderTarget::onAbandon();
+        // In order to correctly handle calling texture idle procs, GrVkTexture must go first.
         GrVkTexture::onAbandon();
+        GrVkRenderTarget::onAbandon();
     }
 
     void onRelease() override {
-        GrVkRenderTarget::onRelease();
+        // In order to correctly handle calling texture idle procs, GrVkTexture must go first.
         GrVkTexture::onRelease();
+        GrVkRenderTarget::onRelease();
     }
 
 private: