Bug 1444537 - Part 3. Fix how redecode errors could cause animated image state inconsistencies. r=tnikkel
authorAndrew Osmond <aosmond@mozilla.com>
Fri, 13 Apr 2018 10:58:52 -0400
changeset 466776 07f5d48b90cb5f65e55ded783254ad5fa6fe2beb
parent 466775 f38a91679fbb75103caa12af07a1b30914c86963
child 466777 0d23d74448c8493ac96828ee757f0211c47a7b44
push id9165
push userasasaki@mozilla.com
push dateThu, 26 Apr 2018 21:04:54 +0000
treeherdermozilla-beta@064c3804de2e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerstnikkel
bugs1444537
milestone61.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 1444537 - Part 3. Fix how redecode errors could cause animated image state inconsistencies. r=tnikkel We can discard frames from an animated image if the memory footprint exceeds the threshold. This will cause us to redecode frames on demand instead. However decoders can fail to produce the same results on subsequent runs due to differences in memory pressure, etc. If this happens our state can get inconsistent. In particular, if we keep failing on the first frame, we end up in an infinite loop on the decoder thread. Since we don't have the owning image to signal, as we had to release our reference to it after the first pass, we can do little but stop decoding. From the user's perspective, the animation will come to a stop.
image/AnimationFrameBuffer.cpp
image/AnimationFrameBuffer.h
image/AnimationSurfaceProvider.cpp
--- a/image/AnimationFrameBuffer.cpp
+++ b/image/AnimationFrameBuffer.cpp
@@ -12,16 +12,17 @@ namespace image {
 AnimationFrameBuffer::AnimationFrameBuffer()
   : mThreshold(0)
   , mBatch(0)
   , mPending(0)
   , mAdvance(0)
   , mInsertIndex(0)
   , mGetIndex(0)
   , mSizeKnown(false)
+  , mRedecodeError(false)
 { }
 
 void
 AnimationFrameBuffer::Initialize(size_t aThreshold,
                                  size_t aBatch,
                                  size_t aStartFrame)
 {
   MOZ_ASSERT(mThreshold == 0);
@@ -66,17 +67,26 @@ AnimationFrameBuffer::Insert(RawAccessFr
   // We should only insert new frames if we actually asked for them.
   MOZ_ASSERT(mPending > 0);
 
   if (mSizeKnown) {
     // We only insert after the size is known if we are repeating the animation
     // and we did not keep all of the frames. Replace whatever is there
     // (probably an empty frame) with the new frame.
     MOZ_ASSERT(MayDiscard());
-    MOZ_ASSERT(mInsertIndex < mFrames.Length());
+
+    // The first decode produced fewer frames than the redecodes, presumably
+    // because it hit an out-of-memory error which later attempts avoided. Just
+    // stop the animation because we can't tell the image that we have more
+    // frames now.
+    if (mInsertIndex >= mFrames.Length()) {
+      mRedecodeError = true;
+      mPending = 0;
+      return false;
+    }
 
     if (mInsertIndex > 0) {
       MOZ_ASSERT(!mFrames[mInsertIndex]);
       mFrames[mInsertIndex] = Move(aFrame);
     }
   } else if (mInsertIndex == mFrames.Length()) {
     // We are still on the first pass of the animation decoding, so this is
     // the first time we have seen this frame.
@@ -122,19 +132,33 @@ AnimationFrameBuffer::Insert(RawAccessFr
     --mAdvance;
   }
   return continueDecoding;
 }
 
 bool
 AnimationFrameBuffer::MarkComplete()
 {
+  // We may have stopped decoding at a different point in the animation than we
+  // did previously. That means the decoder likely hit a new error, e.g. OOM.
+  // This will prevent us from advancing as well, because we are missing the
+  // required frames to blend.
+  //
+  // XXX(aosmond): In an ideal world, we would be generating full frames, and
+  // the consumer of our data doesn't care about our internal state. It simply
+  // knows about the first frame, the current frame, and how long to display the
+  // current frame.
+  if (NS_WARN_IF(mInsertIndex != mFrames.Length())) {
+    MOZ_ASSERT(mSizeKnown);
+    mRedecodeError = true;
+    mPending = 0;
+  }
+
   // We reached the end of the animation, the next frame we get, if we get
   // another, will be the first frame again.
-  MOZ_ASSERT(mInsertIndex == mFrames.Length());
   mInsertIndex = 0;
 
   // Since we only request advancing when we want to resume at a certain point
   // in the animation, we should never exceed the number of frames.
   MOZ_ASSERT(mAdvance == 0);
 
   if (!mSizeKnown) {
     // We just received the last frame in the animation. Compact the frame array
@@ -226,17 +250,17 @@ AnimationFrameBuffer::AdvanceInternal()
     if (mGetIndex > 1) {
       discard = Move(mFrames[mGetIndex - 1]);
     } else if (mGetIndex == 0) {
       MOZ_ASSERT(mSizeKnown && framesLength > 1);
       discard = Move(mFrames[framesLength - 1]);
     }
   }
 
-  if (!mSizeKnown || MayDiscard()) {
+  if (!mRedecodeError && (!mSizeKnown || MayDiscard())) {
     // Calculate how many frames we have requested ahead of the current frame.
     size_t buffered = mPending;
     if (mGetIndex > mInsertIndex) {
       // It wrapped around and we are decoding the beginning again before the
       // the display has finished the loop.
       MOZ_ASSERT(mSizeKnown);
       buffered += mInsertIndex + framesLength - mGetIndex - 1;
     } else {
@@ -271,27 +295,30 @@ AnimationFrameBuffer::Reset()
       mPending = 1;
     }
 
     // Either the decoder is still running, or we have enough frames already.
     // No need for us to restart it.
     return false;
   }
 
-  // If we are over the threshold, then we know we will have missing frames in
-  // our buffer. The easiest thing to do is to drop everything but the first
-  // frame and go back to the initial state.
-  bool restartDecoder = mPending == 0;
-  mInsertIndex = 0;
-  mPending = 2 * mBatch;
-
   // Discard all frames besides the first, because the decoder always expects
   // that when it re-inserts a frame, it is not present. (It doesn't re-insert
   // the first frame.)
   for (size_t i = 1; i < mFrames.Length(); ++i) {
     RawAccessFrameRef discard = Move(mFrames[i]);
   }
 
+  mInsertIndex = 0;
+
+  // If we hit an error after redecoding, we never want to restart decoding.
+  if (mRedecodeError) {
+    MOZ_ASSERT(mPending == 0);
+    return false;
+  }
+
+  bool restartDecoder = mPending == 0;
+  mPending = 2 * mBatch;
   return restartDecoder;
 }
 
 } // namespace image
 } // namespace mozilla
--- a/image/AnimationFrameBuffer.h
+++ b/image/AnimationFrameBuffer.h
@@ -125,16 +125,22 @@ public:
   /**
    * @returns True if the frame buffer was ever marked as complete. This implies
    *          that the total number of frames is known and may be gotten from
    *          Frames().Length().
    */
   bool SizeKnown() const { return mSizeKnown; }
 
   /**
+   * @returns True if encountered an error during redecode which should cause
+   *          the caller to stop inserting frames.
+   */
+  bool HasRedecodeError() const { return mRedecodeError; }
+
+  /**
    * @returns The current frame index we have advanced to.
    */
   size_t Displayed() const { return mGetIndex; }
 
   /**
    * @returns Outstanding frames desired from the decoder.
    */
   size_t PendingDecode() const { return mPending; }
@@ -182,14 +188,17 @@ private:
   // The mFrames index in which to insert the next decoded frame.
   size_t mInsertIndex;
 
   // The mFrames index that we have advanced to.
   size_t mGetIndex;
 
   // True if the total number of frames is known.
   bool mSizeKnown;
+
+  // True if we encountered an error while redecoding.
+  bool mRedecodeError;
 };
 
 } // namespace image
 } // namespace mozilla
 
 #endif // mozilla_image_AnimationFrameBuffer_h
--- a/image/AnimationSurfaceProvider.cpp
+++ b/image/AnimationSurfaceProvider.cpp
@@ -219,40 +219,45 @@ AnimationSurfaceProvider::Run()
       // failure or truncated data may mean that no new frame got produced.
       // Since we're not sure, rather than call CheckForNewFrameAtYield() here
       // we call CheckForNewFrameAtTerminalState(), which handles both of these
       // possibilities.
       bool continueDecoding = CheckForNewFrameAtTerminalState();
       FinishDecoding();
 
       // Even if it is the last frame, we may not have enough frames buffered
-      // ahead of the current.
-      if (continueDecoding) {
-        MOZ_ASSERT(mDecoder);
-        continue;
+      // ahead of the current. If we are shutting down, we want to ensure we
+      // release the thread as soon as possible. The animation may advance even
+      // during shutdown, which keeps us decoding, and thus blocking the decode
+      // pool during teardown.
+      if (!mDecoder || !continueDecoding ||
+          DecodePool::Singleton()->IsShuttingDown()) {
+        return;
       }
-
-      return;
     }
 
     // Notify for the progress we've made so far.
     if (mImage && mDecoder->HasProgress()) {
       NotifyProgress(WrapNotNull(mImage), WrapNotNull(mDecoder));
     }
 
     if (result == LexerResult(Yield::NEED_MORE_DATA)) {
       // We can't make any more progress right now. The decoder itself will ensure
       // that we get reenqueued when more data is available; just return for now.
       return;
     }
 
     // There's new output available - a new frame! Grab it. If we don't need any
-    // more for the moment we can break out of the loop.
+    // more for the moment we can break out of the loop. If we are shutting
+    // down, we want to ensure we release the thread as soon as possible. The
+    // animation may advance even during shutdown, which keeps us decoding, and
+    // thus blocking the decode pool during teardown.
     MOZ_ASSERT(result == LexerResult(Yield::OUTPUT_AVAILABLE));
-    if (!CheckForNewFrameAtYield()) {
+    if (!CheckForNewFrameAtYield() ||
+        DecodePool::Singleton()->IsShuttingDown()) {
       return;
     }
   }
 }
 
 bool
 AnimationSurfaceProvider::CheckForNewFrameAtYield()
 {
@@ -289,20 +294,17 @@ AnimationSurfaceProvider::CheckForNewFra
       justGotFirstFrame = true;
     }
   }
 
   if (justGotFirstFrame) {
     AnnounceSurfaceAvailable();
   }
 
-  // If we are shutting down, we want to ensure we release the thread as soon
-  // as possible. The animation may advance even during shutdown, which keeps
-  // us decoding, and thus blocking the decode pool during teardown.
-  return continueDecoding && !DecodePool::Singleton()->IsShuttingDown();
+  return continueDecoding;
 }
 
 bool
 AnimationSurfaceProvider::CheckForNewFrameAtTerminalState()
 {
   mDecodingMutex.AssertCurrentThreadOwns();
   MOZ_ASSERT(mDecoder);
 
@@ -342,20 +344,17 @@ AnimationSurfaceProvider::CheckForNewFra
       justGotFirstFrame = true;
     }
   }
 
   if (justGotFirstFrame) {
     AnnounceSurfaceAvailable();
   }
 
-  // If we are shutting down, we want to ensure we release the thread as soon
-  // as possible. The animation may advance even during shutdown, which keeps
-  // us decoding, and thus blocking the decode pool during teardown.
-  return continueDecoding && !DecodePool::Singleton()->IsShuttingDown();
+  return continueDecoding;
 }
 
 void
 AnimationSurfaceProvider::AnnounceSurfaceAvailable()
 {
   mFramesMutex.AssertNotCurrentThreadOwns();
   MOZ_ASSERT(mImage);
 
@@ -373,25 +372,25 @@ AnimationSurfaceProvider::FinishDecoding
   mDecodingMutex.AssertCurrentThreadOwns();
   MOZ_ASSERT(mDecoder);
 
   if (mImage) {
     // Send notifications.
     NotifyDecodeComplete(WrapNotNull(mImage), WrapNotNull(mDecoder));
   }
 
-  // Destroy our decoder; we don't need it anymore.
-  bool mayDiscard;
+  // Determine if we need to recreate the decoder, in case we are discarding
+  // frames and need to loop back to the beginning.
+  bool recreateDecoder;
   {
     MutexAutoLock lock(mFramesMutex);
-    mayDiscard = mFrames.MayDiscard();
+    recreateDecoder = !mFrames.HasRedecodeError() && mFrames.MayDiscard();
   }
 
-  if (mayDiscard) {
-    // Recreate the decoder so we can regenerate the frames again.
+  if (recreateDecoder) {
     mDecoder = DecoderFactory::CloneAnimationDecoder(mDecoder);
     MOZ_ASSERT(mDecoder);
   } else {
     mDecoder = nullptr;
   }
 
   // We don't need a reference to our image anymore, either, and we don't want
   // one. We may be stored in the surface cache for a long time after decoding