Bug 1389988 - Handle a data race between a new sync decode request and a pending decoder. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Fri, 09 Feb 2018 08:51:28 -0500
changeset 455587 e8e9b5a102ac972376fca48322e5f8a2d3b248ca
parent 455586 f3eb9b35ee7ea8856d8bfb175dd4a69d5fde7319
child 455588 a44d31cec7286416d0680f3f6372d16a7b398a35
push id8799
push usermtabara@mozilla.com
push dateThu, 01 Mar 2018 16:46:23 +0000
treeherdermozilla-beta@15334014dc67 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1389988
milestone60.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
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
image/DecoderFactory.h
image/RasterImage.cpp
--- a/image/DecoderFactory.cpp
+++ b/image/DecoderFactory.cpp
@@ -104,113 +104,129 @@ DecoderFactory::GetDecoder(DecoderType a
       break;
     default:
       MOZ_ASSERT_UNREACHABLE("Unknown decoder type");
   }
 
   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
   // owning RasterImage will be mediated by DecodedSurfaceProvider.
   RefPtr<Decoder> decoder =
     GetDecoder(aType, nullptr, bool(aDecoderFlags & DecoderFlags::IS_REDECODE));
   MOZ_ASSERT(decoder, "Should have a decoder now");
 
   // Initialize the decoder.
   decoder->SetMetadataDecode(false);
   decoder->SetIterator(aSourceBuffer->Iterator());
   decoder->SetOutputSize(aOutputSize);
   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
   // make this decoder's output available in the surface cache.
   SurfaceKey surfaceKey =
     RasterSurfaceKey(aOutputSize, aSurfaceFlags, PlaybackType::eStatic);
   auto provider = MakeNotNull<RefPtr<DecodedSurfaceProvider>>(
     aImage, surfaceKey, WrapNotNull(decoder));
   if (aDecoderFlags & DecoderFlags::CANNOT_SUBSTITUTE) {
     provider->Availability().SetCannotSubstitute();
   }
 
   // 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,
              "Calling CreateAnimationDecoder for non-animating DecoderType");
 
   // Create an anonymous decoder. Interaction with the SurfaceCache and the
   // owning RasterImage will be mediated by AnimationSurfaceProvider.
   RefPtr<Decoder> decoder = GetDecoder(aType, nullptr, /* aIsRedecode = */ true);
   MOZ_ASSERT(decoder, "Should have a decoder now");
 
   // Initialize the decoder.
   decoder->SetMetadataDecode(false);
   decoder->SetIterator(aSourceBuffer->Iterator());
   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
   // and make this decoder's output available in the surface cache.
   SurfaceKey surfaceKey =
     RasterSurfaceKey(aIntrinsicSize, aSurfaceFlags, PlaybackType::eAnimated);
   auto provider = MakeNotNull<RefPtr<AnimationSurfaceProvider>>(
     aImage, surfaceKey, WrapNotNull(decoder));
 
   // 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>
 DecoderFactory::CreateMetadataDecoder(DecoderType aType,
                                       NotNull<RasterImage*> aImage,
                                       NotNull<SourceBuffer*> aSourceBuffer)
 {
   if (aType == DecoderType::UNKNOWN) {
--- a/image/DecoderFactory.h
+++ b/image/DecoderFactory.h
@@ -59,48 +59,60 @@ public:
    * @param aIntrinsicSize The intrinsic size of the image, normally obtained
    *                       during the metadata decode.
    * @param aOutputSize The output size for the decoder. If this is smaller than
    *                    the intrinsic size, the decoder will downscale the
    *                    image.
    * @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.
    * The decoder will send notifications to @aImage.
    *
    * @param aType Which type of decoder to create - JPEG, PNG, etc.
    * @param aImage The image will own the decoder and which should receive
    *               notifications as decoding progresses.
    * @param aSourceBuffer The SourceBuffer which the decoder will read its data
    *                      from.
    * @param aIntrinsicSize The intrinsic size of the image, normally obtained
    *                       during the metadata decode.
    * @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
    * will only decode the image's header, extracting metadata like the size of
    * the image. No actual image data will be decoded and no surfaces will be
    * allocated. The decoder will send notifications to @aImage.
    *
    * @param aType Which type of decoder to create - JPEG, PNG, etc.
--- a/image/RasterImage.cpp
+++ b/image/RasterImage.cpp
@@ -1244,39 +1244,56 @@ RasterImage::Decode(const IntSize& aSize
   if (IsOpaque()) {
     // If there's no transparency, it doesn't matter whether we premultiply
     // alpha or not.
     surfaceFlags &= ~SurfaceFlags::NO_PREMULTIPLY_ALPHA;
   }
 
   // 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.
 #ifdef DEBUG
     gfx::IntRect rect =
 #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.
   return LaunchDecodingTask(task, this, aFlags, mAllSourceData);
 }
 
 NS_IMETHODIMP
 RasterImage::DecodeMetadata(uint32_t aFlags)