Bug 1401524 - Ensure SurfaceCache state coherency whenever we perform an operation that may discard surfaces. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Thu, 21 Sep 2017 16:56:38 -0400
changeset 668867 220a9b1bed87420e6ec9afae78bdd94771f9af3f
parent 668866 8b73ac23236691ba8bfc692c7b28f572981bca39
child 668868 8f4a95f9255cfdc6f9bf41cdcb11ba2110f27603
push id81146
push userbmo:topwu.tw@gmail.com
push dateFri, 22 Sep 2017 05:24:51 +0000
reviewerstnikkel
bugs1401524, 1370412, 1380649
milestone58.0a1
Bug 1401524 - Ensure SurfaceCache state coherency whenever we perform an operation that may discard surfaces. r=tnikkel There are a number of operations with the surface cache which may result in individual surfaces for a particular image cache to be removed. If an image cache is emptied, and we are in factor of 2 mode, we should reset it to the default mode, because we require at least one surface to be available to determine the native/ideal size. Additionally, if the cache is not locked, it should be removed entirely from the surface cache. We handle this correctly in methods such as Lookup and LookupBestMatch, but Prune and CollectSizeOfSurfaces can also cause this to happen, as recently done in bug 1370412 and bug 1380649.
image/SurfaceCache.cpp
--- a/image/SurfaceCache.cpp
+++ b/image/SurfaceCache.cpp
@@ -274,28 +274,17 @@ public:
 
   already_AddRefed<CachedSurface> Remove(NotNull<CachedSurface*> aSurface)
   {
     MOZ_ASSERT(mSurfaces.GetWeak(aSurface->GetSurfaceKey()),
         "Should not be removing a surface we don't have");
 
     RefPtr<CachedSurface> surface;
     mSurfaces.Remove(aSurface->GetSurfaceKey(), getter_AddRefs(surface));
-
-    if (IsEmpty() && mFactor2Mode) {
-      // The last surface for this cache was removed. This can happen if the
-      // surface was stored in a volatile buffer and got purged, or the surface
-      // expired from the cache. If the cache itself lingers for some reason
-      // (e.g. in the process of performing a lookup, the cache itself is
-      // locked), then we need to reset the factor of 2 state because it
-      // requires at least one surface present to get the native size
-      // information from the image.
-      mFactor2Mode = mFactor2Pruned = false;
-    }
-
+    AfterMaybeRemove();
     return surface.forget();
   }
 
   already_AddRefed<CachedSurface> Lookup(const SurfaceKey& aSurfaceKey,
                                          bool aForAccess)
   {
     RefPtr<CachedSurface> surface;
     mSurfaces.Get(aSurfaceKey, getter_AddRefs(surface));
@@ -532,28 +521,36 @@ public:
 
     // We have no surfaces that are not factor of 2 sized, so we can stop
     // pruning henceforth, because we avoid the insertion of new surfaces that
     // don't match our sizing set (unless the caller won't accept a
     // substitution.)
     if (!hasNotFactorSize) {
       mFactor2Pruned = true;
     }
+
+    // We should never leave factor of 2 mode due to pruning in of itself, but
+    // if we discarded surfaces due to the volatile buffers getting released,
+    // it is possible.
+    AfterMaybeRemove();
   }
 
   IntSize SuggestedSize(const IntSize& aSize) const
   {
     // When not in factor of 2 mode, we can always decode at the given size.
     if (!mFactor2Mode) {
       return aSize;
     }
 
     // We cannot enter factor of 2 mode unless we have a minimum number of
     // surfaces, and we should have left it if the cache was emptied.
-    MOZ_ASSERT(!IsEmpty());
+    if (MOZ_UNLIKELY(IsEmpty())) {
+      MOZ_ASSERT_UNREACHABLE("Should not be empty and in factor of 2 mode!");
+      return aSize;
+    }
 
     // This bit of awkwardness gets the largest native size of the image.
     auto iter = ConstIter();
     NotNull<CachedSurface*> firstSurface = WrapNotNull(iter.UserData());
     Image* image = static_cast<Image*>(firstSurface->GetImageKey());
     IntSize factorSize;
     if (NS_FAILED(image->GetWidth(&factorSize.width)) ||
         NS_FAILED(image->GetHeight(&factorSize.height)) ||
@@ -639,27 +636,43 @@ public:
 
       const IntSize& size = surface->GetSurfaceKey().Size();
       bool factor2Size = false;
       if (mFactor2Mode) {
         factor2Size = (size == SuggestedSize(size));
       }
       report.Add(surface, factor2Size);
     }
+
+    AfterMaybeRemove();
   }
 
   SurfaceTable::Iterator ConstIter() const
   {
     return mSurfaces.ConstIter();
   }
 
   void SetLocked(bool aLocked) { mLocked = aLocked; }
   bool IsLocked() const { return mLocked; }
 
 private:
+  void AfterMaybeRemove()
+  {
+    if (IsEmpty() && mFactor2Mode) {
+      // The last surface for this cache was removed. This can happen if the
+      // surface was stored in a volatile buffer and got purged, or the surface
+      // expired from the cache. If the cache itself lingers for some reason
+      // (e.g. in the process of performing a lookup, the cache itself is
+      // locked), then we need to reset the factor of 2 state because it
+      // requires at least one surface present to get the native size
+      // information from the image.
+      mFactor2Mode = mFactor2Pruned = false;
+    }
+  }
+
   SurfaceTable      mSurfaces;
 
   bool              mLocked;
 
   // True in "factor of 2" mode.
   bool              mFactor2Mode;
 
   // True if all non-factor of 2 surfaces have been removed from the cache. Note
@@ -811,23 +824,17 @@ public:
     // If we failed during StartTracking, we can skip this step.
     if (aStopTracking) {
       StopTracking(aSurface, /* aIsTracked */ true, aAutoLock);
     }
 
     // Individual surfaces must be freed outside the lock.
     mCachedSurfacesDiscard.AppendElement(cache->Remove(aSurface));
 
-    // Remove the per-image cache if it's unneeded now. Keep it if the image is
-    // locked, since the per-image cache is where we store that state. Note that
-    // we don't push it into mImageCachesDiscard because all of its surfaces
-    // have been removed, so it is safe to free while holding the lock.
-    if (cache->IsEmpty() && !cache->IsLocked()) {
-      mImageCaches.Remove(imageKey);
-    }
+    MaybeRemoveEmptyCache(imageKey, cache);
   }
 
   bool StartTracking(NotNull<CachedSurface*> aSurface,
                      const StaticMutexAutoLock& aAutoLock)
   {
     CostEntry costEntry = aSurface->GetCostEntry();
     MOZ_ASSERT(costEntry.GetCost() <= mAvailableCost,
                "Cost too large and the caller didn't catch it");
@@ -1088,16 +1095,18 @@ public:
       return;  // No cached surfaces for this image, so nothing to do.
     }
 
     cache->Prune([this, &aAutoLock](NotNull<CachedSurface*> aSurface) -> void {
       StopTracking(aSurface, /* aIsTracked */ true, aAutoLock);
       // Individual surfaces must be freed outside the lock.
       mCachedSurfacesDiscard.AppendElement(aSurface);
     });
+
+    MaybeRemoveEmptyCache(aImageKey, cache);
   }
 
   void DiscardAll(const StaticMutexAutoLock& aAutoLock)
   {
     // Remove in order of cost because mCosts is an array and the other data
     // structures are all hash tables. Note that locked surfaces are not
     // removed, since they aren't present in mCosts.
     while (!mCosts.IsEmpty()) {
@@ -1194,26 +1203,40 @@ public:
 
     // Report all surfaces in the per-image cache.
     cache->CollectSizeOfSurfaces(aCounters, aMallocSizeOf,
       [this, &aAutoLock](NotNull<CachedSurface*> aSurface) -> void {
       StopTracking(aSurface, /* aIsTracked */ true, aAutoLock);
       // Individual surfaces must be freed outside the lock.
       mCachedSurfacesDiscard.AppendElement(aSurface);
     });
+
+    MaybeRemoveEmptyCache(aImageKey, cache);
   }
 
 private:
   already_AddRefed<ImageSurfaceCache> GetImageCache(const ImageKey aImageKey)
   {
     RefPtr<ImageSurfaceCache> imageCache;
     mImageCaches.Get(aImageKey, getter_AddRefs(imageCache));
     return imageCache.forget();
   }
 
+  void MaybeRemoveEmptyCache(const ImageKey aImageKey,
+                             ImageSurfaceCache* aCache)
+  {
+    // Remove the per-image cache if it's unneeded now. Keep it if the image is
+    // locked, since the per-image cache is where we store that state. Note that
+    // we don't push it into mImageCachesDiscard because all of its surfaces
+    // have been removed, so it is safe to free while holding the lock.
+    if (aCache->IsEmpty() && !aCache->IsLocked()) {
+      mImageCaches.Remove(aImageKey);
+    }
+  }
+
   // This is similar to CanHold() except that it takes into account the costs of
   // locked surfaces. It's used internally in Insert(), but it's not exposed
   // publicly because we permit multithreaded access to the surface cache, which
   // means that the result would be meaningless: another thread could insert a
   // surface or lock an image at any time.
   bool CanHoldAfterDiscarding(const Cost aCost) const
   {
     return aCost <= mMaxCost - mLockedCost;