Bug 1389988 - Handle a data race between a new sync decode request and a pending decoder. r=tnikkel

If there is an active provider which has yet to produce a frame, any
calls to SurfaceCache::Lookup will return MatchType::PENDING. If
RasterImage::Lookup gets the above result while given FLAG_SYNC_DECODE,
it will attempt to start a new decoder. It is entirely possible that
when we try to insert the new provider into the SurfaceCache, it cannot
because the original provider finally did produce something. In that
case we should abandon attempting to redecode and retry our lookup.

image/DecoderFactory.cpp

@@ -109,17 +109,18 @@ DecoderFactory::GetDecoder(DecoderType aType,
   return decoder.forget();
 }
 
-/* static */ already_AddRefed<IDecodingTask>
+/* static */ nsresult
 DecoderFactory::CreateDecoder(DecoderType aType,
                               NotNull<RasterImage*> aImage,
                               NotNull<SourceBuffer*> aSourceBuffer,
                               const IntSize& aIntrinsicSize,
                               const IntSize& aOutputSize,
                               DecoderFlags aDecoderFlags,
-                              SurfaceFlags aSurfaceFlags)
+                              SurfaceFlags aSurfaceFlags,
+                              IDecodingTask** aOutTask)
 {
   if (aType == DecoderType::UNKNOWN) {
-    return nullptr;
+    return NS_ERROR_INVALID_ARG;
   }
 
   // Create an anonymous decoder. Interaction with the SurfaceCache and the
@@ -135,8 +136,9 @@ DecoderFactory::CreateDecoder(DecoderType aType,
   decoder->SetDecoderFlags(aDecoderFlags | DecoderFlags::FIRST_FRAME_ONLY);
   decoder->SetSurfaceFlags(aSurfaceFlags);
 
-  if (NS_FAILED(decoder->Init())) {
-    return nullptr;
+  nsresult rv = decoder->Init();
+  if (NS_FAILED(rv)) {
+    return NS_ERROR_FAILURE;
   }
 
   // Create a DecodedSurfaceProvider which will manage the decoding process and
@@ -151,25 +153,32 @@ DecoderFactory::CreateDecoder(DecoderType aType,
 
   // Attempt to insert the surface provider into the surface cache right away so
   // we won't trigger any more decoders with the same parameters.
-  if (SurfaceCache::Insert(provider) != InsertOutcome::SUCCESS) {
-    return nullptr;
+  switch (SurfaceCache::Insert(provider)) {
+    case InsertOutcome::SUCCESS:
+      break;
+    case InsertOutcome::FAILURE_ALREADY_PRESENT:
+      return NS_ERROR_ALREADY_INITIALIZED;
+    default:
+      return NS_ERROR_FAILURE;
   }
 
   // Return the surface provider in its IDecodingTask guise.
   RefPtr<IDecodingTask> task = provider.get();
-  return task.forget();
+  task.forget(aOutTask);
+  return NS_OK;
 }
 
-/* static */ already_AddRefed<IDecodingTask>
+/* static */ nsresult
 DecoderFactory::CreateAnimationDecoder(DecoderType aType,
                                        NotNull<RasterImage*> aImage,
                                        NotNull<SourceBuffer*> aSourceBuffer,
                                        const IntSize& aIntrinsicSize,
                                        DecoderFlags aDecoderFlags,
-                                       SurfaceFlags aSurfaceFlags)
+                                       SurfaceFlags aSurfaceFlags,
+                                       IDecodingTask** aOutTask)
 {
   if (aType == DecoderType::UNKNOWN) {
-    return nullptr;
+    return NS_ERROR_INVALID_ARG;
   }
 
   MOZ_ASSERT(aType == DecoderType::GIF || aType == DecoderType::PNG,
@@ -186,8 +195,9 @@ DecoderFactory::CreateAnimationDecoder(DecoderType aType,
   decoder->SetDecoderFlags(aDecoderFlags | DecoderFlags::IS_REDECODE);
   decoder->SetSurfaceFlags(aSurfaceFlags);
 
-  if (NS_FAILED(decoder->Init())) {
-    return nullptr;
+  nsresult rv = decoder->Init();
+  if (NS_FAILED(rv)) {
+    return NS_ERROR_FAILURE;
   }
 
   // Create an AnimationSurfaceProvider which will manage the decoding process
@@ -199,13 +209,19 @@ DecoderFactory::CreateAnimationDecoder(DecoderType aType,
 
   // Attempt to insert the surface provider into the surface cache right away so
   // we won't trigger any more decoders with the same parameters.
-  if (SurfaceCache::Insert(provider) != InsertOutcome::SUCCESS) {
-    return nullptr;
+  switch (SurfaceCache::Insert(provider)) {
+    case InsertOutcome::SUCCESS:
+      break;
+    case InsertOutcome::FAILURE_ALREADY_PRESENT:
+      return NS_ERROR_ALREADY_INITIALIZED;
+    default:
+      return NS_ERROR_FAILURE;
   }
 
   // Return the surface provider in its IDecodingTask guise.
   RefPtr<IDecodingTask> task = provider.get();
-  return task.forget();
+  task.forget(aOutTask);
+  return NS_OK;
 }
 
 /* static */ already_AddRefed<IDecodingTask>

image/DecoderFactory.h

@@ -64,15 +64,21 @@ public:
    * @param aDecoderFlags Flags specifying the behavior of this decoder.
    * @param aSurfaceFlags Flags specifying the type of output this decoder
    *                      should produce.
+   * @param aOutTask Task representing the decoder.
+   * @return NS_OK if the decoder has been created/initialized successfully;
+   *         NS_ERROR_ALREADY_INITIALIZED if there is already an active decoder
+   *           for this image;
+   *         Else some other unrecoverable error occurred.
    */
-  static already_AddRefed<IDecodingTask>
+  static nsresult
   CreateDecoder(DecoderType aType,
                 NotNull<RasterImage*> aImage,
                 NotNull<SourceBuffer*> aSourceBuffer,
                 const gfx::IntSize& aIntrinsicSize,
                 const gfx::IntSize& aOutputSize,
                 DecoderFlags aDecoderFlags,
-                SurfaceFlags aSurfaceFlags);
+                SurfaceFlags aSurfaceFlags,
+                IDecodingTask** aOutTask);
 
   /**
    * Creates and initializes a decoder for animated images of type @aType.
@@ -88,14 +94,20 @@ public:
    * @param aDecoderFlags Flags specifying the behavior of this decoder.
    * @param aSurfaceFlags Flags specifying the type of output this decoder
    *                      should produce.
+   * @param aOutTask Task representing the decoder.
+   * @return NS_OK if the decoder has been created/initialized successfully;
+   *         NS_ERROR_ALREADY_INITIALIZED if there is already an active decoder
+   *           for this image;
+   *         Else some other unrecoverable error occurred.
    */
-  static already_AddRefed<IDecodingTask>
+  static nsresult
   CreateAnimationDecoder(DecoderType aType,
                          NotNull<RasterImage*> aImage,
                          NotNull<SourceBuffer*> aSourceBuffer,
                          const gfx::IntSize& aIntrinsicSize,
                          DecoderFlags aDecoderFlags,
-                         SurfaceFlags aSurfaceFlags);
+                         SurfaceFlags aSurfaceFlags,
+                         IDecodingTask** aOutTask);
 
   /**
    * Creates and initializes a metadata decoder of type @aType. This decoder

image/RasterImage.cpp

@@ -1249,10 +1249,29 @@ RasterImage::Decode(const IntSize& aSize,
 
   // Create a decoder.
   RefPtr<IDecodingTask> task;
-  if (mAnimationState && aPlaybackType == PlaybackType::eAnimated) {
-    task = DecoderFactory::CreateAnimationDecoder(mDecoderType, WrapNotNull(this),
-                                                  mSourceBuffer, mSize,
-                                                  decoderFlags, surfaceFlags);
+  nsresult rv;
+  bool animated = mAnimationState && aPlaybackType == PlaybackType::eAnimated;
+  if (animated) {
+    rv = DecoderFactory::CreateAnimationDecoder(mDecoderType, WrapNotNull(this),
+                                                mSourceBuffer, mSize,
+                                                decoderFlags, surfaceFlags,
+                                                getter_AddRefs(task));
+  } else {
+    rv = DecoderFactory::CreateDecoder(mDecoderType, WrapNotNull(this),
+                                       mSourceBuffer, mSize, aSize,
+                                       decoderFlags, surfaceFlags,
+                                       getter_AddRefs(task));
+  }
+
+  if (rv == NS_ERROR_ALREADY_INITIALIZED) {
+    // We raced with an already pending decoder, and it finished before we
+    // managed to insert the new decoder. Pretend we did a sync call to make
+    // the caller lookup in the surface cache again.
+    MOZ_ASSERT(!task);
+    return true;
+  }
+
+  if (animated) {
     // We pass false for aAllowInvalidation because we may be asked to use
     // async notifications. Any potential invalidation here will be sent when
     // RequestRefresh is called, or NotifyDecodeComplete.
@@ -1261,17 +1280,15 @@ RasterImage::Decode(const IntSize& aSize,
 #endif
       mAnimationState->UpdateState(mAnimationFinished, this, mSize, false);
     MOZ_ASSERT(rect.IsEmpty());
-  } else {
-    task = DecoderFactory::CreateDecoder(mDecoderType, WrapNotNull(this),
-                                         mSourceBuffer, mSize, aSize,
-                                         decoderFlags, surfaceFlags);
   }
 
   // Make sure DecoderFactory was able to create a decoder successfully.
-  if (!task) {
+  if (NS_FAILED(rv)) {
+    MOZ_ASSERT(!task);
     return false;
   }
 
+  MOZ_ASSERT(task);
   mDecodeCount++;
 
   // We're ready to decode; start the decoder.