Bug 1146663 (Part 2) - Remove the concept of lifetimes from the SurfaceCache. r=dholbert,a=lizzard
authorSeth Fowler <mark.seth.fowler@gmail.com>
Sat, 19 Sep 2015 00:59:03 -0700
changeset 298073 dfb869dc6a74c304a0e7b012e08e2f6f3221faad
parent 298072 b1e93301c3cfea89f7e38bdcb1f3244200af784c
child 298074 7ef75c70593d0f4670a3db9e5add74800c9631f2
push id962
push userjlund@mozilla.com
push dateFri, 04 Dec 2015 23:28:54 +0000
treeherdermozilla-release@23a2d286e80f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert, lizzard
bugs1146663
milestone43.0a2
Bug 1146663 (Part 2) - Remove the concept of lifetimes from the SurfaceCache. r=dholbert,a=lizzard
image/Decoder.cpp
image/Image.h
image/SurfaceCache.cpp
image/SurfaceCache.h
image/VectorImage.cpp
--- a/image/Decoder.cpp
+++ b/image/Decoder.cpp
@@ -319,18 +319,17 @@ Decoder::AllocateFrameInternal(uint32_t 
     return RawAccessFrameRef();
   }
 
   if (ShouldUseSurfaceCache()) {
     InsertOutcome outcome =
       SurfaceCache::Insert(frame, ImageKey(mImage.get()),
                            RasterSurfaceKey(aTargetSize,
                                             mSurfaceFlags,
-                                            aFrameNum),
-                           Lifetime::Persistent);
+                                            aFrameNum));
     if (outcome == InsertOutcome::FAILURE) {
       // We couldn't insert the surface, almost certainly due to low memory. We
       // treat this as a permanent error to help the system recover; otherwise,
       // we might just end up attempting to decode this image again immediately.
       ref->Abort();
       return RawAccessFrameRef();
     } else if (outcome == InsertOutcome::FAILURE_ALREADY_PRESENT) {
       // Another decoder beat us to decoding this frame. We abort this decoder
--- a/image/Image.h
+++ b/image/Image.h
@@ -212,18 +212,17 @@ public:
    * @param aLastPart Whether this is the final part of the underlying request.
    */
   virtual nsresult OnImageDataComplete(nsIRequest* aRequest,
                                        nsISupports* aContext,
                                        nsresult aStatus,
                                        bool aLastPart) = 0;
 
   /**
-   * Called when the SurfaceCache discards a persistent surface belonging to
-   * this image.
+   * Called when the SurfaceCache discards a surface belonging to this image.
    */
   virtual void OnSurfaceDiscarded() = 0;
 
   virtual void SetInnerWindowID(uint64_t aInnerWindowId) = 0;
   virtual uint64_t InnerWindowID() const = 0;
 
   virtual bool HasError() = 0;
   virtual void SetHasError() = 0;
--- a/image/SurfaceCache.cpp
+++ b/image/SurfaceCache.cpp
@@ -130,27 +130,24 @@ class CachedSurface
   ~CachedSurface() { }
 public:
   MOZ_DECLARE_REFCOUNTED_TYPENAME(CachedSurface)
   NS_INLINE_DECL_THREADSAFE_REFCOUNTING(CachedSurface)
 
   CachedSurface(imgFrame*          aSurface,
                 const Cost         aCost,
                 const ImageKey     aImageKey,
-                const SurfaceKey&  aSurfaceKey,
-                const Lifetime     aLifetime)
+                const SurfaceKey&  aSurfaceKey)
     : mSurface(aSurface)
     , mCost(aCost)
     , mImageKey(aImageKey)
     , mSurfaceKey(aSurfaceKey)
-    , mLifetime(aLifetime)
   {
-    MOZ_ASSERT(!IsPlaceholder() ||
-               (mCost == sPlaceholderCost && mLifetime == Lifetime::Transient),
-               "Placeholder should have trivial cost and transient lifetime");
+    MOZ_ASSERT(!IsPlaceholder() || mCost == sPlaceholderCost,
+               "Placeholder should have trivial cost");
     MOZ_ASSERT(mImageKey, "Must have a valid image key");
   }
 
   DrawableFrameRef DrawableRef() const
   {
     if (MOZ_UNLIKELY(IsPlaceholder())) {
       MOZ_ASSERT_UNREACHABLE("Shouldn't call DrawableRef() on a placeholder");
       return DrawableFrameRef();
@@ -160,34 +157,33 @@ public:
   }
 
   void SetLocked(bool aLocked)
   {
     if (IsPlaceholder()) {
       return;  // Can't lock a placeholder.
     }
 
-    if (aLocked && mLifetime == Lifetime::Persistent) {
+    if (aLocked) {
       // This may fail, and that's OK. We make no guarantees about whether
       // locking is successful if you call SurfaceCache::LockImage() after
       // SurfaceCache::Insert().
       mDrawableRef = mSurface->DrawableRef();
     } else {
       mDrawableRef.reset();
     }
   }
 
   bool IsPlaceholder() const { return !bool(mSurface); }
   bool IsLocked() const { return bool(mDrawableRef); }
 
   ImageKey GetImageKey() const { return mImageKey; }
   SurfaceKey GetSurfaceKey() const { return mSurfaceKey; }
   CostEntry GetCostEntry() { return image::CostEntry(this, mCost); }
   nsExpirationState* GetExpirationState() { return &mExpirationState; }
-  Lifetime GetLifetime() const { return mLifetime; }
 
   bool IsDecoded() const
   {
     return !IsPlaceholder() && mSurface->IsImageComplete();
   }
 
   // A helper type used by SurfaceCacheImpl::CollectSizeOfSurfaces.
   struct MOZ_STACK_CLASS SurfaceMemoryReport
@@ -225,17 +221,16 @@ public:
 
 private:
   nsExpirationState  mExpirationState;
   nsRefPtr<imgFrame> mSurface;
   DrawableFrameRef   mDrawableRef;
   const Cost         mCost;
   const ImageKey     mImageKey;
   const SurfaceKey   mSurfaceKey;
-  const Lifetime     mLifetime;
 };
 
 /**
  * An ImageSurfaceCache is a per-image surface cache. For correctness we must be
  * able to remove all surfaces associated with an image when the image is
  * destroyed or invalidated. Since this will happen frequently, it makes sense
  * to make it cheap by storing the surfaces for each image separately.
  *
@@ -254,19 +249,18 @@ public:
   typedef
     nsRefPtrHashtable<nsGenericHashKey<SurfaceKey>, CachedSurface> SurfaceTable;
 
   bool IsEmpty() const { return mSurfaces.Count() == 0; }
 
   void Insert(const SurfaceKey& aKey, CachedSurface* aSurface)
   {
     MOZ_ASSERT(aSurface, "Should have a surface");
-    MOZ_ASSERT(!mLocked || aSurface->GetLifetime() != Lifetime::Persistent ||
-               aSurface->IsLocked(),
-               "Inserting an unlocked persistent surface for a locked image");
+    MOZ_ASSERT(!mLocked || aSurface->IsPlaceholder() || aSurface->IsLocked(),
+               "Inserting an unlocked surface for a locked image");
     mSurfaces.Put(aKey, aSurface);
   }
 
   void Remove(CachedSurface* aSurface)
   {
     MOZ_ASSERT(aSurface, "Should have a surface");
     MOZ_ASSERT(mSurfaces.GetWeak(aSurface->GetSurfaceKey()),
         "Should not be removing a surface we don't have");
@@ -465,18 +459,17 @@ private:
 public:
   void InitMemoryReporter() { RegisterWeakMemoryReporter(this); }
 
   Mutex& GetMutex() { return mMutex; }
 
   InsertOutcome Insert(imgFrame*         aSurface,
                        const Cost        aCost,
                        const ImageKey    aImageKey,
-                       const SurfaceKey& aSurfaceKey,
-                       Lifetime          aLifetime)
+                       const SurfaceKey& aSurfaceKey)
   {
     // If this is a duplicate surface, refuse to replace the original.
     // XXX(seth): Calling Lookup() and then RemoveSurface() does the lookup
     // twice. We'll make this more efficient in bug 1185137.
     LookupResult result = Lookup(aImageKey, aSurfaceKey, /* aMarkUsed = */ false);
     if (MOZ_UNLIKELY(result)) {
       return InsertOutcome::FAILURE_ALREADY_PRESENT;
     }
@@ -508,22 +501,22 @@ public:
     // for this image, create it.
     nsRefPtr<ImageSurfaceCache> cache = GetImageCache(aImageKey);
     if (!cache) {
       cache = new ImageSurfaceCache;
       mImageCaches.Put(aImageKey, cache);
     }
 
     nsRefPtr<CachedSurface> surface =
-      new CachedSurface(aSurface, aCost, aImageKey, aSurfaceKey, aLifetime);
+      new CachedSurface(aSurface, aCost, aImageKey, aSurfaceKey);
 
-    // We require that locking succeed if the image is locked and the surface is
-    // persistent; the caller may need to know this to handle errors correctly.
-    if (cache->IsLocked() && aLifetime == Lifetime::Persistent) {
-      MOZ_ASSERT(!surface->IsPlaceholder(), "Placeholders should be transient");
+    // We require that locking succeed if the image is locked and we're not
+    // inserting a placeholder; the caller may need to know this to handle
+    // errors correctly.
+    if (cache->IsLocked() && !surface->IsPlaceholder()) {
       surface->SetLocked(true);
       if (!surface->IsLocked()) {
         return InsertOutcome::FAILURE;
       }
     }
 
     // Insert.
     MOZ_ASSERT(aCost <= mAvailableCost, "Inserting despite too large a cost");
@@ -536,18 +529,18 @@ public:
   void Remove(CachedSurface* aSurface)
   {
     MOZ_ASSERT(aSurface, "Should have a surface");
     ImageKey imageKey = aSurface->GetImageKey();
 
     nsRefPtr<ImageSurfaceCache> cache = GetImageCache(imageKey);
     MOZ_ASSERT(cache, "Shouldn't try to remove a surface with no image cache");
 
-    // If the surface was persistent, tell its image that we discarded it.
-    if (aSurface->GetLifetime() == Lifetime::Persistent) {
+    // If the surface was not a placeholder, tell its image that we discarded it.
+    if (!aSurface->IsPlaceholder()) {
       static_cast<Image*>(imageKey)->OnSurfaceDiscarded();
     }
 
     StopTracking(aSurface);
     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.)
@@ -772,19 +765,18 @@ public:
     // The per-image cache isn't needed anymore, so remove it as well.
     // This implicitly unlocks the image if it was locked.
     mImageCaches.Remove(aImageKey);
   }
 
   void DiscardAll()
   {
     // Remove in order of cost because mCosts is an array and the other data
-    // structures are all hash tables. Note that locked surfaces (persistent
-    // surfaces belonging to locked images) are not removed, since they aren't
-    // present in mCosts.
+    // structures are all hash tables. Note that locked surfaces are not
+    // removed, since they aren't present in mCosts.
     while (!mCosts.IsEmpty()) {
       Remove(mCosts.LastElement().GetSurface());
     }
   }
 
   void DiscardForMemoryPressure()
   {
     // Compute our discardable cost. Since locked surfaces aren't discardable,
@@ -808,18 +800,17 @@ public:
     while (mAvailableCost < targetCost) {
       MOZ_ASSERT(!mCosts.IsEmpty(), "Removed everything and still not done");
       Remove(mCosts.LastElement().GetSurface());
     }
   }
 
   void LockSurface(CachedSurface* aSurface)
   {
-    if (aSurface->GetLifetime() == Lifetime::Transient ||
-        aSurface->IsLocked()) {
+    if (aSurface->IsPlaceholder() || aSurface->IsLocked()) {
       return;
     }
 
     StopTracking(aSurface);
 
     // Lock the surface. This can fail.
     aSurface->SetLocked(true);
     StartTracking(aSurface);
@@ -832,18 +823,17 @@ public:
     static_cast<SurfaceCacheImpl*>(aCache)->StopTracking(aSurface);
     return PL_DHASH_NEXT;
   }
 
   static PLDHashOperator DoUnlockSurface(const SurfaceKey&,
                                          CachedSurface*    aSurface,
                                          void*             aCache)
   {
-    if (aSurface->GetLifetime() == Lifetime::Transient ||
-        !aSurface->IsLocked()) {
+    if (aSurface->IsPlaceholder() || !aSurface->IsLocked()) {
       return PL_DHASH_NEXT;
     }
 
     auto cache = static_cast<SurfaceCacheImpl*>(aCache);
     cache->StopTracking(aSurface);
 
     aSurface->SetLocked(false);
     cache->StartTracking(aSurface);
@@ -916,19 +906,19 @@ private:
   {
     nsRefPtr<ImageSurfaceCache> imageCache;
     mImageCaches.Get(aImageKey, getter_AddRefs(imageCache));
     return imageCache.forget();
   }
 
   // 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 if we start permitting multithreaded access to the surface
-  // cache, which seems likely, then the result would be meaningless: another
-  // thread could insert a persistent surface or lock an image at any time.
+  // 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;
   }
 
   void MarkUsed(CachedSurface* aSurface, ImageSurfaceCache* aCache)
   {
     if (aCache->IsLocked()) {
@@ -1091,44 +1081,42 @@ SurfaceCache::LookupBestMatch(const Imag
 
   MutexAutoLock lock(sInstance->GetMutex());
   return sInstance->LookupBestMatch(aImageKey, aSurfaceKey, aAlternateFlags);
 }
 
 /* static */ InsertOutcome
 SurfaceCache::Insert(imgFrame*         aSurface,
                      const ImageKey    aImageKey,
-                     const SurfaceKey& aSurfaceKey,
-                     Lifetime          aLifetime)
+                     const SurfaceKey& aSurfaceKey)
 {
   if (!sInstance) {
     return InsertOutcome::FAILURE;
   }
 
   // Refuse null surfaces.
   if (!aSurface) {
     MOZ_CRASH("Don't pass null surfaces to SurfaceCache::Insert");
   }
 
   MutexAutoLock lock(sInstance->GetMutex());
   Cost cost = ComputeCost(aSurface->GetSize(), aSurface->GetBytesPerPixel());
-  return sInstance->Insert(aSurface, cost, aImageKey, aSurfaceKey, aLifetime);
+  return sInstance->Insert(aSurface, cost, aImageKey, aSurfaceKey);
 }
 
 /* static */ InsertOutcome
 SurfaceCache::InsertPlaceholder(const ImageKey    aImageKey,
                                 const SurfaceKey& aSurfaceKey)
 {
   if (!sInstance) {
     return InsertOutcome::FAILURE;
   }
 
   MutexAutoLock lock(sInstance->GetMutex());
-  return sInstance->Insert(nullptr, sPlaceholderCost, aImageKey, aSurfaceKey,
-                           Lifetime::Transient);
+  return sInstance->Insert(nullptr, sPlaceholderCost, aImageKey, aSurfaceKey);
 }
 
 /* static */ bool
 SurfaceCache::CanHold(const IntSize& aSize, uint32_t aBytesPerPixel /* = 4 */)
 {
   if (!sInstance) {
     return false;
   }
--- a/image/SurfaceCache.h
+++ b/image/SurfaceCache.h
@@ -116,21 +116,16 @@ VectorSurfaceKey(const gfx::IntSize& aSi
                  float aAnimationTime)
 {
   // We don't care about aFlags for VectorImage because none of the flags we
   // have right now influence VectorImage's rendering. If we add a new flag that
   // *does* affect how a VectorImage renders, we'll have to change this.
   return SurfaceKey(aSize, aSVGContext, aAnimationTime, DefaultSurfaceFlags());
 }
 
-enum class Lifetime : uint8_t {
-  Transient,
-  Persistent
-};
-
 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.
 };
 
 /**
  * SurfaceCache is an imagelib-global service that allows caching of temporary
@@ -141,19 +136,18 @@ enum class InsertOutcome : uint8_t {
  * objects, which hold surfaces but also layer on additional features specific
  * to imagelib's needs like animation, padding support, and transparent support
  * for volatile buffers.
  *
  * Sometime it's useful to temporarily prevent surfaces from expiring from the
  * cache. This is most often because losing the data could harm the user
  * experience (for example, we often don't want to allow surfaces that are
  * currently visible to expire) or because it's not possible to rematerialize
- * the surface. SurfaceCache supports this through the use of image locking and
- * surface lifetimes; see the comments for Insert() and LockImage() for more
- * details.
+ * the surface. SurfaceCache supports this through the use of image locking; see
+ * the comments for Insert() and LockImage() for more details.
  *
  * Any image which stores surfaces in the SurfaceCache *must* ensure that it
  * calls RemoveImage() before it is destroyed. See the comments for
  * RemoveImage() for more details.
  */
 struct SurfaceCache
 {
   typedef gfx::IntSize IntSize;
@@ -173,18 +167,18 @@ struct SurfaceCache
    * drawable reference to that imgFrame.
    *
    * If the image associated with the surface is locked, then the surface will
    * be locked before it is returned.
    *
    * If the imgFrame was found in the cache, but had stored its surface in a
    * volatile buffer which was discarded by the OS, then it is automatically
    * removed from the cache and an empty LookupResult is returned. Note that
-   * this will never happen to persistent surfaces associated with a locked
-   * image; the cache keeps a strong reference to such surfaces internally.
+   * this will never happen to surfaces associated with a locked image; the
+   * cache keeps a strong reference to such surfaces internally.
    *
    * @param aImageKey       Key data identifying which image the surface belongs
    *                        to.
    *
    * @param aSurfaceKey     Key data which uniquely identifies the requested
    *                        surface.
    *
    * @param aAlternateFlags If not Nothing(), a different set of flags than the
@@ -231,64 +225,58 @@ struct SurfaceCache
                                       const Maybe<SurfaceFlags>& aAlternateFlags
                                         = Nothing());
 
   /**
    * 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.
    * If a matching placeholder is already present, the placeholder is removed.
    *
-   * Each surface in the cache has a lifetime, either Transient or Persistent.
-   * Transient surfaces can expire from the cache at any time. Persistent
-   * surfaces, on the other hand, will never expire as long as they remain
-   * locked, but if they become unlocked, can expire just like transient
-   * surfaces. When it is first inserted, a persistent surface is locked if its
-   * associated image is locked. When that image is later unlocked, the surface
-   * becomes unlocked too. To become locked again at that point, two things must
-   * happen: the image must become locked again (via LockImage()), and the
-   * surface must be touched again (via one of the Lookup() functions).
+   * Surfaces will never expire as long as they remain locked, but if they
+   * become unlocked, they can expire either because the SurfaceCache runs out
+   * of capacity or because they've gone too long without being used.  When it
+   * is first inserted, a surface is locked if its associated image is locked.
+   * When that image is later unlocked, the surface becomes unlocked too. To
+   * become locked again at that point, two things must happen: the image must
+   * become locked again (via LockImage()), and the surface must be touched
+   * again (via one of the Lookup() functions).
    *
    * All of this means that a very particular procedure has to be followed for
    * surfaces which cannot be rematerialized. First, they must be inserted
-   * with a persistent lifetime *after* the image is locked with LockImage(); if
-   * you use the other order, the surfaces might expire before LockImage() gets
-   * called or before the surface is touched again by Lookup(). Second, the
-   * image they are associated with must never be unlocked.
+   * *after* the image is locked with LockImage(); if you use the other order,
+   * the surfaces might expire before LockImage() gets called or before the
+   * surface is touched again by Lookup(). Second, the image they are associated
+   * with must never be unlocked.
    *
    * If a surface cannot be rematerialized, it may be important to know whether
    * 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, 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.
+   * reasons, or because it was already freed by the OS. If the surface isn't
+   * associated with 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.
    * @param aImageKey    Key data identifying which image the surface belongs
    *                     to.
    * @param aSurfaceKey  Key data which uniquely identifies the requested
    *                     surface.
-   * @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 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 InsertOutcome Insert(imgFrame*         aSurface,
                               const ImageKey    aImageKey,
-                              const SurfaceKey& aSurfaceKey,
-                              Lifetime          aLifetime);
+                              const SurfaceKey& aSurfaceKey);
 
   /**
    * Insert a placeholder for a surface into the cache. If a surface with the
    * same ImageKey and SurfaceKey is already in the cache, InsertPlaceholder()
    * returns FAILURE_ALREADY_PRESENT.
    *
    * Placeholders exist to allow lazy allocation of surfaces. The Lookup*()
    * methods will report whether a placeholder for an exactly matching surface
@@ -328,27 +316,26 @@ struct SurfaceCache
    *                        images.
    *
    * @return false if the surface cache can't hold a surface of that size.
    */
   static bool CanHold(const IntSize& aSize, uint32_t aBytesPerPixel = 4);
   static bool CanHold(size_t aSize);
 
   /**
-   * Locks an image. Any of the image's persistent surfaces which are either
-   * inserted or accessed while the image is locked will not expire.
+   * Locks an image. Any of the image's surfaces which are either inserted or
+   * accessed while the image is locked will not expire.
    *
    * Locking an image does not automatically lock that image's existing
-   * surfaces. A call to LockImage() guarantees that persistent surfaces which
-   * are inserted afterward will not expire before the next call to
-   * UnlockImage() or UnlockSurfaces() for that image. Surfaces that are
-   * accessed via Lookup() or LookupBestMatch() after a LockImage() call will
-   * also not expire until the next UnlockImage() or UnlockSurfaces() call for
-   * that image. Any other surfaces owned by the image may expire at any time,
-   * whether they are persistent or transient.
+   * surfaces. A call to LockImage() guarantees that surfaces which are inserted
+   * afterward will not expire before the next call to UnlockImage() or
+   * UnlockSurfaces() for that image. Surfaces that are accessed via Lookup() or
+   * LookupBestMatch() after a LockImage() call will also not expire until the
+   * next UnlockImage() or UnlockSurfaces() call for that image. Any other
+   * surfaces owned by the image may expire at any time.
    *
    * Regardless of locking, any of an image's surfaces may be removed using
    * RemoveSurface(), and all of an image's surfaces are removed by
    * RemoveImage(), whether the image is locked or not.
    *
    * It's safe to call LockImage() on an image that's already locked; this has
    * no effect.
    *
@@ -376,20 +363,20 @@ struct SurfaceCache
    * Unlocks the existing surfaces of an image, allowing them to expire at any
    * time.
    *
    * This does not unlock the image itself, so accessing the surfaces via
    * Lookup() or LookupBestMatch() will lock them again, and prevent them from
    * expiring.
    *
    * This is intended to be used in situations where it's no longer clear that
-   * all of the persistent surfaces owned by an image are needed. Calling
-   * UnlockSurfaces() and then taking some action that will cause Lookup() to
-   * touch any surfaces that are still useful will permit the remaining surfaces
-   * to expire from the cache.
+   * all of the surfaces owned by an image are needed. Calling UnlockSurfaces()
+   * and then taking some action that will cause Lookup() to touch any surfaces
+   * that are still useful will permit the remaining surfaces to expire from the
+   * cache.
    *
    * If the image is unlocked, this has no effect.
    *
    * @param aImageKey    The image which should have its existing surfaces
    *                     unlocked.
    */
   static void UnlockSurfaces(const ImageKey aImageKey);
 
@@ -420,19 +407,19 @@ struct SurfaceCache
    *
    * @param aImageKey  The image which should be removed from the cache.
    */
   static void RemoveImage(const ImageKey aImageKey);
 
   /**
    * Evicts all evictable surfaces from the cache.
    *
-   * All surfaces are evictable except for persistent surfaces associated with
-   * locked images. Non-evictable surfaces can only be removed by
-   * RemoveSurface() or RemoveImage().
+   * All surfaces are evictable except for surfaces associated with locked
+   * images. Non-evictable surfaces can only be removed by RemoveSurface() or
+   * RemoveImage().
    */
   static void DiscardAll();
 
   /**
    * Collects an accounting of the surfaces contained in the SurfaceCache for
    * the given image, along with their size and various other metadata.
    *
    * This is intended for use with memory reporting.
--- a/image/VectorImage.cpp
+++ b/image/VectorImage.cpp
@@ -915,18 +915,17 @@ VectorImage::CreateSurfaceAndShow(const 
   if (!surface) {
     return Show(svgDrawable, aParams);
   }
 
   // Attempt to cache the frame.
   SurfaceCache::Insert(frame, ImageKey(this),
                        VectorSurfaceKey(aParams.size,
                                         aParams.svgContext,
-                                        aParams.animationTime),
-                       Lifetime::Persistent);
+                                        aParams.animationTime));
 
   // Draw.
   nsRefPtr<gfxDrawable> drawable =
     new gfxSurfaceDrawable(surface, aParams.size);
   Show(drawable, aParams);
 
   // Send out an invalidation so that surfaces that are still in use get
   // re-locked. See the discussion of the UnlockSurfaces call above.