Bug 1443232. Don't insert frames into our AnimationFrameBuffer that we consider in error and unusable. r=aosmond

After decoding the first frame we allocate the second frame, but before it finishes we encounter an error, Decoder::PostError is called it aborts the second frame and decrements the frame count. But AnimationSurfaceProvider::CheckForFrameAtTerminalState just asks for the current frame ref from the decoder (which it never cleared) and inserts that.

The condition that we use from the decoder to decide to report a new frame is mFinishedNewFrame (via TakeCompleteFrameCount), however this doesn't directly correspond to mFrameCount. So we create a new bool on the Decoder to track when there is a frame that we can take.

This didn't cause any problems before but now we have tighter coupling between the list of frames the AnimationSurfaceProvider has and what FrameAnimator expects.

Another possible fix would be to clear the current frame ref in PostError, but the only place we clear the current frame is when we allocate the new frame and we have the mImageData pointer still around that decoders could theorhetically use to do final processing on the last partial frame.

image/AnimationSurfaceProvider.cpp

@@ -267,6 +267,9 @@ AnimationSurfaceProvider::CheckForNewFrameAtYield()
 
     // Try to get the new frame from the decoder.
     RawAccessFrameRef frame = mDecoder->GetCurrentFrameRef();
+    MOZ_ASSERT(mDecoder->HasFrameToTake());
+    mDecoder->ClearHasFrameToTake();
+
     if (!frame) {
       MOZ_ASSERT_UNREACHABLE("Decoder yielded but didn't produce a frame?");
       return true;
@@ -309,6 +312,18 @@ AnimationSurfaceProvider::CheckForNewFrameAtTerminalState()
     // The decoder may or may not have a new frame for us at this point. Avoid
     // reinserting the same frame again.
     RawAccessFrameRef frame = mDecoder->GetCurrentFrameRef();
+
+    // If the decoder didn't finish a new frame (ie if, after starting the
+    // frame, it got an error and aborted the frame and the rest of the decode)
+    // that means it won't be reporting it to the image or FrameAnimator so we
+    // should ignore it too, that's what HasFrameToTake tracks basically.
+    if (!mDecoder->HasFrameToTake()) {
+      frame = RawAccessFrameRef();
+    } else {
+      MOZ_ASSERT(frame);
+      mDecoder->ClearHasFrameToTake();
+    }
+
     if (!frame || (!mFrames.Frames().IsEmpty() &&
                    mFrames.Frames().LastElement().get() == frame.get())) {
       return mFrames.MarkComplete();

image/Decoder.cpp

@@ -63,6 +63,7 @@ Decoder::Decoder(RasterImage* aImage)
   , mHaveExplicitOutputSize(false)
   , mInFrame(false)
   , mFinishedNewFrame(false)
+  , mHasFrameToTake(false)
   , mReachedTerminalState(false)
   , mDecodeDone(false)
   , mError(false)
@@ -293,6 +294,8 @@ Decoder::AllocateFrame(uint32_t aFrameNum,
                                         mCurrentFrame.get());
 
   if (mCurrentFrame) {
+    mHasFrameToTake = true;
+
     // Gather the raw pointers the decoders will use.
     mCurrentFrame->GetImageData(&mImageData, &mImageDataLength);
     mCurrentFrame->GetPaletteData(&mColormap, &mColormapSize);
@@ -532,6 +535,7 @@ Decoder::PostError()
     mCurrentFrame->Abort();
     mInFrame = false;
     --mFrameCount;
+    mHasFrameToTake = false;
   }
 }
 

image/Decoder.h

@@ -407,6 +407,11 @@ public:
                          : RawAccessFrameRef();
   }
 
+  bool HasFrameToTake() const { return mHasFrameToTake; }
+  void ClearHasFrameToTake() {
+    MOZ_ASSERT(mHasFrameToTake);
+    mHasFrameToTake = false;
+  }
 
 protected:
   friend class AutoRecordDecoderTelemetry;
@@ -580,6 +585,10 @@ private:
   bool mInFrame : 1;
   bool mFinishedNewFrame : 1;  // True if PostFrameStop() has been called since
                                // the last call to TakeCompleteFrameCount().
+  // Has a new frame that AnimationSurfaceProvider can take. Unfortunately this
+  // has to be separate from mFinishedNewFrame because the png decoder yields a
+  // new frame before calling PostFrameStop().
+  bool mHasFrameToTake : 1;
   bool mReachedTerminalState : 1;
   bool mDecodeDone : 1;
   bool mError : 1;

image/test/reftest/generic/1443232-1.html

@@ -0,0 +1,30 @@
+<!DOCTYPE html>
+<html class="reftest-wait">
+<body>
+<img id='m1'><br/>
+<script>
+  var im1=document.getElementById('m1');
+  var step_state=0;
+  function handle_step(){
+    step_state+=1;
+    if(step_state == 1){
+      im1.src='data:image/gif;base64,R0lGODlhAQABAIABAP///wAAACwAAAAAAQABAAACAkQBADs=';
+    }
+    else if(step_state==2){
+      im1.src='1443232-1.gif';
+    }
+    else if(step_state==3){
+      if(im1.height==2)
+        im1.height=1;
+      im1.height=2;
+      im1.getBoundingClientRect();
+      setTimeout(function(){document.documentElement.classList.remove("reftest-wait");}, 1000);
+    }
+  }
+  document.addEventListener('DOMContentLoaded', function(){
+    im1.addEventListener('load', handle_step, false);
+    im1.src='data:image/gif;base64,R0lGODlhAQABAAAAACH5BAEKAAEALAAAAAABAAEAAAICTAEAOw==';
+  });
+</script>
+</body>
+</html>

image/test/reftest/generic/reftest.list

@@ -1 +1,2 @@
 HTTP == accept-image-catchall.html accept-image-catchall-ref.html
+pref(image.downscale-during-decode.enabled,true) == 1443232-1.html about:blank