Bug 1118105 - Make SurfaceCache::Insert let you know if you try to insert a duplicate surface. r=dholbert

image/src/Decoder.cpp

@@ -368,13 +368,13 @@ Decoder::InternalAddFrame(uint32_t aFrameNum,
     return RawAccessFrameRef();
   }
 
-  bool succeeded =
+  InsertOutcome outcome =
     SurfaceCache::Insert(frame, ImageKey(&mImage),
                          RasterSurfaceKey(imageSize.ToIntSize(),
                                           aDecodeFlags,
                                           aFrameNum),
                          Lifetime::Persistent);
-  if (!succeeded) {
+  if (outcome != InsertOutcome::SUCCESS) {
     ref->Abort();
     return RawAccessFrameRef();
   }

image/src/SurfaceCache.cpp

@@ -304,19 +304,22 @@ public:
 
   Mutex& GetMutex() { return mMutex; }
 
-  bool Insert(imgFrame*         aSurface,
-              const Cost        aCost,
-              const ImageKey    aImageKey,
-              const SurfaceKey& aSurfaceKey,
-              Lifetime          aLifetime)
+  InsertOutcome Insert(imgFrame*         aSurface,
+                       const Cost        aCost,
+                       const ImageKey    aImageKey,
+                       const SurfaceKey& aSurfaceKey,
+                       Lifetime          aLifetime)
   {
-    MOZ_ASSERT(!Lookup(aImageKey, aSurfaceKey),
-               "Inserting a duplicate surface into the SurfaceCache");
+    // If this is a duplicate surface, refuse to replace the original.
+    if (MOZ_UNLIKELY(Lookup(aImageKey, aSurfaceKey))) {
+      return InsertOutcome::FAILURE_ALREADY_PRESENT;
+    }
 
     // If this is bigger than we can hold after discarding everything we can,
     // refuse to cache it.
-    if (!CanHoldAfterDiscarding(aCost))
-      return false;
+    if (MOZ_UNLIKELY(!CanHoldAfterDiscarding(aCost))) {
+      return InsertOutcome::FAILURE;
+    }
 
     // Remove elements in order of cost until we can fit this in the cache. Note
     // that locked surfaces aren't in mCosts, so we never remove them here.
@@ -341,7 +344,7 @@ public:
     if (cache->IsLocked() && aLifetime == Lifetime::Persistent) {
       surface->SetLocked(true);
       if (!surface->IsLocked()) {
-        return false;
+        return InsertOutcome::FAILURE;
       }
     }
 
@@ -350,7 +353,7 @@ public:
     cache->Insert(aSurfaceKey, surface);
     StartTracking(surface);
 
-    return true;
+    return InsertOutcome::SUCCESS;
   }
 
   void Remove(CachedSurface* aSurface)
@@ -795,14 +798,14 @@ SurfaceCache::Lookup(const ImageKey    aImageKey,
   return sInstance->Lookup(aImageKey, aSurfaceKey);
 }
 
-/* static */ bool
+/* static */ InsertOutcome
 SurfaceCache::Insert(imgFrame*         aSurface,
                      const ImageKey    aImageKey,
                      const SurfaceKey& aSurfaceKey,
                      Lifetime          aLifetime)
 {
   if (!sInstance) {
-    return false;
+    return InsertOutcome::FAILURE;
   }
 
   MutexAutoLock lock(sInstance->GetMutex());

image/src/SurfaceCache.h

@@ -114,6 +114,12 @@ MOZ_BEGIN_ENUM_CLASS(Lifetime, uint8_t)
   Persistent
 MOZ_END_ENUM_CLASS(Lifetime)
 
+MOZ_BEGIN_ENUM_CLASS(InsertOutcome, uint8_t)
+  SUCCESS,                 // Success (but see Insert documentation).
+  FAILURE,                 // Couldn't insert (e.g., for capacity reasons).
+  FAILURE_ALREADY_PRESENT  // A surface with the same key is already present.
+MOZ_END_ENUM_CLASS(InsertOutcome)
+
 /**
  * SurfaceCache is an imagelib-global service that allows caching of temporary
  * surfaces. Surfaces normally expire from the cache automatically if they go
@@ -173,9 +179,8 @@ struct SurfaceCache
                                  const SurfaceKey& aSurfaceKey);
 
   /**
-   * Insert a surface into the cache. It is an error to call this function
-   * without first calling Lookup to verify that the surface is not already in
-   * the cache.
+   * Insert a surface into the cache. If a surface with the same ImageKey and
+   * SurfaceKey is already in the cache, Insert returns FAILURE_ALREADY_PRESENT.
    *
    * Each surface in the cache has a lifetime, either Transient or Persistent.
    * Transient surfaces can expire from the cache at any time. Persistent
@@ -187,13 +192,14 @@ struct SurfaceCache
    * LockImage() gets called.
    *
    * If a surface cannot be rematerialized, it may be important to know whether
-   * it was inserted into the cache successfully. Insert() returns false if it
+   * it was inserted into the cache successfully. Insert() returns FAILURE if it
    * failed to insert the surface, which could happen because of capacity
    * reasons, or because it was already freed by the OS. If you aren't inserting
    * a surface with persistent lifetime, or if the surface isn't associated with
-   * a locked image, the return value is useless: the surface might expire
-   * immediately after being inserted, even though Insert() returned true. Thus,
-   * most callers do not need to check the return value.
+   * a locked image, checking for SUCCESS or FAILURE is useless: the surface
+   * might expire immediately after being inserted, even though Insert()
+   * returned SUCCESS. Thus, many callers do not need to check the result of
+   * Insert() at all.
    *
    * @param aTarget      The new surface (wrapped in an imgFrame) to insert into
    *                     the cache.
@@ -202,14 +208,18 @@ struct SurfaceCache
    * @param aLifetime    Whether this is a transient surface that can always be
    *                     allowed to expire, or a persistent surface that
    *                     shouldn't expire if the image is locked.
-   * @return false if the surface could not be inserted. Only check this if
-   *         inserting a persistent surface associated with a locked image (see
-   *         above for more information).
+   * @return SUCCESS if the surface was inserted successfully. (But see above
+   *           for more information about when you should check this.)
+   *         FAILURE if the surface could not be inserted, e.g. for capacity
+   *           reasons. (But see above for more information about when you
+   *           should check this.)
+   *         FAILURE_ALREADY_PRESENT if a surface with the same ImageKey and
+   *           SurfaceKey already exists in the cache.
    */
-  static bool Insert(imgFrame*         aSurface,
-                     const ImageKey    aImageKey,
-                     const SurfaceKey& aSurfaceKey,
-                     Lifetime          aLifetime);
+  static InsertOutcome Insert(imgFrame*         aSurface,
+                              const ImageKey    aImageKey,
+                              const SurfaceKey& aSurfaceKey,
+                              Lifetime          aLifetime);
 
   /**
    * Checks if a surface of a given size could possibly be stored in the cache.