Backed out 2 changesets (bug 1444537) for causing crashes on test_discardFramesAnimatedImage.html. a=backout
authorCosmin Sabou <csabou@mozilla.com>
Sun, 15 Apr 2018 02:40:30 +0300
changeset 466915 a0c455e036df741556d695cf46baeacc581c0c58
parent 466914 008f977fc4ac4fc09b98580ff4355b03c14f1e19
child 466916 a79d460bf2a33fd79c6646236f9c4df78e66e7b7
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)
reviewersbackout
bugs1444537
milestone61.0a1
backs out0d23d74448c8493ac96828ee757f0211c47a7b44
07f5d48b90cb5f65e55ded783254ad5fa6fe2beb
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
Backed out 2 changesets (bug 1444537) for causing crashes on test_discardFramesAnimatedImage.html. a=backout Backed out changeset 0d23d74448c8 (bug 1444537) Backed out changeset 07f5d48b90cb (bug 1444537)
image/AnimationFrameBuffer.cpp
image/AnimationFrameBuffer.h
image/AnimationSurfaceProvider.cpp
image/test/gtest/TestAnimationFrameBuffer.cpp
--- a/image/AnimationFrameBuffer.cpp
+++ b/image/AnimationFrameBuffer.cpp
@@ -12,17 +12,16 @@ 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);
@@ -67,26 +66,17 @@ 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());
-
-    // 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;
-    }
+    MOZ_ASSERT(mInsertIndex < mFrames.Length());
 
     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.
@@ -132,33 +122,19 @@ 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
@@ -250,17 +226,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 (!mRedecodeError && (!mSizeKnown || MayDiscard())) {
+  if (!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 {
@@ -295,30 +271,27 @@ 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,22 +125,16 @@ 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; }
@@ -188,17 +182,14 @@ 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,45 +219,40 @@ 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 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;
+      // ahead of the current.
+      if (continueDecoding) {
+        MOZ_ASSERT(mDecoder);
+        continue;
       }
+
+      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. 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.
+    // more for the moment we can break out of the loop.
     MOZ_ASSERT(result == LexerResult(Yield::OUTPUT_AVAILABLE));
-    if (!CheckForNewFrameAtYield() ||
-        DecodePool::Singleton()->IsShuttingDown()) {
+    if (!CheckForNewFrameAtYield()) {
       return;
     }
   }
 }
 
 bool
 AnimationSurfaceProvider::CheckForNewFrameAtYield()
 {
@@ -294,17 +289,20 @@ AnimationSurfaceProvider::CheckForNewFra
       justGotFirstFrame = true;
     }
   }
 
   if (justGotFirstFrame) {
     AnnounceSurfaceAvailable();
   }
 
-  return continueDecoding;
+  // 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();
 }
 
 bool
 AnimationSurfaceProvider::CheckForNewFrameAtTerminalState()
 {
   mDecodingMutex.AssertCurrentThreadOwns();
   MOZ_ASSERT(mDecoder);
 
@@ -344,17 +342,20 @@ AnimationSurfaceProvider::CheckForNewFra
       justGotFirstFrame = true;
     }
   }
 
   if (justGotFirstFrame) {
     AnnounceSurfaceAvailable();
   }
 
-  return continueDecoding;
+  // 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();
 }
 
 void
 AnimationSurfaceProvider::AnnounceSurfaceAvailable()
 {
   mFramesMutex.AssertNotCurrentThreadOwns();
   MOZ_ASSERT(mImage);
 
@@ -372,25 +373,25 @@ AnimationSurfaceProvider::FinishDecoding
   mDecodingMutex.AssertCurrentThreadOwns();
   MOZ_ASSERT(mDecoder);
 
   if (mImage) {
     // Send notifications.
     NotifyDecodeComplete(WrapNotNull(mImage), WrapNotNull(mDecoder));
   }
 
-  // Determine if we need to recreate the decoder, in case we are discarding
-  // frames and need to loop back to the beginning.
-  bool recreateDecoder;
+  // Destroy our decoder; we don't need it anymore.
+  bool mayDiscard;
   {
     MutexAutoLock lock(mFramesMutex);
-    recreateDecoder = !mFrames.HasRedecodeError() && mFrames.MayDiscard();
+    mayDiscard = mFrames.MayDiscard();
   }
 
-  if (recreateDecoder) {
+  if (mayDiscard) {
+    // Recreate the decoder so we can regenerate the frames again.
     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
--- a/image/test/gtest/TestAnimationFrameBuffer.cpp
+++ b/image/test/gtest/TestAnimationFrameBuffer.cpp
@@ -138,17 +138,16 @@ TEST_F(ImageAnimationFrameBuffer, Finish
     EXPECT_FALSE(buffer.SizeKnown());
 
     if (i == 4) {
       EXPECT_EQ(size_t(15), buffer.PendingDecode());
       keepDecoding = buffer.MarkComplete();
       EXPECT_FALSE(keepDecoding);
       EXPECT_TRUE(buffer.SizeKnown());
       EXPECT_EQ(size_t(0), buffer.PendingDecode());
-      EXPECT_FALSE(buffer.HasRedecodeError());
     }
 
     EXPECT_FALSE(buffer.MayDiscard());
 
     DrawableFrameRef gotFrame = buffer.Get(i);
     EXPECT_EQ(frame.get(), gotFrame.get());
     ASSERT_EQ(i + 1, frames.Length());
     EXPECT_EQ(frame.get(), frames[i].get());
@@ -216,17 +215,16 @@ TEST_F(ImageAnimationFrameBuffer, Finish
   // Add the last frame.
   keepDecoding = buffer.Insert(CreateEmptyFrame());
   EXPECT_TRUE(keepDecoding);
   keepDecoding = buffer.MarkComplete();
   EXPECT_FALSE(keepDecoding);
   EXPECT_TRUE(buffer.SizeKnown());
   EXPECT_EQ(size_t(0), buffer.PendingDecode());
   EXPECT_EQ(size_t(5), frames.Length());
-  EXPECT_FALSE(buffer.HasRedecodeError());
 
   // Finish progressing through the animation.
   for ( ; i < frames.Length(); ++i) {
     DrawableFrameRef gotFrame = buffer.Get(i);
     EXPECT_TRUE(gotFrame);
     restartDecoder = buffer.AdvanceTo(i);
     EXPECT_FALSE(restartDecoder);
   }
@@ -327,17 +325,16 @@ TEST_F(ImageAnimationFrameBuffer, MayDis
   // frame will restart at the beginning.
   keepDecoding = buffer.Insert(CreateEmptyFrame());
   EXPECT_TRUE(keepDecoding);
   keepDecoding = buffer.MarkComplete();
   EXPECT_TRUE(keepDecoding);
   EXPECT_TRUE(buffer.SizeKnown());
   EXPECT_EQ(size_t(2), buffer.PendingDecode());
   EXPECT_EQ(size_t(10), frames.Length());
-  EXPECT_FALSE(buffer.HasRedecodeError());
 
   // Use remaining pending room. It shouldn't add new frames, only replace.
   do {
     keepDecoding = buffer.Insert(CreateEmptyFrame());
   } while (keepDecoding);
 
   EXPECT_EQ(size_t(0), buffer.PendingDecode());
   EXPECT_EQ(size_t(10), frames.Length());
@@ -511,164 +508,8 @@ TEST_F(ImageAnimationFrameBuffer, StartA
   // in the real scenario, the decoder thread is still running and it is easier
   // to let it insert its last frame than to coordinate quitting earlier.
   buffer.Reset();
   EXPECT_EQ(size_t(0), buffer.Displayed());
   EXPECT_EQ(size_t(1), buffer.PendingDecode());
   EXPECT_EQ(size_t(0), buffer.PendingAdvance());
 }
 
-TEST_F(ImageAnimationFrameBuffer, RedecodeMoreFrames)
-{
-  const size_t kThreshold = 5;
-  const size_t kBatch = 2;
-  AnimationFrameBuffer buffer;
-  buffer.Initialize(kThreshold, kBatch, 0);
-  const auto& frames = buffer.Frames();
-
-  // Add frames until we exceed the threshold.
-  bool keepDecoding;
-  bool restartDecoder;
-  size_t i = 0;
-  do {
-    keepDecoding = buffer.Insert(CreateEmptyFrame());
-    EXPECT_TRUE(keepDecoding);
-    if (i > 0) {
-      restartDecoder = buffer.AdvanceTo(i);
-      EXPECT_FALSE(restartDecoder);
-    }
-    ++i;
-  } while (!buffer.MayDiscard());
-
-  // Should have threshold + 1 frames, and still not complete.
-  EXPECT_EQ(size_t(6), frames.Length());
-  EXPECT_FALSE(buffer.SizeKnown());
-
-  // Now we lock in at 6 frames.
-  keepDecoding = buffer.MarkComplete();
-  EXPECT_TRUE(keepDecoding);
-  EXPECT_TRUE(buffer.SizeKnown());
-  EXPECT_FALSE(buffer.HasRedecodeError());
-
-  // Reinsert 6 frames first.
-  i = 0;
-  do {
-    keepDecoding = buffer.Insert(CreateEmptyFrame());
-    EXPECT_TRUE(keepDecoding);
-    restartDecoder = buffer.AdvanceTo(i);
-    EXPECT_FALSE(restartDecoder);
-    ++i;
-  } while (i < 6);
-
-  // We should now encounter an error and shutdown further decodes.
-  keepDecoding = buffer.Insert(CreateEmptyFrame());
-  EXPECT_FALSE(keepDecoding);
-  EXPECT_EQ(size_t(0), buffer.PendingDecode());
-  EXPECT_TRUE(buffer.HasRedecodeError());
-}
-
-TEST_F(ImageAnimationFrameBuffer, RedecodeFewerFrames)
-{
-  const size_t kThreshold = 5;
-  const size_t kBatch = 2;
-  AnimationFrameBuffer buffer;
-  buffer.Initialize(kThreshold, kBatch, 0);
-  const auto& frames = buffer.Frames();
-
-  // Add frames until we exceed the threshold.
-  bool keepDecoding;
-  bool restartDecoder;
-  size_t i = 0;
-  do {
-    keepDecoding = buffer.Insert(CreateEmptyFrame());
-    EXPECT_TRUE(keepDecoding);
-    if (i > 0) {
-      restartDecoder = buffer.AdvanceTo(i);
-      EXPECT_FALSE(restartDecoder);
-    }
-    ++i;
-  } while (!buffer.MayDiscard());
-
-  // Should have threshold + 1 frames, and still not complete.
-  EXPECT_EQ(size_t(6), frames.Length());
-  EXPECT_FALSE(buffer.SizeKnown());
-
-  // Now we lock in at 6 frames.
-  keepDecoding = buffer.MarkComplete();
-  EXPECT_TRUE(keepDecoding);
-  EXPECT_TRUE(buffer.SizeKnown());
-  EXPECT_FALSE(buffer.HasRedecodeError());
-
-  // Reinsert 5 frames before marking complete.
-  i = 0;
-  do {
-    keepDecoding = buffer.Insert(CreateEmptyFrame());
-    EXPECT_TRUE(keepDecoding);
-    restartDecoder = buffer.AdvanceTo(i);
-    EXPECT_FALSE(restartDecoder);
-    ++i;
-  } while (i < 5);
-
-  // We should now encounter an error and shutdown further decodes.
-  keepDecoding = buffer.MarkComplete();
-  EXPECT_FALSE(keepDecoding);
-  EXPECT_EQ(size_t(0), buffer.PendingDecode());
-  EXPECT_TRUE(buffer.HasRedecodeError());
-}
-
-TEST_F(ImageAnimationFrameBuffer, RedecodeFewerFramesAndBehindAdvancing)
-{
-  const size_t kThreshold = 5;
-  const size_t kBatch = 2;
-  AnimationFrameBuffer buffer;
-  buffer.Initialize(kThreshold, kBatch, 0);
-  const auto& frames = buffer.Frames();
-
-  // Add frames until we exceed the threshold.
-  bool keepDecoding;
-  bool restartDecoder;
-  size_t i = 0;
-  do {
-    keepDecoding = buffer.Insert(CreateEmptyFrame());
-    EXPECT_TRUE(keepDecoding);
-    if (i > 0) {
-      restartDecoder = buffer.AdvanceTo(i);
-      EXPECT_FALSE(restartDecoder);
-    }
-    ++i;
-  } while (!buffer.MayDiscard());
-
-  // Should have threshold + 1 frames, and still not complete.
-  EXPECT_EQ(size_t(6), frames.Length());
-  EXPECT_FALSE(buffer.SizeKnown());
-
-  // Now we lock in at 6 frames.
-  keepDecoding = buffer.MarkComplete();
-  EXPECT_TRUE(keepDecoding);
-  EXPECT_TRUE(buffer.SizeKnown());
-  EXPECT_FALSE(buffer.HasRedecodeError());
-
-  // Reinsert frames without advancing until we exhaust our pending space. This
-  // should be less than the current buffer length by definition.
-  i = 0;
-  do {
-    keepDecoding = buffer.Insert(CreateEmptyFrame());
-    ++i;
-  } while (keepDecoding);
-
-  EXPECT_EQ(size_t(2), i);
-
-  // We should now encounter an error and shutdown further decodes.
-  keepDecoding = buffer.MarkComplete();
-  EXPECT_FALSE(keepDecoding);
-  EXPECT_EQ(size_t(0), buffer.PendingDecode());
-  EXPECT_TRUE(buffer.HasRedecodeError());
-
-  // We should however be able to continue advancing to the last decoded frame
-  // without it requesting the decoder to restart.
-  i = 0;
-  do {
-    restartDecoder = buffer.AdvanceTo(i);
-    EXPECT_FALSE(restartDecoder);
-    ++i;
-  } while (i < 2);
-}
-