Backed out 2 changesets (bug 1249578) for build bustage

Backed out changeset caddb604d934 (bug 1249578)
Backed out changeset a2a645bf7ccf (bug 1249578)

image/decoders/nsICODecoder.cpp

@@ -102,57 +102,58 @@ nsICODecoder::GetFinalStateFromContainedDecoder()
   MOZ_ASSERT(HasError() || !mCurrentFrame || mCurrentFrame->IsFinished());
 }
 
+// A BMP inside of an ICO has *2 height because of the AND mask
+// that follows the actual bitmap.  The BMP shouldn't know about
+// this difference though.
 bool
-nsICODecoder::CheckAndFixBitmapSize(int8_t* aBIH)
+nsICODecoder::FixBitmapHeight(int8_t* bih)
 {
-  // Get the width from the BMP file information header. This is
-  // (unintuitively) a signed integer; see the documentation at:
-  //
-  //   https://msdn.microsoft.com/en-us/library/windows/desktop/dd183376(v=vs.85).aspx
-  //
-  // However, we reject negative widths since they aren't meaningful.
-  const int32_t width = LittleEndian::readInt32(aBIH + 4);
-  if (width <= 0 || width > 256) {
-    return false;
-  }
-
-  // Verify that the BMP width matches the width we got from the ICO directory
-  // entry. If not, decoding fails, because if we were to allow it to continue
-  // the intrinsic size of the image wouldn't match the size of the decoded
-  // surface.
-  if (width != GetRealWidth()) {
-    return false;
-  }
-
-  // Get the height from the BMP file information header. This is also signed,
-  // but in this case negative values are meaningful; see below.
-  int32_t height = LittleEndian::readInt32(aBIH + 8);
-  if (height == 0) {
-    return false;
-  }
+  // Get the height from the BMP file information header.
+  int32_t height = LittleEndian::readInt32(bih + 8);
 
   // BMPs can be stored inverted by having a negative height.
-  // XXX(seth): Should we really be writing the absolute value into the BIH
-  // below? Seems like this could be problematic for inverted BMPs.
   height = abs(height);
 
-  // The height field is double the actual height of the image to account for
-  // the AND mask. This is true even if the AND mask is not present.
+  // The bitmap height is by definition * 2 what it should be to account for
+  // the 'AND mask'. It is * 2 even if the `AND mask` is not present.
   height /= 2;
+
   if (height > 256) {
     return false;
   }
 
-  // Verify that the BMP height matches the height we got from the ICO directory
-  // entry. If not, again, decoding fails.
-  if (height != GetRealHeight()) {
+  // We should always trust the height from the bitmap itself instead of
+  // the ICO height.  So fix the ICO height.
+  if (height == 256) {
+    mDirEntry.mHeight = 0;
+  } else {
+    mDirEntry.mHeight = (int8_t)height;
+  }
+
+  // Fix the BMP height in the BIH so that the BMP decoder can work properly
+  LittleEndian::writeInt32(bih + 8, height);
+  return true;
+}
+
+// We should always trust the contained resource for the width
+// information over our own information.
+bool
+nsICODecoder::FixBitmapWidth(int8_t* bih)
+{
+  // Get the width from the BMP file information header.
+  int32_t width = LittleEndian::readInt32(bih + 4);
+
+  if (width > 256) {
     return false;
   }
 
-  // Fix the BMP height in the BIH so that the BMP decoder, which does not know
-  // about the AND mask that may follow the actual bitmap, can work properly.
-  LittleEndian::writeInt32(aBIH + 8, GetRealHeight());
-
+  // We should always trust the width from the bitmap itself instead of
+  // the ICO width.
+  if (width == 256) {
+    mDirEntry.mWidth = 0;
+  } else {
+    mDirEntry.mWidth = (int8_t)width;
+  }
   return true;
 }
 
@@ -380,10 +381,15 @@ nsICODecoder::ReadBIH(const char* aData)
   }
   mContainedDecoder->Init();
 
-  // Verify that the BIH width and height values match the ICO directory entry,
-  // and fix the BIH height value to compensate for the fact that the underlying
-  // BMP decoder doesn't know about AND masks.
-  if (!CheckAndFixBitmapSize(reinterpret_cast<int8_t*>(mBIHraw))) {
+  // Fix the ICO height from the BIH. It needs to be halved so our BMP decoder
+  // will understand, because the BMP decoder doesn't expect the alpha mask that
+  // follows the BMP data in an ICO.
+  if (!FixBitmapHeight(reinterpret_cast<int8_t*>(mBIHraw))) {
+    return Transition::TerminateFailure();
+  }
+
+  // Fix the ICO width from the BIH.
+  if (!FixBitmapWidth(reinterpret_cast<int8_t*>(mBIHraw))) {
     return Transition::TerminateFailure();
   }
 

image/decoders/nsICODecoder.h

@@ -85,16 +85,14 @@ private:
   // Gets decoder state from the contained decoder so it's visible externally.
   void 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.
-   */
-  bool CheckAndFixBitmapSize(int8_t* aBIH);
-
+  // Fixes the ICO height to match that of the BIH.
+  // and also fixes the BIH height to be /2 of what it was.
+  // See definition for explanation.
+  // Returns false if invalid information is contained within.
+  bool FixBitmapHeight(int8_t* bih);
+  // Fixes the ICO width to match that of the BIH.
+  // Returns false if invalid information is contained within.
+  bool FixBitmapWidth(int8_t* bih);
   // Obtains the number of colors from the BPP, mBPP must be filled in
   uint16_t GetNumColors();
 

image/test/gtest/Common.cpp

@@ -485,22 +485,6 @@ ImageTestCase CorruptTestCase()
                        TEST_CASE_HAS_ERROR);
 }
 
-ImageTestCase CorruptICOWithBadBMPWidthTestCase()
-{
-  // This ICO contains a BMP icon which has a width that doesn't match the size
-  // listed in the corresponding ICO directory entry.
-  return ImageTestCase("corrupt-with-bad-bmp-width.ico", "image/x-icon",
-                       IntSize(100, 100), TEST_CASE_HAS_ERROR);
-}
-
-ImageTestCase CorruptICOWithBadBMPHeightTestCase()
-{
-  // This ICO contains a BMP icon which has a height that doesn't match the size
-  // listed in the corresponding ICO directory entry.
-  return ImageTestCase("corrupt-with-bad-bmp-height.ico", "image/x-icon",
-                       IntSize(100, 100), TEST_CASE_HAS_ERROR);
-}
-
 ImageTestCase TransparentPNGTestCase()
 {
   return ImageTestCase("transparent.png", "image/png", IntSize(32, 32),

image/test/gtest/Common.h

@@ -327,8 +327,6 @@ ImageTestCase GreenFirstFrameAnimatedGIFTestCase();
 ImageTestCase GreenFirstFrameAnimatedPNGTestCase();
 
 ImageTestCase CorruptTestCase();
-ImageTestCase CorruptICOWithBadBMPWidthTestCase();
-ImageTestCase CorruptICOWithBadBMPHeightTestCase();
 
 ImageTestCase TransparentPNGTestCase();
 ImageTestCase TransparentGIFTestCase();

image/test/gtest/TestDecoders.cpp

@@ -350,26 +350,6 @@ TEST(ImageDecoders, CorruptMultiChunk)
   CheckDecoderMultiChunk(CorruptTestCase());
 }
 
-TEST(ImageDecoders, CorruptICOWithBadBMPWidthSingleChunk)
-{
-  CheckDecoderSingleChunk(CorruptICOWithBadBMPWidthTestCase());
-}
-
-TEST(ImageDecoders, CorruptICOWithBadBMPWidthMultiChunk)
-{
-  CheckDecoderMultiChunk(CorruptICOWithBadBMPWidthTestCase());
-}
-
-TEST(ImageDecoders, CorruptICOWithBadBMPHeightSingleChunk)
-{
-  CheckDecoderSingleChunk(CorruptICOWithBadBMPHeightTestCase());
-}
-
-TEST(ImageDecoders, CorruptICOWithBadBMPHeightMultiChunk)
-{
-  CheckDecoderMultiChunk(CorruptICOWithBadBMPHeightTestCase());
-}
-
 TEST(ImageDecoders, AnimatedGIFWithExtraImageSubBlocks)
 {
   ImageTestCase testCase = ExtraImageSubBlocksAnimatedGIFTestCase();

image/test/gtest/moz.build

@@ -31,8 +31,6 @@ SOURCES += [
 
 TEST_HARNESS_FILES.gtest += [
     'animated-with-extra-image-sub-blocks.gif',
-    'corrupt-with-bad-bmp-height.ico',
-    'corrupt-with-bad-bmp-width.ico',
     'corrupt.jpg',
     'downscaled.bmp',
     'downscaled.gif',