Backed out changeset 37340346a89e (Bug 1289628 - Return ISurfaceProvider objects from SurfaceCache lookup functions. r=dholbert,edwin) for causing bug 1292290. a=ritu
authorTimothy Nikkel <tnikkel@gmail.com>
Mon, 15 Aug 2016 19:58:35 -0500
changeset 349858 2a0981531355dc77614e72c72fda7498bb074ffe
parent 349857 e6bbc936ae4ab1c5880c5ea3ad48839c748b5262
child 349859 3443b3c753233c7ea260b09366b4b28ac2c4525a
push id1230
push userjlund@mozilla.com
push dateMon, 31 Oct 2016 18:13:35 +0000
treeherdermozilla-release@5e06e3766db2 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdholbert, edwin, ritu
bugs1289628, 1292290
milestone50.0a2
backs out37340346a89e3ff5680559afabbc430ffaf9bb74
Backed out changeset 37340346a89e (Bug 1289628 - Return ISurfaceProvider objects from SurfaceCache lookup functions. r=dholbert,edwin) for causing bug 1292290. a=ritu
image/FrameAnimator.cpp
image/LookupResult.h
image/RasterImage.cpp
image/SurfaceCache.cpp
image/SurfaceCache.h
image/VectorImage.cpp
image/VectorImage.h
--- a/image/FrameAnimator.cpp
+++ b/image/FrameAnimator.cpp
@@ -2,17 +2,16 @@
  * This Source Code Form is subject to the terms of the Mozilla Public
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 #include "FrameAnimator.h"
 
 #include "mozilla/MemoryReporting.h"
 #include "mozilla/Move.h"
-#include "mozilla/NotNull.h"
 #include "imgIContainer.h"
 #include "LookupResult.h"
 #include "MainThreadUtils.h"
 #include "RasterImage.h"
 
 #include "pixman.h"
 
 namespace mozilla {
@@ -297,31 +296,27 @@ FrameAnimator::RequestRefresh(AnimationS
 
 LookupResult
 FrameAnimator::GetCompositedFrame(uint32_t aFrameNum)
 {
   MOZ_ASSERT(aFrameNum != 0, "First frame is never composited");
 
   // If we have a composited version of this frame, return that.
   if (mLastCompositedFrameIndex == int32_t(aFrameNum)) {
-    RefPtr<ISurfaceProvider> provider =
-      new SimpleSurfaceProvider(WrapNotNull(mCompositingFrame.get()));
-    return LookupResult(Move(provider), MatchType::EXACT);
+    return LookupResult(mCompositingFrame->DrawableRef(), MatchType::EXACT);
   }
 
   // Otherwise return the raw frame. DoBlend is required to ensure that we only
   // hit this case if the frame is not paletted and doesn't require compositing.
   LookupResult result =
     SurfaceCache::Lookup(ImageKey(mImage),
                          RasterSurfaceKey(mSize,
                                           DefaultSurfaceFlags(),
                                           aFrameNum));
-  MOZ_ASSERT(!result ||
-             !result.Provider()->DrawableRef() ||
-             !result.Provider()->DrawableRef()->GetIsPaletted(),
+  MOZ_ASSERT(!result || !result.DrawableRef()->GetIsPaletted(),
              "About to return a paletted frame");
   return result;
 }
 
 FrameTimeout
 FrameAnimator::GetTimeoutForFrame(uint32_t aFrameNum) const
 {
   RawAccessFrameRef frame = GetRawFrame(aFrameNum);
@@ -381,28 +376,18 @@ FrameAnimator::CollectSizeOfCompositingS
 RawAccessFrameRef
 FrameAnimator::GetRawFrame(uint32_t aFrameNum) const
 {
   LookupResult result =
     SurfaceCache::Lookup(ImageKey(mImage),
                          RasterSurfaceKey(mSize,
                                           DefaultSurfaceFlags(),
                                           aFrameNum));
-  if (!result) {
-    return RawAccessFrameRef();
-  }
-
-  DrawableFrameRef drawableRef = result.Provider()->DrawableRef();
-  if (!drawableRef) {
-    MOZ_ASSERT_UNREACHABLE("Animated image frames should be locked and "
-                           "in-memory; how did we lose one?");
-    return RawAccessFrameRef();
-  }
-
-  return drawableRef->RawAccessRef();
+  return result ? result.DrawableRef()->RawAccessRef()
+                : RawAccessFrameRef();
 }
 
 //******************************************************************************
 // DoBlend gets called when the timer for animation get fired and we have to
 // update the composited frame of the animation.
 bool
 FrameAnimator::DoBlend(IntRect* aDirtyRect,
                        uint32_t aPrevFrameIndex,
--- a/image/LookupResult.h
+++ b/image/LookupResult.h
@@ -8,83 +8,83 @@
  * combines a surface with relevant metadata tracked by SurfaceCache.
  */
 
 #ifndef mozilla_image_LookupResult_h
 #define mozilla_image_LookupResult_h
 
 #include "mozilla/Attributes.h"
 #include "mozilla/Move.h"
-#include "ISurfaceProvider.h"
+#include "imgFrame.h"
 
 namespace mozilla {
 namespace image {
 
 enum class MatchType : uint8_t
 {
   NOT_FOUND,  // No matching surface and no placeholder.
   PENDING,    // Found a matching placeholder, but no surface.
   EXACT,      // Found a surface that matches exactly.
   SUBSTITUTE_BECAUSE_NOT_FOUND,  // No exact match, but found a similar one.
   SUBSTITUTE_BECAUSE_PENDING     // Found a similar surface and a placeholder
                                  // for an exact match.
 };
 
 /**
  * LookupResult is the return type of SurfaceCache's Lookup*() functions. It
- * combines an ISurfaceProvider with relevant metadata tracked by SurfaceCache.
+ * combines a surface with relevant metadata tracked by SurfaceCache.
  */
 class MOZ_STACK_CLASS LookupResult
 {
 public:
   explicit LookupResult(MatchType aMatchType)
     : mMatchType(aMatchType)
   {
     MOZ_ASSERT(mMatchType == MatchType::NOT_FOUND ||
                mMatchType == MatchType::PENDING,
                "Only NOT_FOUND or PENDING make sense with no surface");
   }
 
   LookupResult(LookupResult&& aOther)
-    : mProvider(Move(aOther.mProvider))
+    : mDrawableRef(Move(aOther.mDrawableRef))
     , mMatchType(aOther.mMatchType)
   { }
 
-  LookupResult(RefPtr<ISurfaceProvider>&& aProvider, MatchType aMatchType)
-    : mProvider(Move(aProvider))
+  LookupResult(DrawableFrameRef&& aDrawableRef, MatchType aMatchType)
+    : mDrawableRef(Move(aDrawableRef))
     , mMatchType(aMatchType)
   {
-    MOZ_ASSERT(!mProvider || !(mMatchType == MatchType::NOT_FOUND ||
-                               mMatchType == MatchType::PENDING),
+    MOZ_ASSERT(!mDrawableRef || !(mMatchType == MatchType::NOT_FOUND ||
+                                  mMatchType == MatchType::PENDING),
                "Only NOT_FOUND or PENDING make sense with no surface");
-    MOZ_ASSERT(mProvider || mMatchType == MatchType::NOT_FOUND ||
-                            mMatchType == MatchType::PENDING,
+    MOZ_ASSERT(mDrawableRef || mMatchType == MatchType::NOT_FOUND ||
+                               mMatchType == MatchType::PENDING,
                "NOT_FOUND or PENDING do not make sense with a surface");
   }
 
   LookupResult& operator=(LookupResult&& aOther)
   {
     MOZ_ASSERT(&aOther != this, "Self-move-assignment is not supported");
-    mProvider = Move(aOther.mProvider);
+    mDrawableRef = Move(aOther.mDrawableRef);
     mMatchType = aOther.mMatchType;
     return *this;
   }
 
-  ISurfaceProvider* Provider() { return mProvider.get(); }
-  const ISurfaceProvider* Provider() const { return mProvider.get(); }
+  DrawableFrameRef& DrawableRef() { return mDrawableRef; }
+  const DrawableFrameRef& DrawableRef() const { return mDrawableRef; }
 
-  /// @return true if this LookupResult contains a surface provider.
-  explicit operator bool() const { return bool(mProvider); }
+  /// @return true if this LookupResult contains a surface.
+  explicit operator bool() const { return bool(mDrawableRef); }
 
   /// @return what kind of match this is (exact, substitute, etc.)
   MatchType Type() const { return mMatchType; }
 
 private:
   LookupResult(const LookupResult&) = delete;
 
-  RefPtr<ISurfaceProvider> mProvider;
+  DrawableFrameRef mDrawableRef;
   MatchType mMatchType;
 };
 
 } // namespace image
 } // namespace mozilla
 
 #endif // mozilla_image_LookupResult_h
--- a/image/RasterImage.cpp
+++ b/image/RasterImage.cpp
@@ -349,51 +349,40 @@ RasterImage::LookupFrame(uint32_t aFrame
     }
   }
 
   if (!result) {
     // We still weren't able to get a frame. Give up.
     return DrawableFrameRef();
   }
 
-  // OK, we've got an ISurfaceProvider. Ask it to give us a surface containing
-  // the frame we're looking for. (This could be computationally expensive in
-  // some cases.)
-  DrawableFrameRef drawableRef = result.Provider()->DrawableRef();
-  if (!drawableRef) {
-    // An ISurfaceProvider will only fail to give us a surface in catastrophic
-    // cases such as the operating system freeing our volatile buffers. Our best
-    // bet is to throw everything out and attempt to recover.
-    RecoverFromInvalidFrames(aSize, aFlags);
+  if (result.DrawableRef()->GetCompositingFailed()) {
     return DrawableFrameRef();
   }
 
-  if (drawableRef->GetCompositingFailed()) {
-    return DrawableFrameRef();
-  }
-
-  MOZ_ASSERT(!drawableRef->GetIsPaletted(), "Should not have a paletted frame");
+  MOZ_ASSERT(!result.DrawableRef()->GetIsPaletted(),
+             "Should not have a paletted frame");
 
   // Sync decoding guarantees that we got the frame, but if it's owned by an
   // async decoder that's currently running, the contents of the frame may not
   // be available yet. Make sure we get everything.
   if (mHasSourceData && (aFlags & FLAG_SYNC_DECODE)) {
-    drawableRef->WaitUntilFinished();
+    result.DrawableRef()->WaitUntilFinished();
   }
 
   // If we could have done some decoding in this function we need to check if
   // that decoding encountered an error and hence aborted the surface. We want
   // to avoid calling IsAborted if we weren't passed any sync decode flag because
   // IsAborted acquires the monitor for the imgFrame.
   if (aFlags & (FLAG_SYNC_DECODE | FLAG_SYNC_DECODE_IF_FAST) &&
-      drawableRef->IsAborted()) {
+    result.DrawableRef()->IsAborted()) {
     return DrawableFrameRef();
   }
 
-  return Move(drawableRef);
+  return Move(result.DrawableRef());
 }
 
 uint32_t
 RasterImage::GetCurrentFrameIndex() const
 {
   if (mAnimationState) {
     return mAnimationState->GetCurrentAnimationFrameIndex();
   }
--- a/image/SurfaceCache.cpp
+++ b/image/SurfaceCache.cpp
@@ -51,17 +51,16 @@ class SurfaceCacheImpl;
 
 ///////////////////////////////////////////////////////////////////////////////
 // Static Data
 ///////////////////////////////////////////////////////////////////////////////
 
 // The single surface cache instance.
 static StaticRefPtr<SurfaceCacheImpl> sInstance;
 
-
 ///////////////////////////////////////////////////////////////////////////////
 // SurfaceCache Implementation
 ///////////////////////////////////////////////////////////////////////////////
 
 /**
  * Cost models the cost of storing a surface in the cache. Right now, this is
  * simply an estimate of the size of the surface in bytes, but in the future it
  * may be worth taking into account the cost of rematerializing the surface as
@@ -141,27 +140,24 @@ public:
     , mImageKey(aImageKey)
     , mSurfaceKey(aSurfaceKey)
   {
     MOZ_ASSERT(!IsPlaceholder() || mCost == sPlaceholderCost,
                "Placeholder should have trivial cost");
     MOZ_ASSERT(mImageKey, "Must have a valid image key");
   }
 
-  already_AddRefed<ISurfaceProvider> Provider() const
+  DrawableFrameRef DrawableRef() const
   {
     if (MOZ_UNLIKELY(IsPlaceholder())) {
-      MOZ_ASSERT_UNREACHABLE("Shouldn't call Provider() on a placeholder");
-      return nullptr;
+      MOZ_ASSERT_UNREACHABLE("Shouldn't call DrawableRef() on a placeholder");
+      return DrawableFrameRef();
     }
 
-    MOZ_ASSERT(mProvider);
-
-    RefPtr<ISurfaceProvider> provider = mProvider;
-    return provider.forget();
+    return mProvider->DrawableRef();
   }
 
   void SetLocked(bool aLocked)
   {
     if (IsPlaceholder()) {
       return;  // Can't lock a placeholder.
     }
 
@@ -192,51 +188,40 @@ public:
 
     void Add(CachedSurface* aCachedSurface)
     {
       MOZ_ASSERT(aCachedSurface, "Should have a CachedSurface");
 
       SurfaceMemoryCounter counter(aCachedSurface->GetSurfaceKey(),
                                    aCachedSurface->IsLocked());
 
-      if (aCachedSurface->IsPlaceholder()) {
-        mCounters.AppendElement(counter);
-        return;
-      }
-
-      RefPtr<ISurfaceProvider> provider = aCachedSurface->Provider();
-      if (!provider) {
-        MOZ_ASSERT_UNREACHABLE("Not a placeholder, but no ISurfaceProvider?");
-        mCounters.AppendElement(counter);
-        return;
-      }
+      if (!aCachedSurface->IsPlaceholder()) {
+        DrawableFrameRef surfaceRef = aCachedSurface->DrawableRef();
+        if (surfaceRef) {
+          counter.SubframeSize() = Some(surfaceRef->GetSize());
 
-      DrawableFrameRef surfaceRef = provider->DrawableRef();
-      if (!surfaceRef) {
-        mCounters.AppendElement(counter);
-        return;
+          size_t heap = 0, nonHeap = 0;
+          surfaceRef->AddSizeOfExcludingThis(mMallocSizeOf, heap, nonHeap);
+          counter.Values().SetDecodedHeap(heap);
+          counter.Values().SetDecodedNonHeap(nonHeap);
+        }
       }
 
-      counter.SubframeSize() = Some(surfaceRef->GetSize());
-      size_t heap = 0, nonHeap = 0;
-      surfaceRef->AddSizeOfExcludingThis(mMallocSizeOf, heap, nonHeap);
-      counter.Values().SetDecodedHeap(heap);
-      counter.Values().SetDecodedNonHeap(nonHeap);
-
       mCounters.AppendElement(counter);
     }
 
   private:
     nsTArray<SurfaceMemoryCounter>& mCounters;
     MallocSizeOf                    mMallocSizeOf;
   };
 
 private:
   nsExpirationState  mExpirationState;
   RefPtr<ISurfaceProvider> mProvider;
+  DrawableFrameRef   mDrawableRef;
   const Cost         mCost;
   const ImageKey     mImageKey;
   const SurfaceKey   mSurfaceKey;
 };
 
 static int64_t
 AreaOfIntSize(const IntSize& aSize) {
   return static_cast<int64_t>(aSize.width) * static_cast<int64_t>(aSize.height);
@@ -598,68 +583,81 @@ public:
       // Lookup in the per-image cache missed.
       return LookupResult(MatchType::NOT_FOUND);
     }
 
     if (surface->IsPlaceholder()) {
       return LookupResult(MatchType::PENDING);
     }
 
-    RefPtr<ISurfaceProvider> provider = surface->Provider();
-    if (!provider) {
-      MOZ_ASSERT_UNREACHABLE("Not a placeholder, but no ISurfaceProvider?");
+    DrawableFrameRef ref = surface->DrawableRef();
+    if (!ref) {
+      // The surface was released by the operating system. Remove the cache
+      // entry as well.
       Remove(surface);
       return LookupResult(MatchType::NOT_FOUND);
     }
 
     if (aMarkUsed) {
       MarkUsed(surface, cache);
     }
 
     MOZ_ASSERT(surface->GetSurfaceKey() == aSurfaceKey,
                "Lookup() not returning an exact match?");
-    return LookupResult(Move(provider), MatchType::EXACT);
+    return LookupResult(Move(ref), MatchType::EXACT);
   }
 
   LookupResult LookupBestMatch(const ImageKey         aImageKey,
                                const SurfaceKey&      aSurfaceKey)
   {
     RefPtr<ImageSurfaceCache> cache = GetImageCache(aImageKey);
     if (!cache) {
       // No cached surfaces for this image.
       return LookupResult(MatchType::NOT_FOUND);
     }
 
+    // Repeatedly look up the best match, trying again if the resulting surface
+    // has been freed by the operating system, until we can either lock a
+    // surface for drawing or there are no matching surfaces left.
+    // XXX(seth): This is O(N^2), but N is expected to be very small. If we
+    // encounter a performance problem here we can revisit this.
+
     RefPtr<CachedSurface> surface;
+    DrawableFrameRef ref;
     MatchType matchType = MatchType::NOT_FOUND;
-    Tie(surface, matchType) = cache->LookupBestMatch(aSurfaceKey);
-    if (!surface) {
-      return LookupResult(matchType);  // Lookup in the per-image cache missed.
-    }
+    while (true) {
+      Tie(surface, matchType) = cache->LookupBestMatch(aSurfaceKey);
+
+      if (!surface) {
+        return LookupResult(matchType);  // Lookup in the per-image cache missed.
+      }
 
-    RefPtr<ISurfaceProvider> provider = surface->Provider();
-    if (!provider) {
-      MOZ_ASSERT_UNREACHABLE("Not a placeholder, but no ISurfaceProvider?");
+      ref = surface->DrawableRef();
+      if (ref) {
+        break;
+      }
+
+      // The surface was released by the operating system. Remove the cache
+      // entry as well.
       Remove(surface);
-      return LookupResult(MatchType::NOT_FOUND);
     }
 
     MOZ_ASSERT_IF(matchType == MatchType::EXACT,
                   surface->GetSurfaceKey() == aSurfaceKey);
     MOZ_ASSERT_IF(matchType == MatchType::SUBSTITUTE_BECAUSE_NOT_FOUND ||
                   matchType == MatchType::SUBSTITUTE_BECAUSE_PENDING,
       surface->GetSurfaceKey().SVGContext() == aSurfaceKey.SVGContext() &&
       surface->GetSurfaceKey().AnimationTime() == aSurfaceKey.AnimationTime() &&
       surface->GetSurfaceKey().Flags() == aSurfaceKey.Flags());
 
     if (matchType == MatchType::EXACT) {
       MarkUsed(surface, cache);
     }
 
-    return LookupResult(Move(provider), matchType);
+    return LookupResult(Move(ref), matchType);
   }
 
   bool CanHold(const Cost aCost) const
   {
     return aCost <= mMaxCost;
   }
 
   size_t MaximumCapacity() const
--- a/image/SurfaceCache.h
+++ b/image/SurfaceCache.h
@@ -158,56 +158,57 @@ struct SurfaceCache
   static void Initialize();
 
   /**
    * Release static data. Called during imagelib module shutdown.
    */
   static void Shutdown();
 
   /**
-   * Looks up and returns the requested cache entry.
+   * Looks up the requested cache entry and returns a drawable reference to its
+   * associated surface.
    *
    * If the image associated with the cache entry is locked, then the entry will
    * be locked before it is returned.
    *
-   * This function returns an ISurfaceProvider, but it does not guarantee that
-   * the ISurfaceProvider can give the caller a drawable surface. Lookup()
-   * callers should check that ISurfaceProvider::DrawableRef() returns a
-   * non-empty value; if not, some sort of serious failure has occurred, and the
-   * best bet is to remove all existing surfaces for the image from the cache
-   * using RemoveImage() and try to recover. The most likely cause for this kind
-   * of failure is the surface's volatile buffer being freed by the operating
-   * system due to extreme memory pressure.
+   * If a matching ISurfaceProvider was found in the cache, but SurfaceCache
+   * couldn't obtain a surface from it (e.g. because it 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 ISurfaceProviders associated with a
+   * locked image; SurfaceCache tells such ISurfaceProviders to keep a strong
+   * references to their data internally.
    *
    * @param aImageKey       Key data identifying which image the cache entry
    *                        belongs to.
    * @param aSurfaceKey     Key data which uniquely identifies the requested
    *                        cache entry.
-   * @return                a LookupResult which will contain an ISurfaceProvider
-   *                        for the requested surface if a matching cache entry
-   *                        was found.
+   * @return                a LookupResult, which will either contain a
+   *                        DrawableFrameRef to a surface, or an empty
+   *                        DrawableFrameRef if the cache entry was not found.
    */
   static LookupResult Lookup(const ImageKey    aImageKey,
                              const SurfaceKey& aSurfaceKey);
 
   /**
-   * Looks up and returns the best matching cache entry.
+   * Looks up the best matching cache entry and returns a drawable reference to
+   * its associated surface.
    *
    * The result may vary from the requested cache entry only in terms of size.
    *
    * @param aImageKey       Key data identifying which image the cache entry
    *                        belongs to.
    * @param aSurfaceKey     Key data which uniquely identifies the requested
    *                        cache entry.
-   * @return                a LookupResult which will contain either an
-   *                        ISurfaceProvider for a surface similar to the one
-   *                        the caller requested, or no ISurfaceProvider if no
-   *                        acceptable match was found. Callers can use
-   *                        LookupResult::IsExactMatch() to check whether the
-   *                        returned ISurfaceProvider exactly matches
+   * @return                a LookupResult, which will either contain a
+   *                        DrawableFrameRef to a surface similar to the
+   *                        the one the caller requested, or an empty
+   *                        DrawableFrameRef if no acceptable match was found.
+   *                        Callers can use LookupResult::IsExactMatch() to check
+   *                        whether the returned surface exactly matches
    *                        @aSurfaceKey.
    */
   static LookupResult LookupBestMatch(const ImageKey    aImageKey,
                                       const SurfaceKey& aSurfaceKey);
 
   /**
    * Insert an ISurfaceProvider into the cache. If an entry with the same
    * ImageKey and SurfaceKey is already in the cache, Insert returns
--- a/image/VectorImage.cpp
+++ b/image/VectorImage.cpp
@@ -854,65 +854,44 @@ VectorImage::Draw(gfxContext* aContext,
                                  : mSVGDocumentWrapper->GetCurrentTime();
   AutoSVGRenderingState autoSVGState(svgContext, animTime,
                                      mSVGDocumentWrapper->GetRootSVGElem());
 
 
   SVGDrawingParameters params(aContext, aSize, aRegion, aSamplingFilter,
                               svgContext, animTime, aFlags);
 
-  // If we have an prerasterized version of this image that matches the
-  // drawing parameters, use that.
-  RefPtr<gfxDrawable> svgDrawable = LookupCachedSurface(params);
-  if (svgDrawable) {
-    Show(svgDrawable, params);
+  if (aFlags & FLAG_BYPASS_SURFACE_CACHE) {
+    CreateSurfaceAndShow(params);
     return DrawResult::SUCCESS;
   }
 
-  // We didn't get a hit in the surface cache, so we'll need to rerasterize.
-  CreateSurfaceAndShow(params);
-  return DrawResult::SUCCESS;
-}
-
-already_AddRefed<gfxDrawable>
-VectorImage::LookupCachedSurface(const SVGDrawingParameters& aParams)
-{
-  // If we're not allowed to use a cached surface, don't attempt a lookup.
-  if (aParams.flags & FLAG_BYPASS_SURFACE_CACHE) {
-    return nullptr;
-  }
-
   LookupResult result =
     SurfaceCache::Lookup(ImageKey(this),
-                         VectorSurfaceKey(aParams.size,
-                                          aParams.svgContext,
-                                          aParams.animationTime));
-  if (!result) {
-    return nullptr;  // No matching surface.
+                         VectorSurfaceKey(params.size,
+                                          params.svgContext,
+                                          params.animationTime));
+
+  // Draw.
+  if (result) {
+    RefPtr<SourceSurface> surface = result.DrawableRef()->GetSurface();
+    if (surface) {
+      RefPtr<gfxDrawable> svgDrawable =
+        new gfxSurfaceDrawable(surface, result.DrawableRef()->GetSize());
+      Show(svgDrawable, params);
+      return DrawResult::SUCCESS;
+    }
+
+    // We lost our surface due to some catastrophic event.
+    RecoverFromLossOfSurfaces();
   }
 
-  DrawableFrameRef drawableRef = result.Provider()->DrawableRef();
-  if (!drawableRef) {
-    // Something went wrong. (Probably the OS freeing our volatile buffer due to
-    // low memory.) Attempt to recover.
-    RecoverFromLossOfSurfaces();
-    return nullptr;
-  }
+  CreateSurfaceAndShow(params);
 
-  RefPtr<SourceSurface> surface = drawableRef->GetSurface();
-  if (!surface) {
-    // Something went wrong. (Probably a GPU driver crash or device reset.)
-    // Attempt to recover.
-    RecoverFromLossOfSurfaces();
-    return nullptr;
-  }
-
-  RefPtr<gfxDrawable> svgDrawable =
-    new gfxSurfaceDrawable(surface, drawableRef->GetSize());
-  return svgDrawable.forget();
+  return DrawResult::SUCCESS;
 }
 
 void
 VectorImage::CreateSurfaceAndShow(const SVGDrawingParameters& aParams)
 {
   mSVGDocumentWrapper->UpdateViewportBounds(aParams.viewportSize);
   mSVGDocumentWrapper->FlushImageTransformInvalidation();
 
--- a/image/VectorImage.h
+++ b/image/VectorImage.h
@@ -73,24 +73,20 @@ public:
 protected:
   explicit VectorImage(ImageURL* aURI = nullptr);
   virtual ~VectorImage();
 
   virtual nsresult StartAnimation() override;
   virtual nsresult StopAnimation() override;
   virtual bool     ShouldAnimate() override;
 
-private:
-  /// Attempt to find a cached surface matching @aParams in the SurfaceCache.
-  already_AddRefed<gfxDrawable>
-    LookupCachedSurface(const SVGDrawingParameters& aParams);
-
   void CreateSurfaceAndShow(const SVGDrawingParameters& aParams);
   void Show(gfxDrawable* aDrawable, const SVGDrawingParameters& aParams);
 
+private:
   nsresult Init(const char* aMimeType, uint32_t aFlags);
 
   /**
    * In catastrophic circumstances like a GPU driver crash, we may lose our
    * surfaces even if they're locked. RecoverFromLossOfSurfaces discards all
    * existing surfaces, allowing us to recover.
    */
   void RecoverFromLossOfSurfaces();