Bug 1285867 (Part 4) - Decide whether we're done decoding by checking if we've reached a terminal state. r=edwin

image/Decoder.cpp

@@ -128,6 +128,12 @@ Decoder::Decode(NotNull<IResumable*> aOnResume)
   // terminal state) or there are no more chunks available.
   Maybe<TerminalState> terminalState;
   do {
+    if (GetDecodeDone()) {
+      MOZ_ASSERT_UNREACHABLE("Finished decode without reaching terminal state?");
+      terminalState = Some(TerminalState::SUCCESS);
+      break;
+    }
+
     switch (mIterator->AdvanceOrScheduleResume(aOnResume.get())) {
       case SourceBufferIterator::WAITING:
         // We can't continue because the rest of the data hasn't arrived from
@@ -166,17 +172,17 @@ Decoder::Decode(NotNull<IResumable*> aOnResume)
         MOZ_ASSERT_UNREACHABLE("Unknown SourceBufferIterator state");
         terminalState = Some(TerminalState::FAILURE);
     }
-  } while (!GetDecodeDone() && !terminalState);
+  } while (!terminalState);
+
+  MOZ_ASSERT(terminalState);
 
   // If decoding failed, record that fact.
   if (terminalState == Some(TerminalState::FAILURE)) {
     PostDataError();
   }
 
-  // If we're done decoding, perform final cleanup.
-  if (terminalState) {
-    CompleteDecode();
-  }
+  // We're done decoding; perform final cleanup.
+  CompleteDecode();
 
   return HasError() ? NS_ERROR_FAILURE : NS_OK;
 }

image/decoders/nsBMPDecoder.cpp

@@ -581,10 +581,8 @@ nsBMPDecoder::ReadInfoHeaderRest(const char* aData, size_t aLength)
     return Transition::TerminateFailure();
   }
 
-  // Post our size to the superclass.
-  uint32_t absHeight = AbsoluteHeight();
-  PostSize(mH.mWidth, absHeight);
-  mCurrentRow = absHeight;
+  // Initialize our current row to the top of the image.
+  mCurrentRow = AbsoluteHeight();
 
   // Round it up to the nearest byte count, then pad to 4-byte boundary.
   // Compute this even for a metadate decode because GetCompressedImageSize()
@@ -652,6 +650,9 @@ nsBMPDecoder::ReadBitfields(const char* aData, size_t aLength)
     PostHasTransparency();
   }
 
+  // Post our size to the superclass.
+  PostSize(mH.mWidth, AbsoluteHeight());
+
   // We've now read all the headers. If we're doing a metadata decode, we're
   // done.
   if (IsMetadataDecode()) {

image/test/gtest/Common.cpp

@@ -533,6 +533,15 @@ ImageTestCase CorruptTestCase()
                        TEST_CASE_HAS_ERROR);
 }
 
+ImageTestCase CorruptBMPWithTruncatedHeader()
+{
+  // This BMP has a header which is truncated right between the BIH and the
+  // bitfields, which is a particularly error-prone place w.r.t. the BMP decoder
+  // state machine.
+  return ImageTestCase("invalid-truncated-metadata.bmp", "image/bmp",
+                       IntSize(100, 100), TEST_CASE_HAS_ERROR);
+}
+
 ImageTestCase CorruptICOWithBadBMPWidthTestCase()
 {
   // This ICO contains a BMP icon which has a width that doesn't match the size

image/test/gtest/Common.h

@@ -340,6 +340,7 @@ ImageTestCase GreenFirstFrameAnimatedGIFTestCase();
 ImageTestCase GreenFirstFrameAnimatedPNGTestCase();
 
 ImageTestCase CorruptTestCase();
+ImageTestCase CorruptBMPWithTruncatedHeader();
 ImageTestCase CorruptICOWithBadBMPWidthTestCase();
 ImageTestCase CorruptICOWithBadBMPHeightTestCase();
 

image/test/gtest/TestDecoders.cpp

@@ -340,6 +340,16 @@ TEST_F(ImageDecoders, CorruptMultiChunk)
   CheckDecoderMultiChunk(CorruptTestCase());
 }
 
+TEST_F(ImageDecoders, CorruptBMPWithTruncatedHeaderSingleChunk)
+{
+  CheckDecoderSingleChunk(CorruptBMPWithTruncatedHeader());
+}
+
+TEST_F(ImageDecoders, CorruptBMPWithTruncatedHeaderMultiChunk)
+{
+  CheckDecoderMultiChunk(CorruptBMPWithTruncatedHeader());
+}
+
 TEST_F(ImageDecoders, CorruptICOWithBadBMPWidthSingleChunk)
 {
   CheckDecoderSingleChunk(CorruptICOWithBadBMPWidthTestCase());

image/test/gtest/moz.build

@@ -50,6 +50,7 @@ TEST_HARNESS_FILES.gtest += [
     'green.icon',
     'green.jpg',
     'green.png',
+    'invalid-truncated-metadata.bmp',
     'no-frame-delay.gif',
     'rle4.bmp',
     'rle8.bmp',