Backed out 2 changesets (bug 1249578) for build bustage
authorWes Kocher <wkocher@mozilla.com>
Tue, 21 Jun 2016 18:38:46 -0700
changeset 302260 2245bcadd89fd4650ba14414cab0efcaef7f13e9
parent 302259 caddb604d934f633e88f0bcf8e1835f5f3249157
child 302261 1f727c7981aef52ab3c21bdfb84627aef6a5d350
push id78670
push userkwierso@gmail.com
push dateWed, 22 Jun 2016 01:38:59 +0000
treeherdermozilla-inbound@2245bcadd89f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
bugs1249578
milestone50.0a1
backs outcaddb604d934f633e88f0bcf8e1835f5f3249157
a2a645bf7ccf4ffd4313fa55019889feb7957082
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
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
image/decoders/nsICODecoder.h
image/test/gtest/Common.cpp
image/test/gtest/Common.h
image/test/gtest/TestDecoders.cpp
image/test/gtest/corrupt-with-bad-bmp-height.ico
image/test/gtest/corrupt-with-bad-bmp-width.ico
image/test/gtest/moz.build
--- a/image/decoders/nsICODecoder.cpp
+++ b/image/decoders/nsICODecoder.cpp
@@ -97,67 +97,68 @@ nsICODecoder::GetFinalStateFromContained
   mDecodeAborted = mContainedDecoder->WasAborted();
   mProgress |= mContainedDecoder->TakeProgress();
   mInvalidRect.UnionRect(mInvalidRect, mContainedDecoder->TakeInvalidRect());
   mCurrentFrame = mContainedDecoder->GetCurrentFrameRef();
 
   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;
 }
 
 LexerTransition<ICOState>
 nsICODecoder::ReadHeader(const char* aData)
 {
   // If the third byte is 1, this is an icon. If 2, a cursor.
   if ((aData[2] != 1) && (aData[2] != 2)) {
@@ -375,20 +376,25 @@ nsICODecoder::ReadBIH(const char* aData)
   mContainedDecoder->SetMetadataDecode(IsMetadataDecode());
   mContainedDecoder->SetDecoderFlags(GetDecoderFlags());
   mContainedDecoder->SetSurfaceFlags(GetSurfaceFlags());
   if (mDownscaler) {
     mContainedDecoder->SetTargetSize(mDownscaler->TargetSize());
   }
   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();
   }
 
   // Write out the BMP's bitmap info header.
   if (!WriteToContainedDecoder(mBIHraw, sizeof(mBIHraw))) {
     return Transition::TerminateFailure();
   }
 
--- a/image/decoders/nsICODecoder.h
+++ b/image/decoders/nsICODecoder.h
@@ -80,26 +80,24 @@ private:
 
   // 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();
 
-  /**
-   * 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();
 
   LexerTransition<ICOState> ReadHeader(const char* aData);
   LexerTransition<ICOState> ReadDirEntry(const char* aData);
   LexerTransition<ICOState> SniffResource(const char* aData);
   LexerTransition<ICOState> ReadPNG(const char* aData, uint32_t aLen);
   LexerTransition<ICOState> ReadBIH(const char* aData);
--- a/image/test/gtest/Common.cpp
+++ b/image/test/gtest/Common.cpp
@@ -480,32 +480,16 @@ ImageTestCase GreenFirstFrameAnimatedPNG
 }
 
 ImageTestCase CorruptTestCase()
 {
   return ImageTestCase("corrupt.jpg", "image/jpeg", 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
-  // 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),
                        TEST_CASE_IS_TRANSPARENT);
 }
 
 ImageTestCase TransparentGIFTestCase()
 {
--- a/image/test/gtest/Common.h
+++ b/image/test/gtest/Common.h
@@ -322,18 +322,16 @@ ImageTestCase GreenJPGTestCase();
 ImageTestCase GreenBMPTestCase();
 ImageTestCase GreenICOTestCase();
 ImageTestCase GreenIconTestCase();
 
 ImageTestCase GreenFirstFrameAnimatedGIFTestCase();
 ImageTestCase GreenFirstFrameAnimatedPNGTestCase();
 
 ImageTestCase CorruptTestCase();
-ImageTestCase CorruptICOWithBadBMPWidthTestCase();
-ImageTestCase CorruptICOWithBadBMPHeightTestCase();
 
 ImageTestCase TransparentPNGTestCase();
 ImageTestCase TransparentGIFTestCase();
 ImageTestCase FirstFramePaddingGIFTestCase();
 ImageTestCase NoFrameDelayGIFTestCase();
 ImageTestCase ExtraImageSubBlocksAnimatedGIFTestCase();
 
 ImageTestCase TransparentBMPWhenBMPAlphaEnabledTestCase();
--- a/image/test/gtest/TestDecoders.cpp
+++ b/image/test/gtest/TestDecoders.cpp
@@ -345,36 +345,16 @@ TEST(ImageDecoders, CorruptSingleChunk)
   CheckDecoderSingleChunk(CorruptTestCase());
 }
 
 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();
 
   // 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.
 
deleted file mode 100644
index ee4a90fcd78ffea0c28ed23835983b6928206be2..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
GIT binary patch
literal 0
Hc$@<O00001
deleted file mode 100644
index aa4051cd07ab64ca2e2f755c3cf70d8d28781ceb..e69de29bb2d1d6434b8b29ae775ad8c2e48c5391
GIT binary patch
literal 0
Hc$@<O00001
--- a/image/test/gtest/moz.build
+++ b/image/test/gtest/moz.build
@@ -26,18 +26,16 @@ 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-with-bad-bmp-height.ico',
-    'corrupt-with-bad-bmp-width.ico',
     'corrupt.jpg',
     'downscaled.bmp',
     'downscaled.gif',
     'downscaled.ico',
     'downscaled.icon',
     'downscaled.jpg',
     'downscaled.png',
     'first-frame-green.gif',