Bug 1279859 - Correctly skip over extra image sub blocks in the GIF decoder. r=njn
authorSeth Fowler <mark.seth.fowler@gmail.com>
Mon, 20 Jun 2016 20:42:31 -0700
changeset 342166 629faa5d9254287324f40989941fca237beb93ac
parent 342165 c7933c6620c53681b2f2443deed69e442e1db542
child 342167 0ffa18cfc8b4e9970275ab6dd94686792a5c8ae1
child 342169 d1e5d75a2e9d6f46a48bc66c243dcfeb2cd40788
push id6389
push userraliiev@mozilla.com
push dateMon, 19 Sep 2016 13:38:22 +0000
treeherdermozilla-beta@01d67bfe6c81 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnjn
bugs1279859
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 1279859 - Correctly skip over extra image sub blocks in the GIF decoder. r=njn
image/decoders/nsGIFDecoder2.cpp
image/decoders/nsGIFDecoder2.h
image/test/gtest/Common.cpp
image/test/gtest/Common.h
image/test/gtest/TestDecoders.cpp
image/test/gtest/animated-with-extra-image-sub-blocks.gif
image/test/gtest/moz.build
--- a/image/decoders/nsGIFDecoder2.cpp
+++ b/image/decoders/nsGIFDecoder2.cpp
@@ -490,16 +490,18 @@ nsGIFDecoder2::WriteInternal(const char*
           case State::FINISHED_LOCAL_COLOR_TABLE:
             return FinishedLocalColorTable();
           case State::IMAGE_DATA_BLOCK:
             return ReadImageDataBlock(aData);
           case State::IMAGE_DATA_SUB_BLOCK:
             return ReadImageDataSubBlock(aData);
           case State::LZW_DATA:
             return ReadLZWData(aData, aLength);
+          case State::SKIP_LZW_DATA:
+            return Transition::ContinueUnbuffered(State::SKIP_LZW_DATA);
           case State::FINISHED_LZW_DATA:
             return Transition::To(State::IMAGE_DATA_SUB_BLOCK, SUB_BLOCK_HEADER_LEN);
           case State::SKIP_SUB_BLOCKS:
             return SkipSubBlocks(aData);
           case State::SKIP_DATA_THEN_SKIP_SUB_BLOCKS:
             return Transition::ContinueUnbuffered(State::SKIP_DATA_THEN_SKIP_SUB_BLOCKS);
           case State::FINISHED_SKIPPING_DATA:
             return Transition::To(State::SKIP_SUB_BLOCKS, SUB_BLOCK_HEADER_LEN);
@@ -982,26 +984,30 @@ nsGIFDecoder2::ReadImageDataSubBlock(con
   const uint8_t subBlockLength = aData[0];
   if (subBlockLength == 0) {
     // We hit the block terminator.
     EndImageFrame();
     return Transition::To(State::BLOCK_HEADER, BLOCK_HEADER_LEN);
   }
 
   if (mGIFStruct.pixels_remaining == 0) {
-    // We've already written to the entire image. |subBlockLength| should've
-    // been zero.
+    // We've already written to the entire image; we should've hit the block
+    // terminator at this point. This image is corrupt, but we'll tolerate it.
+
     if (subBlockLength == GIF_TRAILER) {
       // This GIF is missing the block terminator for the final block; we'll put
       // up with it.
       FinishInternal();
       return Transition::TerminateSuccess();
     }
 
-    return Transition::To(State::IMAGE_DATA_SUB_BLOCK, SUB_BLOCK_HEADER_LEN);
+    // We're not at the end of the image, so just skip the extra data.
+    return Transition::ToUnbuffered(State::FINISHED_LZW_DATA,
+                                    State::SKIP_LZW_DATA,
+                                    subBlockLength);
   }
 
   // Handle the standard case: there's data in the sub-block and pixels left to
   // fill in the image. We read the sub-block unbuffered so we can get pixels on
   // the screen as soon as possible.
   return Transition::ToUnbuffered(State::FINISHED_LZW_DATA,
                                   State::LZW_DATA,
                                   subBlockLength);
--- a/image/decoders/nsGIFDecoder2.h
+++ b/image/decoders/nsGIFDecoder2.h
@@ -85,16 +85,17 @@ private:
     NETSCAPE_EXTENSION_SUB_BLOCK,
     NETSCAPE_EXTENSION_DATA,
     IMAGE_DESCRIPTOR,
     LOCAL_COLOR_TABLE,
     FINISHED_LOCAL_COLOR_TABLE,
     IMAGE_DATA_BLOCK,
     IMAGE_DATA_SUB_BLOCK,
     LZW_DATA,
+    SKIP_LZW_DATA,
     FINISHED_LZW_DATA,
     SKIP_SUB_BLOCKS,
     SKIP_DATA_THEN_SKIP_SUB_BLOCKS,
     FINISHED_SKIPPING_DATA
   };
 
   LexerTransition<State> ReadGIFHeader(const char* aData);
   LexerTransition<State> ReadScreenDescriptor(const char* aData);
--- a/image/test/gtest/Common.cpp
+++ b/image/test/gtest/Common.cpp
@@ -528,16 +528,24 @@ ImageTestCase NoFrameDelayGIFTestCase()
 {
   // This is an invalid (or at least, questionably valid) GIF that's animated
   // even though it specifies a frame delay of zero. It's animated, but it's not
   // marked TEST_CASE_IS_ANIMATED because the metadata decoder can't detect that
   // it's animated.
   return ImageTestCase("no-frame-delay.gif", "image/gif", IntSize(100, 100));
 }
 
+ImageTestCase ExtraImageSubBlocksAnimatedGIFTestCase()
+{
+  // This is a corrupt GIF that has extra image sub blocks between the first and
+  // second frame.
+  return ImageTestCase("animated-with-extra-image-sub-blocks.gif", "image/gif",
+                       IntSize(100, 100));
+}
+
 ImageTestCase DownscaledPNGTestCase()
 {
   // This testcase (and all the other "downscaled") testcases) consists of 25
   // lines of green, followed by 25 lines of red, followed by 25 lines of green,
   // followed by 25 more lines of red. It's intended that tests downscale it
   // from 100x100 to 20x20, so we specify a 20x20 output size.
   return ImageTestCase("downscaled.png", "image/png", IntSize(100, 100),
                        IntSize(20, 20));
--- a/image/test/gtest/Common.h
+++ b/image/test/gtest/Common.h
@@ -327,16 +327,17 @@ ImageTestCase GreenFirstFrameAnimatedGIF
 ImageTestCase GreenFirstFrameAnimatedPNGTestCase();
 
 ImageTestCase CorruptTestCase();
 
 ImageTestCase TransparentPNGTestCase();
 ImageTestCase TransparentGIFTestCase();
 ImageTestCase FirstFramePaddingGIFTestCase();
 ImageTestCase NoFrameDelayGIFTestCase();
+ImageTestCase ExtraImageSubBlocksAnimatedGIFTestCase();
 
 ImageTestCase TransparentBMPWhenBMPAlphaEnabledTestCase();
 ImageTestCase RLE4BMPTestCase();
 ImageTestCase RLE8BMPTestCase();
 
 ImageTestCase DownscaledPNGTestCase();
 ImageTestCase DownscaledGIFTestCase();
 ImageTestCase DownscaledJPGTestCase();
--- a/image/test/gtest/TestDecoders.cpp
+++ b/image/test/gtest/TestDecoders.cpp
@@ -344,8 +344,77 @@ TEST(ImageDecoders, CorruptSingleChunk)
 {
   CheckDecoderSingleChunk(CorruptTestCase());
 }
 
 TEST(ImageDecoders, CorruptMultiChunk)
 {
   CheckDecoderMultiChunk(CorruptTestCase());
 }
+
+TEST(ImageDecoders, AnimatedGIFWithExtraImageSubBlocks)
+{
+  ImageTestCase testCase = ExtraImageSubBlocksAnimatedGIFTestCase();
+
+  // Verify that we can decode this test case and get two frames, even though
+  // there are extra image sub blocks between the first and second frame. The
+  // extra data shouldn't confuse the decoder or cause the decode to fail.
+
+  // Create an image.
+  RefPtr<Image> image =
+    ImageFactory::CreateAnonymousImage(nsDependentCString(testCase.mMimeType));
+  ASSERT_TRUE(!image->HasError());
+
+  nsCOMPtr<nsIInputStream> inputStream = LoadFile(testCase.mPath);
+  ASSERT_TRUE(inputStream);
+
+  // Figure out how much data we have.
+  uint64_t length;
+  nsresult rv = inputStream->Available(&length);
+  ASSERT_TRUE(NS_SUCCEEDED(rv));
+
+  // Write the data into the image.
+  rv = image->OnImageDataAvailable(nullptr, nullptr, inputStream, 0,
+                                   static_cast<uint32_t>(length));
+  ASSERT_TRUE(NS_SUCCEEDED(rv));
+
+  // Let the image know we've sent all the data.
+  rv = image->OnImageDataComplete(nullptr, nullptr, NS_OK, true);
+  ASSERT_TRUE(NS_SUCCEEDED(rv));
+
+  RefPtr<ProgressTracker> tracker = image->GetProgressTracker();
+  tracker->SyncNotifyProgress(FLAG_LOAD_COMPLETE);
+
+  // Use GetFrame() to force a sync decode of the image.
+  RefPtr<SourceSurface> surface =
+    image->GetFrame(imgIContainer::FRAME_CURRENT,
+                    imgIContainer::FLAG_SYNC_DECODE);
+
+  // Ensure that the image's metadata meets our expectations.
+  IntSize imageSize(0, 0);
+  rv = image->GetWidth(&imageSize.width);
+  EXPECT_TRUE(NS_SUCCEEDED(rv));
+  rv = image->GetHeight(&imageSize.height);
+  EXPECT_TRUE(NS_SUCCEEDED(rv));
+
+  EXPECT_EQ(testCase.mSize.width, imageSize.width);
+  EXPECT_EQ(testCase.mSize.height, imageSize.height);
+
+  Progress imageProgress = tracker->GetProgress();
+
+  EXPECT_TRUE(bool(imageProgress & FLAG_HAS_TRANSPARENCY) == false);
+  EXPECT_TRUE(bool(imageProgress & FLAG_IS_ANIMATED) == true);
+
+  // Ensure that we decoded both frames of the image.
+  LookupResult firstFrameLookupResult =
+    SurfaceCache::Lookup(ImageKey(image.get()),
+                         RasterSurfaceKey(imageSize,
+                                          DefaultSurfaceFlags(),
+                                          /* aFrameNum = */ 0));
+  EXPECT_EQ(MatchType::EXACT, firstFrameLookupResult.Type());
+
+  LookupResult secondFrameLookupResult =
+    SurfaceCache::Lookup(ImageKey(image.get()),
+                         RasterSurfaceKey(imageSize,
+                                          DefaultSurfaceFlags(),
+                                          /* aFrameNum = */ 1));
+  EXPECT_EQ(MatchType::EXACT, secondFrameLookupResult.Type());
+}
new file mode 100644
index e69de29bb2d1d6434b8b29ae775ad8c2e48c5391..a145c814a684c454e4003ab41699798db0ed8366
GIT binary patch
literal 434
zc${<hbhEHbOkqf2_`txx@E?d76o0ZXaDnLm+<vYh!Oo5Wu10zW%#1)G9gsXoF#}U^
zOaIE#Z}}I`*>bCU^SwR4`P&|OOncV3?A57l@3@bD^11%4_xZ1L-~aLRaJ3&<^s&Q7
zYx<dGpE-NImS0)*wd-uw_B-pobMti{f3oRkkKgL+Z?^sB?LYne%dWqD=U;#Sv+qB@
qgocGjL`Fr&j1@ah+<5VWBQt_wAnb$(2i)%s_4V}(0~7)b)?5Ivyyp%8
--- a/image/test/gtest/moz.build
+++ b/image/test/gtest/moz.build
@@ -25,16 +25,17 @@ if CONFIG['MOZ_ENABLE_SKIA']:
     ]
 
 SOURCES += [
     # Can't be unified because it manipulates the preprocessor environment.
     'TestDownscalingFilterNoSkia.cpp',
 ]
 
 TEST_HARNESS_FILES.gtest += [
+    'animated-with-extra-image-sub-blocks.gif',
     'corrupt.jpg',
     'downscaled.bmp',
     'downscaled.gif',
     'downscaled.ico',
     'downscaled.icon',
     'downscaled.jpg',
     'downscaled.png',
     'first-frame-green.gif',