1315554 - favicon.ico file doesn't display correctly, works fine in Chrome and Microsoft Edge

Status: RESOLVED FIXED

(Core :: Graphics: ImageLib, defect)

Description

[u580221]

Attachment: Screenshot of incorrect render in firefox

User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:51.0) Gecko/20100101 Firefox/51.0
Build ID: 20161101004006

Steps to reproduce:

I have a favicon.ico file which doesn't display correctly, but it works fine in Chrome and Microsoft Edge (and also opens correctly in GIMP).


Actual results:

The favicon at https://wobblylang.org/favicon.ico blinks up... something for a second, then reverts to grey (see attached screenshot) which is definitely not what it looks like


Expected results:

Favicon at https://wobblylang.org/favicon.ico displays correctly when visiting the url directoy

Comment 1

[Matthias Versen [:Matti]]

I can confirm this report with Firefox 49.0.2 on windows7

Comment 2

[YF (Yang)]

Regression window:
m-c [2015-09-19, 2015-09-20]

Comment 3

[Andrew Osmond [:aosmond] (he/him)]

Part 1 of Bug 1201796 caused this regression. The contained PNG has a size of 512x512 but the icon itself says it is 0x0 (which maps to 256x256).

https://hg.mozilla.org/mozilla-central/rev/4ce4da0007104e7e45c30bb7712ff55725ca0faf

Comment 4

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Accept big ICOs, v1

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=4bc7dd37360946acdbd7baaf777fdbdb4f56e385

Comment 5

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8808623 [details] [diff] [review]
Accept big ICOs, v1

So an ico containing a 512x512 png will report it's intrinsic size as 256x256, correct? If we are then asked to draw it at 256x256 no downscaler will be created, so we'll put a 512x512 surface in the surface cache. And we'll keep trying to decode at 256x256 everytime we draw it because the lookup result will be "substitute because pending". Or am I wrong?

Comment 6

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Timothy Nikkel (:tnikkel) from comment #5)
> Comment on attachment 8808623 [details] [diff] [review]
> Accept big ICOs, v1
> 
> So an ico containing a 512x512 png will report it's intrinsic size as
> 256x256, correct? If we are then asked to draw it at 256x256 no downscaler
> will be created, so we'll put a 512x512 surface in the surface cache. And
> we'll keep trying to decode at 256x256 everytime we draw it because the
> lookup result will be "substitute because pending". Or am I wrong?

The cache should have a 256x256 image -- the fact there is a larger image embedded in the ICO can be considered internal to the decoder (downscaling as necessary). This isn't ideal anything else requires us to implement seek for the decoders. That isn't necessarily a bad idea but I'd rather put that on the TODO optimization list right now and see how things shake out with quantum/webrender first :).

Comment 7

[Timothy Nikkel (:tnikkel)]

(In reply to Andrew Osmond [:aosmond] from comment #6)
> The cache should have a 256x256 image -- the fact there is a larger image
> embedded in the ICO can be considered internal to the decoder (downscaling
> as necessary).

Hmm, okay, so I guess it works for PNGs because the ico decoder won't have an mDownscaler, but the contained PNG decoder will have a downscaler (actually it uses surface pipe, but the result is the same). But for BMPs I think there will be a disagreement between the ico decoder and the contained decoder. The ico decoder won't have a downscaler but the contained bmp decoder will have a downscaler. This could lead to problems if there is a mask because we downscale the mask in the icodecoder, but we downscale the image in the contained bmp decoder. So the ico decoder won't be downscaling, but the contained bmp decoder will be downscaling and we will try to combine a mask and image of different sizes.

That's really gross, so I hope I'm wrong.

An easy fix would be to limit your patch to only apply to PNGs.

We should also add asserts so that a situation like I described can't happen.

Comment 8

[u580221]

As far as I'm concerned, limiting this to PNGs seems reasonable. I don't think anyone would reasonably want 512x512 in a non-PNG .ico since that would be an unreasonably huge favicon file.

However, maybe the browser should try to fallback to a smaller size in such a case though, instead of completely refusing to read such a file. (if possible)

Comment 9

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Accept big ICOs, v2

I changed it to 1) only accept the differing size if it is a PNG, 2) keep the original check if a BMP, 3) moved the check to be earlier in the process for BMPs so it won't attempt to apply a mask against the wrong size, and 4) removed skipping the mask and instead transitions directly into terminate success (as there is no check to do afterwards anymore, as it is already done).

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=85b197b80fd59c27cacd78c4e3f8257b1df8e252

Comment 10

[mberchtold]

I can also confirm this with the icon from here:
https://www.smartftp.com/static/favicon.ico
and with Firefox 51.0.1 (64-bit)

Comment 11

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Reuse the same SourceBuffer instead of copying, v1

Comment 12

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Get ICO size from resource when unspecified, v1

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d63adb22c098098c90104d7fd79553eec6895c2

Comment 13

[Andrew Osmond [:aosmond] (he/him)]

(In reply to mberchtold from comment #10)
> I can also confirm this with the icon from here:
> https://www.smartftp.com/static/favicon.ico
> and with Firefox 51.0.1 (64-bit)

Hm, this should have worked before given it is just a 256x256 image (as rendered in Chrome and Firefox + patches).

Comment 14

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Reuse the same SourceBuffer instead of copying, v2

Comment 15

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Get ICO size from resource when unspecified, v2

Comment 16

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Get ICO size from resource when unspecified, v3

This should fix the broken gtests...

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2602c7dc89b54acb3316b355c2bf2402df95c1e3

Comment 17

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Get ICO size from resource when unspecified, v4

Some minor refactoring, no functional change intended.

Comment 18

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Get ICO size from resource when unspecified, v5

Fix the gtest I missed, but it is a bug in the test as a result of the changes, rather than a bug in the decoder.

Comment 19

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Reuse the same SourceBuffer instead of copying, v3

Enforce a read limit on the iterator to make sure the contained decoder can't overrun the resource size.

Comment 20

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Get ICO size from resource when unspecified, v6

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a90127e78c56e75270bca54e35fb97d52cb03691

Rebase on part 1 and also ensure we flush the contained decoder as new data comes in to have the same behaviour as before (allows progressively drawing a theoretically giant ICO).

Comment 21

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. Get ICO size from resource when unspecified, v7

Rebase on top of bug 1343499.

Comment 22

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8845911 [details] [diff] [review]
Part 1. Reuse the same SourceBuffer instead of copying, v3

>     mContainedDecoder =
>       DecoderFactory::CreateDecoderForICOResource(DecoderType::PNG,
>-                                                  WrapNotNull(mContainedSourceBuffer),
>+                                                  *mContainedIterator,
>                                                   WrapNotNull(this));

Since this does a move of mContainedIterator, mContainedIterator is going to be an empty iterator (no owner source buffer) most of the time. This seems a bit silly to store as a class member.

Comment 23

[Andrew Osmond [PLEASE USE MOZILLA ACCT]]

(In reply to Timothy Nikkel (:tnikkel) from comment #22)
> Comment on attachment 8845911 [details] [diff] [review]
> Part 1. Reuse the same SourceBuffer instead of copying, v3
> 
> >     mContainedDecoder =
> >       DecoderFactory::CreateDecoderForICOResource(DecoderType::PNG,
> >-                                                  WrapNotNull(mContainedSourceBuffer),
> >+                                                  *mContainedIterator,
> >                                                   WrapNotNull(this));
> 
> Since this does a move of mContainedIterator, mContainedIterator is going to
> be an empty iterator (no owner source buffer) most of the time. This seems a
> bit silly to store as a class member.

I use it in nsICODecoder::SniffResource and nsICODecoder::ReadBIH, and I lose context between those calls. It needs to be at the same position as in nsICODecoder::SniffResource. I can change StreamingLexer::Clone to accept an additional offset parameter, or make SniffResource take in the BMP header size (or the entire resource size, if smaller, to ensure we can still read tiny PNGs if they exist).

Comment 24

[Timothy Nikkel (:tnikkel)]

(In reply to Andrew Osmond from comment #23)
> I use it in nsICODecoder::SniffResource and nsICODecoder::ReadBIH, and I
> lose context between those calls. It needs to be at the same position as in
> nsICODecoder::SniffResource. I can change StreamingLexer::Clone to accept an
> additional offset parameter, or make SniffResource take in the BMP header
> size (or the entire resource size, if smaller, to ensure we can still read
> tiny PNGs if they exist).

Maybe change the name so its clear that it is essentially a temp? And since it's a Maybe can you make it go back to being a none after it is transfered to the contained decoder?

Comment 25

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8845911 [details] [diff] [review]
Part 1. Reuse the same SourceBuffer instead of copying, v3

> nsICODecoder::SniffResource(const char* aData)
>     mContainedDecoder =
>       DecoderFactory::CreateDecoderForICOResource(DecoderType::PNG,
>-                                                  WrapNotNull(mContainedSourceBuffer),
>+                                                  *mContainedIterator,
>                                                   WrapNotNull(this));
> 
>-    if (!WriteToContainedDecoder(aData, PNGSIGNATURESIZE)) {
>-      return Transition::TerminateFailure();
>-    }
>-

Do we not need a Flush call here in place of the Write call?

>@@ -382,35 +320,38 @@ nsICODecoder::ReadBIH(const char* aData)
>   // 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))) {
>+  if (!mContainedDecoder->HasSize()) {
>     return Transition::TerminateFailure();
>   }

The comment doesn't match what's happening anymore.

>-  // 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 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. All the caller needs to do is flush when there is new data.
>   LexerResult result = mContainedDecoder->Decode();

Having the IResumable we pass to Decode default to nullptr really doesn't make this callsite clear what is happening. I had to puzzle over it for a while. We should do a followup to make the IResumable a required argument.

>+TEST_F(ImageSourceBuffer, CompleteSuccessWithSmallerReadLength)
>+{
>+  SourceBufferIterator iterator = mSourceBuffer->Iterator(1);
>+
>+  // Write a single byte to the buffer and complete the buffer. (We have to
>+  // write at least one byte because completing a zero length buffer always
>+  // fails; see the ZeroLengthBufferAlwaysFails test.)
>+  CheckedAppendToBuffer(mData, 2);
>+  CheckedCompleteBuffer(iterator, 2);

Looks like you're writing more than a single byte :)

Comment 26

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8845911 [details] [diff] [review]
Part 1. Reuse the same SourceBuffer instead of copying, v3

>diff --git a/image/SourceBuffer.cpp b/image/SourceBuffer.cpp
> SourceBufferIterator
>+SourceBuffer::Iterator(size_t aReadLength)

Can you add /* = SIZE_MAX */ so its clear this has a default parameter

>diff --git a/image/SourceBuffer.h b/image/SourceBuffer.h
>@@ -254,16 +274,18 @@ private:
>     } mIterating;
>     struct {
>       nsresult mStatus;
>     } mAtEnd;
>   } mData;
> 
>   uint32_t mChunkCount;  // Count of chunks we've advanced through.
>   size_t mByteCount;     // Count of bytes in all chunks we've advanced through.
>+  size_t mRemainderToRead; // Count of bytes left to read if there is a maximum
>+                           // imposed by the caller.
> };

The comments in this class are a bit sparse, since it's fresh in your mind would you mind adding comments (in another patch or bug)? It's hard to review this code without reading the code to figure out exactly what the variables mean. mOffset, mAvailableLength, mNextReadLength in mData should be documented.

"chunks we've advanced through" isn't obvious if the chunk we are currently iterating through is included or not.

mRemainderToRead should say it is SIZE_MAX if no limit.

>diff --git a/image/StreamingLexer.h b/image/StreamingLexer.h
>+  SourceBufferIterator Clone(SourceBufferIterator& aIterator,
>+                             size_t aReadLength) const

Can you write a comment explaning what this does? Like "Creates an iterator at the same position but limited to reading aReadLength bytes". Specify where the aReadLength limit applies from (current pos or pos from the start). aReadLength isn't a very clear variable name. Maybe read limit? Something with limit in the name.

Comment 27

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Enforce the parent decoder size (ICO) for child decoders (BMP, PNG)., v1

Comment 28

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 2. The BMP decoder should be responsible for adjusting its size when embedded inside an ICO., v1

Comment 29

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Expose Decoder::IsValidICO for all decoders., v1

Comment 30

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 4. Combine nsICODecoder::ReadBMP and ::ReadPNG., v1

Comment 31

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 5. Add method to clone a SourceBufferIterator when decoding., v1

Comment 32

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 6. Reuse the same SourceBuffer when decoding a resource within an ICO., v1

Comment 33

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 7. Remove unnecessary buffering of BMP header in ICO decoder., v1

Comment 34

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 8. Allow DecoderFactory::CreateDecoderForICOResource to create metadata decoders., v1

Comment 35

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 9. Get the ICO size from the resource instead of the dir entry if unspecified., v1

Comment 36

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 10. Add large embedded PNG/BMP ICO tests., v1

Comment 37

[Andrew Osmond [:aosmond] (he/him)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0f298538e667c91f7dc7b346e91e6c642c4def8e

Comment 38

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8857158 [details] [diff] [review]
Part 1. Enforce the parent decoder size (ICO) for child decoders (BMP, PNG)., v1

>diff --git a/image/decoders/nsIconDecoder.cpp b/image/decoders/nsIconDecoder.cpp
>--- a/image/decoders/nsIconDecoder.cpp
>+++ b/image/decoders/nsIconDecoder.cpp
>@@ -54,16 +54,19 @@ nsIconDecoder::ReadHeader(const char* aD
>   PostSize(width, height);
>+  if (HasError()) {
>+    return Transition::TerminateFailure();
>+  }

I don't think we need this. These don't look to be related to icos.

Comment 39

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8857160 [details] [diff] [review]
Part 3. Expose Decoder::IsValidICO for all decoders., v1

>+  /// Is the image valid if embedded inside an ICO.
>+  virtual bool IsValidICO() const
>+  {
>+    return true;
>+  }
>+

Since this function is now callable on an nsICODecoder it might be confusing to name it this. IsValidInsideAnICO? IsValidICOResource?

I haven't read the remaining patches to know how you are using this, but should this return false and only bmp and png decoders override to return true in some cases?

Comment 40

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8857169 [details] [diff] [review]
Part 5. Add method to clone a SourceBufferIterator when decoding., v1

>+      uint32_t mChunk;   // Unique ID (relative to buffer) of the chunk.

It's just an index, right? Not a unique id.

Comment 41

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8857170 [details] [diff] [review]
Part 6. Reuse the same SourceBuffer when decoding a resource within an ICO., v1

>@@ -329,29 +321,28 @@ nsICODecoder::ReadBIH(const char* aData)
>   // Create a BMP decoder which will do most of the work for us; the exception
>   // is the AND mask, which isn't present in standalone BMPs.
>-  mContainedSourceBuffer = new SourceBuffer();
>-  mContainedSourceBuffer->ExpectLength(mDirEntry.mBytesInRes);
>   mContainedDecoder =
>     DecoderFactory::CreateDecoderForICOResource(DecoderType::BMP,
>-                                                WrapNotNull(mContainedSourceBuffer),
>+                                                Move(*mContainedIterator),
>                                                 WrapNotNull(this),
>                                                 Some(GetRealSize()),
>                                                 Some(dataOffset));

So after this call mContainedIterator should be a iterator with a null owner? Can we assert that and reset mContainedIterator so its None again? Leaving it around seems bad. And add a comment to the definition of mContainedIterator that is only exists for a short period of time?

> LexerTransition<ICOState>
> nsICODecoder::FinishResource()
> {
>+  // We have received all of the data required by the PNG/BMP decoder so
>+  // flushing here guarantees the decode has finished.
>+  if (!FlushContainedDecoder()) {
>+    return Transition::TerminateFailure();
>+  }
>+
>+  MOZ_ASSERT(mContainedDecoder->GetDecodeDone());

If the ico is malformed then we could hit this no? So this should terminate failure?

Comment 42

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tnikkel) from comment #41)
> So after this call mContainedIterator should be a iterator with a null
> owner? Can we assert that and reset mContainedIterator so its None again?
> Leaving it around seems bad. And add a comment to the definition of
> mContainedIterator that is only exists for a short period of time?

Oh, I see the next patch addresses this.

Comment 43

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8857176 [details] [diff] [review]
Part 10. Add large embedded PNG/BMP ICO tests., v1

Shouldn't we include a test where the ico contains a png larger than 256x256?

Comment 44

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Timothy Nikkel (:tnikkel) from comment #38)
> Comment on attachment 8857158 [details] [diff] [review]
> Part 1. Enforce the parent decoder size (ICO) for child decoders (BMP,
> PNG)., v1
> 
> >diff --git a/image/decoders/nsIconDecoder.cpp b/image/decoders/nsIconDecoder.cpp
> >--- a/image/decoders/nsIconDecoder.cpp
> >+++ b/image/decoders/nsIconDecoder.cpp
> >@@ -54,16 +54,19 @@ nsIconDecoder::ReadHeader(const char* aD
> >   PostSize(width, height);
> >+  if (HasError()) {
> >+    return Transition::TerminateFailure();
> >+  }
> 
> I don't think we need this. These don't look to be related to icos.

Hm, I was concerned about somebody ignoring the future errors in PostSize at a later date given the precedent here, but you are right. What I will do instead is add a comment in PostSize indicating not all decoders handle this.

Comment 45

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Timothy Nikkel (:tnikkel) from comment #39)
> Comment on attachment 8857160 [details] [diff] [review]
> Part 3. Expose Decoder::IsValidICO for all decoders., v1
> 
> >+  /// Is the image valid if embedded inside an ICO.
> >+  virtual bool IsValidICO() const
> >+  {
> >+    return true;
> >+  }
> >+
> 
> Since this function is now callable on an nsICODecoder it might be confusing
> to name it this. IsValidInsideAnICO? IsValidICOResource?
> 
> I haven't read the remaining patches to know how you are using this, but
> should this return false and only bmp and png decoders override to return
> true in some cases?

Makes sense, I'll update the name and invert the default.

Comment 46

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Timothy Nikkel (:tnikkel) from comment #41)
> Comment on attachment 8857170 [details] [diff] [review]
> Part 6. Reuse the same SourceBuffer when decoding a resource within an ICO.,
> v1
> 
> >@@ -329,29 +321,28 @@ nsICODecoder::ReadBIH(const char* aData)
> >   // Create a BMP decoder which will do most of the work for us; the exception
> >   // is the AND mask, which isn't present in standalone BMPs.
> >-  mContainedSourceBuffer = new SourceBuffer();
> >-  mContainedSourceBuffer->ExpectLength(mDirEntry.mBytesInRes);
> >   mContainedDecoder =
> >     DecoderFactory::CreateDecoderForICOResource(DecoderType::BMP,
> >-                                                WrapNotNull(mContainedSourceBuffer),
> >+                                                Move(*mContainedIterator),
> >                                                 WrapNotNull(this),
> >                                                 Some(GetRealSize()),
> >                                                 Some(dataOffset));
> 
> So after this call mContainedIterator should be a iterator with a null
> owner? Can we assert that and reset mContainedIterator so its None again?
> Leaving it around seems bad. And add a comment to the definition of
> mContainedIterator that is only exists for a short period of time?
> 
> > LexerTransition<ICOState>
> > nsICODecoder::FinishResource()
> > {
> >+  // We have received all of the data required by the PNG/BMP decoder so
> >+  // flushing here guarantees the decode has finished.
> >+  if (!FlushContainedDecoder()) {
> >+    return Transition::TerminateFailure();
> >+  }
> >+
> >+  MOZ_ASSERT(mContainedDecoder->GetDecodeDone());
> 
> If the ico is malformed then we could hit this no? So this should terminate
> failure?

FlushContainedDecoder will return false if any error occurs. Even if it somehow got past that, GetDecodeDone returns true if PostDecodeDone or PostError are called, if a terminal state is reached (includes truncation) or if it was a metadata decode and got a size. Unless you can think of one, I don't think there is any way to reach this point with GetDecondeDone == false :).

Comment 47

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Timothy Nikkel (:tnikkel) from comment #43)
> Comment on attachment 8857176 [details] [diff] [review]
> Part 10. Add large embedded PNG/BMP ICO tests., v1
> 
> Shouldn't we include a test where the ico contains a png larger than 256x256?

If this was the original code, I would be inclined to agree, but there is no longer anything special about 256x256. If it gets 0x0 in the ICO entry table, it defers to the resource itself -- what specific size it gets from the resource is now irrelevant.

Comment 48

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 1. Enforce the parent decoder size (ICO) for child decoders (BMP, PNG)., v2 [carries r=tnikkel]

Comment 49

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 3. Expose Decoder::IsValidICOResource for all decoders., v1 [carries r=tnikkel]

Comment 50

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 5. Add method to clone a SourceBufferIterator when decoding., v2 [carries r=tnikkel]

Comment 51

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 9. Get the ICO size from the resource instead of the dir entry if unspecified., v2

Rebase.

Comment 52

[Andrew Osmond [:aosmond] (he/him)]

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=60a1bfe8a2e702a4f3dfced29e3581049ca83cd0

Comment 53

[Timothy Nikkel (:tnikkel)]

(In reply to Andrew Osmond [:aosmond] from comment #44)
> (In reply to Timothy Nikkel (:tnikkel) from comment #38)
> > Comment on attachment 8857158 [details] [diff] [review]
> > Part 1. Enforce the parent decoder size (ICO) for child decoders (BMP,
> > PNG)., v1
> > 
> > >diff --git a/image/decoders/nsIconDecoder.cpp b/image/decoders/nsIconDecoder.cpp
> > >--- a/image/decoders/nsIconDecoder.cpp
> > >+++ b/image/decoders/nsIconDecoder.cpp
> > >@@ -54,16 +54,19 @@ nsIconDecoder::ReadHeader(const char* aD
> > >   PostSize(width, height);
> > >+  if (HasError()) {
> > >+    return Transition::TerminateFailure();
> > >+  }
> > 
> > I don't think we need this. These don't look to be related to icos.
> 
> Hm, I was concerned about somebody ignoring the future errors in PostSize at
> a later date given the precedent here, but you are right. What I will do
> instead is add a comment in PostSize indicating not all decoders handle this.

Okay, we can add it to all decoders.

Comment 54

[Timothy Nikkel (:tnikkel)]

(In reply to Andrew Osmond [:aosmond] from comment #46)
 > > If the ico is malformed then we could hit this no? So this should terminate
> > failure?
> 
> FlushContainedDecoder will return false if any error occurs. Even if it
> somehow got past that, GetDecodeDone returns true if PostDecodeDone or
> PostError are called, if a terminal state is reached (includes truncation)
> or if it was a metadata decode and got a size. Unless you can think of one,
> I don't think there is any way to reach this point with GetDecondeDone ==
> false :).

Okay, I see now how truncated can turn into a terminal state.

Comment 55

[Timothy Nikkel (:tnikkel)]

(In reply to Andrew Osmond [:aosmond] from comment #47)
> (In reply to Timothy Nikkel (:tnikkel) from comment #43)
> > Comment on attachment 8857176 [details] [diff] [review]
> > Part 10. Add large embedded PNG/BMP ICO tests., v1
> > 
> > Shouldn't we include a test where the ico contains a png larger than 256x256?
> 
> If this was the original code, I would be inclined to agree, but there is no
> longer anything special about 256x256. If it gets 0x0 in the ICO entry
> table, it defers to the resource itself -- what specific size it gets from
> the resource is now irrelevant.

Well, it's a new feature, so we want to test it so that we know it is working as expected and so it won't be regressed in the future.

Comment 56

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8857486 [details] [diff] [review]
Part 9. Get the ICO size from the resource instead of the dir entry if unspecified., v2

>+  struct IconDirEntryEx : public IconDirEntry {
>+    gfx::IntSize mSize;
>+  };

Can we call this mRealSize? Or something to differentiate it from mWidth/mHeight which is also on this struct.

Also, a general comment on how this decoder works would be good because it is now quite complicated, and different from every other decoder (in that it jumps around).

Comment 57

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8857486 [details] [diff] [review]
Part 9. Get the ICO size from the resource instead of the dir entry if unspecified., v2

Can we change FINISHED_DIR_ENTRY and FinishDirEntry() to FINISHED_DIR_ENTRIES and FinishDirEntries()? Since they happen after we've finished all dir entries, not after we finish one dir entry.

Comment 58

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8857486 [details] [diff] [review]
Part 9. Get the ICO size from the resource instead of the dir entry if unspecified., v2

>+  for (size_t i = 0; i < mDirEntries.Length(); ++i) {
>+    IconDirEntryEx& e = mDirEntries[i];
>+    mImageMetadata.AddNativeSize(e.mSize);
>+
>+    if (!biggestEntry ||
>+        (e.mBitCount >= biggestEntry->mBitCount &&
>+         e.mSize.width * e.mSize.height >=
>+           biggestEntry->mSize.width * biggestEntry->mSize.height)) {

Since these are 32bit now we should make sure this multiplication doesn't overflow.

Comment 59

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8857486 [details] [diff] [review]
Part 9. Get the ICO size from the resource instead of the dir entry if unspecified., v2

>+  uint32_t expectedLength = mMaskRowSize * mDirEntry->mSize.height;

Check for overflow here too. And the following:

>+    memset(mDownscaler->RowBuffer(), 0xFF, mDirEntry->mSize.width * sizeof(uint32_t));

>+    decoded = imageData + mCurrMaskLine * mDirEntry->mSize.width;

>+  uint32_t* decodedRowEnd = decoded + mDirEntry->mSize.width;

Comment 60

[Milan Sreckovic [:milan] (needinfo for best results)]

This will miss 55, right?

Comment 61

[Andrew Osmond [:aosmond] (he/him)]

(In reply to Milan Sreckovic [:milan] from comment #60)
> This will miss 55, right?

Um, yes. But not 56.

try looks clean: https://treeherder.mozilla.org/#/jobs?repo=try&revision=6bec76570dcec9a5cdc01f8534c1d9dbd9b493f2

Comment 62

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 10. Add large embedded PNG/BMP ICO tests., v2

Incorporate feedback, make PNG ICO test 512x512 instead of 256x256 -- r=me.

Comment 63

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd310390a64a
Part 1. Enforce the parent decoder size (ICO) for child decoders (BMP, PNG). r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1eec63b920f
Part 2. The BMP decoder should be responsible for adjusting its size when embedded inside an ICO. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/9780a01b3e94
Part 3. Expose Decoder::IsValidICOResource for all decoders. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/e67f6df41836
Part 4. Combine nsICODecoder::ReadBMP and ::ReadPNG. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fa3ad97ce9c
Part 5. Add method to clone a SourceBufferIterator when decoding. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/e39309b6fe7f
Part 6. Reuse the same SourceBuffer when decoding a resource within an ICO. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/dbae61d1cbee
Part 7. Remove unnecessary buffering of BMP header in ICO decoder. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/d46b7e02802c
Part 8. Allow DecoderFactory::CreateDecoderForICOResource to create metadata decoders. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/18614b05270d
Part 9. Get the ICO size from the resource instead of the dir entry if unspecified. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/abc949687bdc
Part 10. Add large embedded PNG/BMP ICO tests. r=me

Comment 64

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

Backed out for failing GTest's ImageDecoders.LargeICOWithPNGSingleChunk on OS X opt:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8b52b9a7c5f80abc74ab0716b51b9471d28ad6a2
https://hg.mozilla.org/integration/mozilla-inbound/rev/d0bb537fcefe580aaebb059f6879e7bd8b2bfe0e
https://hg.mozilla.org/integration/mozilla-inbound/rev/082df1a7a6414401b5899b6b31c901c3da5c5fc5
https://hg.mozilla.org/integration/mozilla-inbound/rev/a652fc078608bc1086574d6eeb89e45e9e7ce5a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/2df50fe08bc681d471b8f351b8de8337d41adb4e
https://hg.mozilla.org/integration/mozilla-inbound/rev/aee78154a945c91d826a856d7ad1dfd3553d9eeb
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2e062406f6320a608921b0eaa0ffc95dbd9b108
https://hg.mozilla.org/integration/mozilla-inbound/rev/b6cbfeed5f34e75410645c92c6ee2633736d6d68
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2a6c6b64735e03bfc9bfa3975a2664419a06d9d
https://hg.mozilla.org/integration/mozilla-inbound/rev/0322d59fdaa300b77452ab61aa18ca733fffe6b8

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=abc949687bdc774389bdfe12a49f7408373d514f&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=runnable&filter-resultStatus=usercancel
Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=116601152&repo=mozilla-inbound

21:36:56     INFO -  TEST-START | ImageDecoders.LargeICOWithPNGSingleChunk
21:36:56  WARNING -  TEST-UNEXPECTED-FAIL | ImageDecoders.LargeICOWithPNGSingleChunk |       Expected: aColor.mBlue
21:36:56     INFO -        Which is: '\0'
21:36:56     INFO -  To be equal to: data[i + 0]
21:36:56     INFO -        Which is: '\x5' (5) @ /home/worker/workspace/build/src/image/test/gtest/Common.cpp:174
21:36:56  WARNING -  TEST-UNEXPECTED-FAIL | ImageDecoders.LargeICOWithPNGSingleChunk | Value of: IsSolidColor(surface, BGRAColor::Green(), aTestCase.mFlags & TEST_CASE_IS_FUZZY ? 1 : 0)
21:36:56     INFO -    Actual: false
21:36:56     INFO -  Expected: true @ /home/worker/workspace/build/src/image/test/gtest/TestDecoders.cpp:89
21:36:56  WARNING -  TEST-UNEXPECTED-FAIL | ImageDecoders.LargeICOWithPNGSingleChunk | test completed (time: 3ms)
21:36:56     INFO -  TEST-START | ImageDecoders.LargeICOWithPNGMultiChunk
21:36:56  WARNING -  TEST-UNEXPECTED-FAIL | ImageDecoders.LargeICOWithPNGMultiChunk |       Expected: aColor.mBlue
21:36:56     INFO -        Which is: '\0'
21:36:56     INFO -  To be equal to: data[i + 0]
21:36:56     INFO -        Which is: '\x5' (5) @ /home/worker/workspace/build/src/image/test/gtest/Common.cpp:174
21:36:56  WARNING -  TEST-UNEXPECTED-FAIL | ImageDecoders.LargeICOWithPNGMultiChunk | Value of: IsSolidColor(surface, BGRAColor::Green(), aTestCase.mFlags & TEST_CASE_IS_FUZZY ? 1 : 0)
21:36:56     INFO -    Actual: false
21:36:56     INFO -  Expected: true @ /home/worker/workspace/build/src/image/test/gtest/TestDecoders.cpp:89
21:36:56  WARNING -  TEST-UNEXPECTED-FAIL | ImageDecoders.LargeICOWithPNGMultiChunk | test completed (time: 5ms)

Comment 65

[Andrew Osmond [:aosmond] (he/him)]

Failed my own gtest after a full try run, thinking "updating the size should be fine, I'll just run locally on Linux before landing." Sigh.

Comment 66

[Andrew Osmond [:aosmond] (he/him)]

Attachment: Part 10b. Force CMS output profile to be sRGB for gtests., v1

This should silence such nonsense forever as I suspect the OS X colour profile was not sRGB.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=df13747a56e21b888668e392aaa2144f99728b00

Comment 67

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/cd85620ea21d
Part 1. Enforce the parent decoder size (ICO) for child decoders (BMP, PNG). r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/6fef952ac780
Part 2. The BMP decoder should be responsible for adjusting its size when embedded inside an ICO. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/92d1377f6bb8
Part 3. Expose Decoder::IsValidICOResource for all decoders. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/94eda3aa3808
Part 4. Combine nsICODecoder::ReadBMP and ::ReadPNG. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ff4eca8da26
Part 5. Add method to clone a SourceBufferIterator when decoding. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/6cf887326d88
Part 6. Reuse the same SourceBuffer when decoding a resource within an ICO. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/80c948a98dca
Part 7. Remove unnecessary buffering of BMP header in ICO decoder. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/1b2f04e53ea2
Part 8. Allow DecoderFactory::CreateDecoderForICOResource to create metadata decoders. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/44311d857010
Part 9. Get the ICO size from the resource instead of the dir entry if unspecified. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/6a5ba604200e
Part 10a. Add large embedded PNG/BMP ICO tests. r=me
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b3ee47d2f8f
Part 10b. Force CMS output profile to be sRGB for gtests. r=me

Comment 69

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/cd85620ea21d
https://hg.mozilla.org/mozilla-central/rev/6fef952ac780
https://hg.mozilla.org/mozilla-central/rev/92d1377f6bb8
https://hg.mozilla.org/mozilla-central/rev/94eda3aa3808
https://hg.mozilla.org/mozilla-central/rev/1ff4eca8da26
https://hg.mozilla.org/mozilla-central/rev/6cf887326d88
https://hg.mozilla.org/mozilla-central/rev/80c948a98dca
https://hg.mozilla.org/mozilla-central/rev/1b2f04e53ea2
https://hg.mozilla.org/mozilla-central/rev/44311d857010
https://hg.mozilla.org/mozilla-central/rev/6a5ba604200e
https://hg.mozilla.org/mozilla-central/rev/6b3ee47d2f8f