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

@@ -17,6 +17,7 @@ AnimationFrameBuffer::AnimationFrameBuffer()
   , mInsertIndex(0)
   , mGetIndex(0)
   , mSizeKnown(false)
+  , mRedecodeError(false)
 { }
 
 void
@@ -71,7 +72,16 @@ AnimationFrameBuffer::Insert(RawAccessFrameRef&& aFrame)
     // 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]);
@@ -127,9 +137,23 @@ AnimationFrameBuffer::Insert(RawAccessFrameRef&& aFrame)
 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
@@ -231,7 +255,7 @@ AnimationFrameBuffer::AdvanceInternal()
     }
   }
 
-  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) {
@@ -276,13 +300,6 @@ AnimationFrameBuffer::Reset()
     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.)
@@ -290,6 +307,16 @@ AnimationFrameBuffer::Reset()
     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;
 }
 

image/AnimationFrameBuffer.h

@@ -129,6 +129,12 @@ public:
    */
   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.
    */
@@ -187,6 +193,9 @@ private:
 
   // True if the total number of frames is known.
   bool mSizeKnown;
+
+  // True if we encountered an error while redecoding.
+  bool mRedecodeError;
 };
 
 } // namespace image

image/AnimationSurfaceProvider.cpp

@@ -224,13 +224,17 @@ AnimationSurfaceProvider::Run()
       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;
+      // Restart from the very beginning because the decoder was recreated.
+      continue;
     }
 
     // Notify for the progress we've made so far.
@@ -245,9 +249,13 @@ AnimationSurfaceProvider::Run()
     }
 
     // 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;
     }
   }
@@ -294,10 +302,7 @@ AnimationSurfaceProvider::CheckForNewFrameAtYield()
     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
@@ -347,10 +352,7 @@ AnimationSurfaceProvider::CheckForNewFrameAtTerminalState()
     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
@@ -378,15 +380,15 @@ AnimationSurfaceProvider::FinishDecoding()
     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 {