Bug 1285867 (Part 2) - Don't call Decoder::PostDataError() from Decoder subclasses. r=edwin
authorSeth Fowler <mark.seth.fowler@gmail.com>
Mon, 11 Jul 2016 00:38:54 -0700
changeset 305088 8c5e700ca1aefa0ff382cbb060b4184a182162ac
parent 305087 77fe4e663e345389c4260ea573dbf58afa85da1b
child 305089 560ca5d7336ece5f1df1ed44a6fc389895040e7b
push id79492
push usermfowler@mozilla.com
push dateFri, 15 Jul 2016 23:41:37 +0000
treeherdermozilla-inbound@401978e000d8 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersedwin
bugs1285867
milestone50.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 1285867 (Part 2) - Don't call Decoder::PostDataError() from Decoder subclasses. r=edwin
image/Decoder.cpp
image/Decoder.h
image/decoders/nsBMPDecoder.cpp
image/decoders/nsBMPDecoder.h
image/decoders/nsGIFDecoder2.cpp
image/decoders/nsGIFDecoder2.h
image/decoders/nsICODecoder.cpp
image/decoders/nsICODecoder.h
image/decoders/nsJPEGDecoder.cpp
image/decoders/nsJPEGDecoder.h
--- a/image/Decoder.cpp
+++ b/image/Decoder.cpp
@@ -172,22 +172,31 @@ Decoder::ShouldSyncDecode(size_t aByteLi
   MOZ_ASSERT(mIterator, "Should have a SourceBufferIterator");
 
   return mIterator->RemainingBytesIsNoMoreThan(aByteLimit);
 }
 
 void
 Decoder::CompleteDecode()
 {
-  // Implementation-specific finalization
-  BeforeFinishInternal();
-  if (!HasError()) {
-    FinishInternal();
-  } else {
-    FinishWithErrorInternal();
+  // Implementation-specific finalization.
+  nsresult rv = BeforeFinishInternal();
+  if (NS_FAILED(rv)) {
+    PostDataError();
+  }
+
+  rv = HasError() ? FinishWithErrorInternal()
+                  : FinishInternal();
+  if (NS_FAILED(rv)) {
+    PostDataError();
+  }
+
+  // If this was a metadata decode and we never got a size, the decode failed.
+  if (IsMetadataDecode() && !HasSize()) {
+    PostDataError();
   }
 
   // If the implementation left us mid-frame, finish that up.
   if (mInFrame && !HasError()) {
     PostFrameStop();
   }
 
   // If PostDecodeDone() has not been called, and this decoder wasn't aborted
@@ -272,18 +281,16 @@ Decoder::AllocateFrame(uint32_t aFrameNu
     if (aFrameNum + 1 == mFrameCount) {
       // If we're past the first frame, PostIsAnimated() should've been called.
       MOZ_ASSERT_IF(mFrameCount > 1, HasAnimation());
 
       // Update our state to reflect the new frame
       MOZ_ASSERT(!mInFrame, "Starting new frame but not done with old one!");
       mInFrame = true;
     }
-  } else {
-    PostDataError();
   }
 
   return mCurrentFrame ? NS_OK : NS_ERROR_FAILURE;
 }
 
 RawAccessFrameRef
 Decoder::AllocateFrameInternal(uint32_t aFrameNum,
                                const nsIntSize& aTargetSize,
@@ -385,19 +392,19 @@ Decoder::AllocateFrameInternal(uint32_t 
   return ref;
 }
 
 /*
  * Hook stubs. Override these as necessary in decoder implementations.
  */
 
 nsresult Decoder::InitInternal() { return NS_OK; }
-void Decoder::BeforeFinishInternal() { }
-void Decoder::FinishInternal() { }
-void Decoder::FinishWithErrorInternal() { }
+nsresult Decoder::BeforeFinishInternal() { return NS_OK; }
+nsresult Decoder::FinishInternal() { return NS_OK; }
+nsresult Decoder::FinishWithErrorInternal() { return NS_OK; }
 
 /*
  * Progress Notifications
  */
 
 void
 Decoder::PostSize(int32_t aWidth,
                   int32_t aHeight,
--- a/image/Decoder.h
+++ b/image/Decoder.h
@@ -282,23 +282,23 @@ protected:
   virtual ~Decoder();
 
   /*
    * Internal hooks. Decoder implementations may override these and
    * only these methods.
    *
    * BeforeFinishInternal() can be used to detect if decoding is in an
    * incomplete state, e.g. due to file truncation, in which case it should
-   * call PostDataError().
+   * return a failing nsresult.
    */
   virtual nsresult InitInternal();
   virtual Maybe<TerminalState> DoDecode(SourceBufferIterator& aIterator) = 0;
-  virtual void BeforeFinishInternal();
-  virtual void FinishInternal();
-  virtual void FinishWithErrorInternal();
+  virtual nsresult BeforeFinishInternal();
+  virtual nsresult FinishInternal();
+  virtual nsresult FinishWithErrorInternal();
 
   /*
    * Progress notifications.
    */
 
   // Called by decoders when they determine the size of the image. Informs
   // the image of its size and sends notifications.
   void PostSize(int32_t aWidth,
--- a/image/decoders/nsBMPDecoder.cpp
+++ b/image/decoders/nsBMPDecoder.cpp
@@ -217,25 +217,27 @@ nsBMPDecoder::GetCompressedImageSize() c
 {
   // In the RGB case mImageSize might not be set, so compute it manually.
   MOZ_ASSERT(mPixelRowSize != 0);
   return mH.mCompression == Compression::RGB
        ? mPixelRowSize * AbsoluteHeight()
        : mH.mImageSize;
 }
 
-void
+nsresult
 nsBMPDecoder::BeforeFinishInternal()
 {
   if (!IsMetadataDecode() && !mImageData) {
-    PostDataError();
+    return NS_ERROR_FAILURE;  // No image; something went wrong.
   }
+
+  return NS_OK;
 }
 
-void
+nsresult
 nsBMPDecoder::FinishInternal()
 {
   // We shouldn't be called in error cases.
   MOZ_ASSERT(!HasError(), "Can't call FinishInternal on error!");
 
   // We should never make multiple frames.
   MOZ_ASSERT(GetFrameCount() <= 1, "Multiple BMP frames?");
 
@@ -263,16 +265,18 @@ nsBMPDecoder::FinishInternal()
     if (mDoesHaveTransparency) {
       MOZ_ASSERT(mMayHaveTransparency);
       PostFrameStop(Opacity::SOME_TRANSPARENCY);
     } else {
       PostFrameStop(Opacity::FULLY_OPAQUE);
     }
     PostDecodeDone();
   }
+
+  return NS_OK;
 }
 
 // ----------------------------------------
 // Actual Data Processing
 // ----------------------------------------
 
 void
 BitFields::Value::Set(uint32_t aMask)
@@ -464,17 +468,16 @@ nsBMPDecoder::DoDecode(SourceBufferItera
 
 LexerTransition<nsBMPDecoder::State>
 nsBMPDecoder::ReadFileHeader(const char* aData, size_t aLength)
 {
   mPreGapLength += aLength;
 
   bool signatureOk = aData[0] == 'B' && aData[1] == 'M';
   if (!signatureOk) {
-    PostDataError();
     return Transition::TerminateFailure();
   }
 
   // We ignore the filesize (aData + 2) and reserved (aData + 6) fields.
 
   mH.mDataOffset = LittleEndian::readUint32(aData + 10);
 
   return Transition::To(State::INFO_HEADER_SIZE, BIHSIZE_FIELD_LENGTH);
@@ -491,17 +494,16 @@ nsBMPDecoder::ReadInfoHeaderSize(const c
 
   bool bihSizeOk = mH.mBIHSize == InfoHeaderLength::WIN_V2 ||
                    mH.mBIHSize == InfoHeaderLength::WIN_V3 ||
                    mH.mBIHSize == InfoHeaderLength::WIN_V4 ||
                    mH.mBIHSize == InfoHeaderLength::WIN_V5 ||
                    (mH.mBIHSize >= InfoHeaderLength::OS2_V2_MIN &&
                     mH.mBIHSize <= InfoHeaderLength::OS2_V2_MAX);
   if (!bihSizeOk) {
-    PostDataError();
     return Transition::TerminateFailure();
   }
   // ICO BMPs must have a WinBMPv3 header. nsICODecoder should have already
   // terminated decoding if this isn't the case.
   MOZ_ASSERT_IF(mIsWithinICO, mH.mBIHSize == InfoHeaderLength::WIN_V3);
 
   return Transition::To(State::INFO_HEADER_REST,
                         mH.mBIHSize - BIHSIZE_FIELD_LENGTH);
@@ -546,17 +548,16 @@ nsBMPDecoder::ReadInfoHeaderRest(const c
 
   // BMPs with negative width are invalid. Also, reject extremely wide images
   // to keep the math sane. And reject INT_MIN as a height because you can't
   // get its absolute value (because -INT_MIN is one more than INT_MAX).
   const int32_t k64KWidth = 0x0000FFFF;
   bool sizeOk = 0 <= mH.mWidth && mH.mWidth <= k64KWidth &&
                 mH.mHeight != INT_MIN;
   if (!sizeOk) {
-    PostDataError();
     return Transition::TerminateFailure();
   }
 
   // Check mBpp and mCompression.
   bool bppCompressionOk =
     (mH.mCompression == Compression::RGB &&
       (mH.mBpp ==  1 || mH.mBpp ==  4 || mH.mBpp ==  8 ||
        mH.mBpp == 16 || mH.mBpp == 24 || mH.mBpp == 32)) ||
@@ -565,17 +566,16 @@ nsBMPDecoder::ReadInfoHeaderRest(const c
     (mH.mCompression == Compression::BITFIELDS &&
       // For BITFIELDS compression we require an exact match for one of the
       // WinBMP BIH sizes; this clearly isn't an OS2 BMP.
       (mH.mBIHSize == InfoHeaderLength::WIN_V3 ||
        mH.mBIHSize == InfoHeaderLength::WIN_V4 ||
        mH.mBIHSize == InfoHeaderLength::WIN_V5) &&
       (mH.mBpp == 16 || mH.mBpp == 32));
   if (!bppCompressionOk) {
-    PostDataError();
     return Transition::TerminateFailure();
   }
 
   // Post our size to the superclass.
   uint32_t absHeight = AbsoluteHeight();
   PostSize(mH.mWidth, absHeight);
   mCurrentRow = absHeight;
 
@@ -710,17 +710,16 @@ nsBMPDecoder::ReadColorTable(const char*
   // We know how many bytes we've read so far (mPreGapLength) and we know the
   // offset of the pixel data (mH.mDataOffset), so we can determine the length
   // of the gap (possibly zero) between the color table and the pixel data.
   //
   // If the gap is negative the file must be malformed (e.g. mH.mDataOffset
   // points into the middle of the color palette instead of past the end) and
   // we give up.
   if (mPreGapLength > mH.mDataOffset) {
-    PostDataError();
     return Transition::TerminateFailure();
   }
 
   uint32_t gapLength = mH.mDataOffset - mPreGapLength;
   return Transition::ToUnbuffered(State::AFTER_GAP, State::GAP, gapLength);
 }
 
 LexerTransition<nsBMPDecoder::State>
--- a/image/decoders/nsBMPDecoder.h
+++ b/image/decoders/nsBMPDecoder.h
@@ -145,18 +145,18 @@ public:
   /// Force transparency from outside. (Used by the ICO decoder.)
   void SetHasTransparency()
   {
     mMayHaveTransparency = true;
     mDoesHaveTransparency = true;
   }
 
   Maybe<TerminalState> DoDecode(SourceBufferIterator& aIterator) override;
-  virtual void BeforeFinishInternal() override;
-  virtual void FinishInternal() override;
+  nsresult BeforeFinishInternal() override;
+  nsresult FinishInternal() override;
 
 private:
   friend class DecoderFactory;
 
   enum class State {
     FILE_HEADER,
     INFO_HEADER_SIZE,
     INFO_HEADER_REST,
--- a/image/decoders/nsGIFDecoder2.cpp
+++ b/image/decoders/nsGIFDecoder2.cpp
@@ -95,29 +95,31 @@ nsGIFDecoder2::nsGIFDecoder2(RasterImage
   mGIFStruct.loop_count = 1;
 }
 
 nsGIFDecoder2::~nsGIFDecoder2()
 {
   free(mGIFStruct.local_colormap);
 }
 
-void
+nsresult
 nsGIFDecoder2::FinishInternal()
 {
   MOZ_ASSERT(!HasError(), "Shouldn't call FinishInternal after error!");
 
   // If the GIF got cut off, handle it anyway
   if (!IsMetadataDecode() && mGIFOpen) {
     if (mCurrentFrameIndex == mGIFStruct.images_decoded) {
       EndImageFrame();
     }
     PostDecodeDone(mGIFStruct.loop_count - 1);
     mGIFOpen = false;
   }
+
+  return NS_OK;
 }
 
 void
 nsGIFDecoder2::FlushImageData()
 {
   Maybe<SurfaceInvalidRect> invalidRect = mPipe.TakeInvalidRect();
   if (!invalidRect) {
     return;
--- a/image/decoders/nsGIFDecoder2.h
+++ b/image/decoders/nsGIFDecoder2.h
@@ -20,17 +20,17 @@ class RasterImage;
 // nsGIFDecoder2 Definition
 
 class nsGIFDecoder2 : public Decoder
 {
 public:
   ~nsGIFDecoder2();
 
   Maybe<TerminalState> DoDecode(SourceBufferIterator& aIterator) override;
-  virtual void FinishInternal() override;
+  nsresult FinishInternal() override;
   virtual Telemetry::ID SpeedHistogram() override;
 
 private:
   friend class DecoderFactory;
 
   // Decoders should only be instantiated via DecoderFactory.
   explicit nsGIFDecoder2(RasterImage* aImage);
 
--- a/image/decoders/nsICODecoder.cpp
+++ b/image/decoders/nsICODecoder.cpp
@@ -62,58 +62,61 @@ nsICODecoder::nsICODecoder(RasterImage* 
   , mCurrIcon(0)
   , mBPP(0)
   , mMaskRowSize(0)
   , mCurrMaskLine(0)
   , mIsCursor(false)
   , mHasMaskAlpha(false)
 { }
 
-void
+nsresult
 nsICODecoder::FinishInternal()
 {
   // We shouldn't be called in error cases
   MOZ_ASSERT(!HasError(), "Shouldn't call FinishInternal after error!");
 
-  GetFinalStateFromContainedDecoder();
+  return GetFinalStateFromContainedDecoder();
 }
 
-void
+nsresult
 nsICODecoder::FinishWithErrorInternal()
 {
-  GetFinalStateFromContainedDecoder();
+  return GetFinalStateFromContainedDecoder();
 }
 
-void
+nsresult
 nsICODecoder::GetFinalStateFromContainedDecoder()
 {
   if (!mContainedDecoder) {
-    return;
+    return NS_OK;
   }
 
   MOZ_ASSERT(mContainedSourceBuffer,
              "Should have a SourceBuffer if we have a decoder");
 
   // Let the contained decoder finish up if necessary.
   if (!mContainedSourceBuffer->IsComplete()) {
     mContainedSourceBuffer->Complete(NS_OK);
-    if (NS_FAILED(mContainedDecoder->Decode(mDoNotResume))) {
-      PostDataError();
-    }
+    mContainedDecoder->Decode(mDoNotResume);
   }
 
   // Make our state the same as the state of the contained decoder.
   mDecodeDone = mContainedDecoder->GetDecodeDone();
-  mDataError = mDataError || mContainedDecoder->HasDataError();
   mDecodeAborted = mContainedDecoder->WasAborted();
   mProgress |= mContainedDecoder->TakeProgress();
   mInvalidRect.UnionRect(mInvalidRect, mContainedDecoder->TakeInvalidRect());
   mCurrentFrame = mContainedDecoder->GetCurrentFrameRef();
 
-  MOZ_ASSERT(HasError() || !mCurrentFrame || mCurrentFrame->IsFinished());
+  // Propagate errors.
+  nsresult rv = HasError() || mContainedDecoder->HasError()
+              ? NS_ERROR_FAILURE
+              : NS_OK;
+
+  MOZ_ASSERT(NS_FAILED(rv) || !mCurrentFrame || mCurrentFrame->IsFinished());
+  return rv;
 }
 
 bool
 nsICODecoder::CheckAndFixBitmapSize(int8_t* aBIH)
 {
   // Get the width from the BMP file information header. This is
   // (unintuitively) a signed integer; see the documentation at:
   //
@@ -642,28 +645,31 @@ nsICODecoder::WriteToContainedDecoder(co
 {
   MOZ_ASSERT(mContainedDecoder);
   MOZ_ASSERT(mContainedSourceBuffer);
 
   // Append the provided data to the SourceBuffer that the contained decoder is
   // reading from.
   mContainedSourceBuffer->Append(aBuffer, aCount);
 
+  bool succeeded = true;
+
   // Write to the contained decoder. If we run out of data, the ICO decoder will
   // get resumed when there's more data available, as usual, so we don't need
   // the contained decoder to get resumed too. To avoid that, we provide an
   // IResumable which just does nothing.
   if (NS_FAILED(mContainedDecoder->Decode(mDoNotResume))) {
-    PostDataError();
+    succeeded = false;
   }
 
-  // Make our state the same as the state of the contained decoder.
+  // Make our state the same as the state of the contained decoder, and
+  // propagate errors.
   mProgress |= mContainedDecoder->TakeProgress();
   mInvalidRect.UnionRect(mInvalidRect, mContainedDecoder->TakeInvalidRect());
   if (mContainedDecoder->HasDataError()) {
-    PostDataError();
+    succeeded = false;
   }
 
-  return !HasError();
+  return succeeded;
 }
 
 } // namespace image
 } // namespace mozilla
--- a/image/decoders/nsICODecoder.h
+++ b/image/decoders/nsICODecoder.h
@@ -66,31 +66,31 @@ public:
   {
     return gfx::IntSize(GetRealWidth(), GetRealHeight());
   }
 
   /// @return The offset from the beginning of the ICO to the first resource.
   size_t FirstResourceOffset() const;
 
   Maybe<TerminalState> DoDecode(SourceBufferIterator& aIterator) override;
-  virtual void FinishInternal() override;
-  virtual void FinishWithErrorInternal() override;
+  nsresult FinishInternal() override;
+  nsresult FinishWithErrorInternal() override;
 
 private:
   friend class DecoderFactory;
 
   // Decoders should only be instantiated via DecoderFactory.
   explicit nsICODecoder(RasterImage* aImage);
 
   // Writes to the contained decoder and sets the appropriate errors
   // Returns true if there are no errors.
   bool WriteToContainedDecoder(const char* aBuffer, uint32_t aCount);
 
   // Gets decoder state from the contained decoder so it's visible externally.
-  void GetFinalStateFromContainedDecoder();
+  nsresult GetFinalStateFromContainedDecoder();
 
   /**
    * Verifies that the width and height values in @aBIH are valid and match the
    * values we read from the ICO directory entry. If everything looks OK, the
    * height value in @aBIH is updated to compensate for the AND mask, which the
    * underlying BMP decoder doesn't know about.
    *
    * @return true if the width and height values in @aBIH are valid and correct.
--- a/image/decoders/nsJPEGDecoder.cpp
+++ b/image/decoders/nsJPEGDecoder.cpp
@@ -164,25 +164,27 @@ nsJPEGDecoder::InitInternal()
   // Record app markers for ICC data
   for (uint32_t m = 0; m < 16; m++) {
     jpeg_save_markers(&mInfo, JPEG_APP0 + m, 0xFFFF);
   }
 
   return NS_OK;
 }
 
-void
+nsresult
 nsJPEGDecoder::FinishInternal()
 {
   // If we're not in any sort of error case, force our state to JPEG_DONE.
   if ((mState != JPEG_DONE && mState != JPEG_SINK_NON_JPEG_TRAILER) &&
       (mState != JPEG_ERROR) &&
       !IsMetadataDecode()) {
     mState = JPEG_DONE;
   }
+
+  return NS_OK;
 }
 
 Maybe<TerminalState>
 nsJPEGDecoder::DoDecode(SourceBufferIterator& aIterator)
 {
   MOZ_ASSERT(!HasError(), "Shouldn't call DoDecode after error!");
   MOZ_ASSERT(aIterator.Data());
   MOZ_ASSERT(aIterator.Length() > 0);
@@ -296,34 +298,32 @@ nsJPEGDecoder::ReadJPEGData(const char* 
           break;
         case JCS_CMYK:
         case JCS_YCCK:
             // qcms doesn't support cmyk
             mismatch = true;
           break;
         default:
           mState = JPEG_ERROR;
-          PostDataError();
           MOZ_LOG(sJPEGDecoderAccountingLog, LogLevel::Debug,
                  ("} (unknown colorpsace (1))"));
           return Transition::TerminateFailure();
       }
 
       if (!mismatch) {
         qcms_data_type type;
         switch (mInfo.out_color_space) {
           case JCS_GRAYSCALE:
             type = QCMS_DATA_GRAY_8;
             break;
           case JCS_RGB:
             type = QCMS_DATA_RGB_8;
             break;
           default:
             mState = JPEG_ERROR;
-            PostDataError();
             MOZ_LOG(sJPEGDecoderAccountingLog, LogLevel::Debug,
                    ("} (unknown colorpsace (2))"));
             return Transition::TerminateFailure();
         }
 #if 0
         // We don't currently support CMYK profiles. The following
         // code dealt with lcms types. Add something like this
         // back when we gain support for CMYK.
@@ -372,17 +372,16 @@ nsJPEGDecoder::ReadJPEGData(const char* 
           break;
         case JCS_CMYK:
         case JCS_YCCK:
           // libjpeg can convert from YCCK to CMYK, but not to RGB
           mInfo.out_color_space = JCS_CMYK;
           break;
         default:
           mState = JPEG_ERROR;
-          PostDataError();
           MOZ_LOG(sJPEGDecoderAccountingLog, LogLevel::Debug,
                  ("} (unknown colorpsace (3))"));
           return Transition::TerminateFailure();
       }
     }
 
     // Don't allocate a giant and superfluous memory buffer
     // when not doing a progressive decode.
--- a/image/decoders/nsJPEGDecoder.h
+++ b/image/decoders/nsJPEGDecoder.h
@@ -54,17 +54,17 @@ public:
 
   virtual void SetSampleSize(int aSampleSize) override
   {
     mSampleSize = aSampleSize;
   }
 
   nsresult InitInternal() override;
   Maybe<TerminalState> DoDecode(SourceBufferIterator& aIterator) override;
-  virtual void FinishInternal() override;
+  nsresult FinishInternal() override;
 
   virtual Telemetry::ID SpeedHistogram() override;
   void NotifyDone();
 
 protected:
   Orientation ReadOrientationFromEXIF();
   void OutputScanlines(bool* suspend);