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

RESOLVED FIXED in Firefox 56

Status

()

defect
RESOLVED FIXED
3 years ago
11 months ago

People

(Reporter: jonas, Assigned: aosmond)

Tracking

({regression})

43 Branch
mozilla56
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox-esr52 wontfix, firefox54 wontfix, firefox55 wontfix, firefox56 fixed)

Details

()

Attachments

(11 attachments, 18 obsolete attachments)

7.87 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.67 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
15.47 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
9.27 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
5.83 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
9.94 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
5.10 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
13.81 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
28.44 KB, patch
tnikkel
: review+
Details | Diff | Splinter Review
6.08 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
3.83 KB, patch
aosmond
: review+
Details | Diff | Splinter Review
Reporter

Description

3 years ago
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
Reporter

Updated

3 years ago
I can confirm this report with Firefox 49.0.2 on windows7
Status: UNCONFIRMED → NEW
Component: Untriaged → ImageLib
Ever confirmed: true
Product: Firefox → Core

Comment 2

3 years ago
Regression window:
m-c [2015-09-19, 2015-09-20]
Has Regression Range: --- → no
Has STR: --- → yes
OS: Unspecified → All
Hardware: Unspecified → All
Assignee

Updated

3 years ago
Assignee: nobody → aosmond
Assignee

Comment 3

3 years ago
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

Updated

3 years ago
Has Regression Range: no → yes
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?
Assignee

Comment 6

3 years ago
(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 :).
(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.
Reporter

Comment 8

3 years ago
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)
Assignee

Comment 9

3 years ago
Posted patch Accept big ICOs, v2 (obsolete) — Splinter Review
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
Attachment #8808623 - Attachment is obsolete: true
Attachment #8808623 - Flags: review?(tnikkel)
Attachment #8815707 - Flags: review?(tnikkel)

Comment 10

2 years ago
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)
Assignee

Updated

2 years ago
Status: NEW → ASSIGNED
Assignee

Comment 11

2 years ago
Attachment #8815707 - Attachment is obsolete: true
Attachment #8815707 - Flags: review?(tnikkel)
Attachment #8845447 - Flags: review?(tnikkel)
Assignee

Comment 13

2 years ago
(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).
Assignee

Comment 14

2 years ago
Attachment #8845447 - Attachment is obsolete: true
Attachment #8845447 - Flags: review?(tnikkel)
Assignee

Comment 15

2 years ago
Attachment #8845448 - Attachment is obsolete: true
Attachment #8845448 - Flags: review?(tnikkel)
Assignee

Comment 17

2 years ago
Some minor refactoring, no functional change intended.
Attachment #8845547 - Attachment is obsolete: true
Assignee

Comment 18

2 years ago
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.
Attachment #8845591 - Attachment is obsolete: true
Assignee

Comment 19

2 years ago
Enforce a read limit on the iterator to make sure the contained decoder can't overrun the resource size.
Attachment #8845537 - Attachment is obsolete: true
Attachment #8845911 - Flags: review?(tnikkel)
Assignee

Comment 20

2 years ago
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).
Attachment #8845607 - Attachment is obsolete: true
Attachment #8845912 - Flags: review?(tnikkel)
Assignee

Comment 21

2 years ago
Rebase on top of bug 1343499.
Attachment #8807984 - Attachment is obsolete: true
Attachment #8845912 - Attachment is obsolete: true
Attachment #8845912 - Flags: review?(tnikkel)
Attachment #8850496 - Flags: review?(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

2 years ago
(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).
(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 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 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.
Assignee

Updated

2 years ago
Blocks: 1351201
Assignee

Comment 27

2 years ago
Attachment #8845911 - Attachment is obsolete: true
Attachment #8850496 - Attachment is obsolete: true
Attachment #8845911 - Flags: review?(tnikkel)
Attachment #8850496 - Flags: review?(tnikkel)
Attachment #8857158 - Flags: review?(tnikkel)
Assignee

Comment 36

2 years ago
Attachment #8857176 - Flags: review?(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.
Attachment #8857158 - Flags: review?(tnikkel) → review+
Attachment #8857159 - Flags: review?(tnikkel) → review+
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?
Attachment #8857160 - Flags: review?(tnikkel) → review+
Attachment #8857161 - Flags: review?(tnikkel) → review+
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.
Attachment #8857169 - Flags: review?(tnikkel) → review+
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?
Attachment #8857171 - Flags: review?(tnikkel) → review+
(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.
Attachment #8857172 - Flags: review?(tnikkel) → review+
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?
Assignee

Comment 44

2 years ago
(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.
Assignee

Comment 45

2 years ago
(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.
Assignee

Comment 46

2 years ago
(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 :).
Assignee

Comment 47

2 years ago
(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.
Assignee

Updated

2 years ago
Attachment #8857482 - Attachment description: Part 1. Enforce the parent decoder size (ICO) for child decoders (BMP, PNG)., v2 → Part 1. Enforce the parent decoder size (ICO) for child decoders (BMP, PNG)., v2 [carries r=tnikkel]
Assignee

Comment 51

2 years ago
Rebase.
Attachment #8857173 - Attachment is obsolete: true
Attachment #8857173 - Flags: review?(tnikkel)
Attachment #8857486 - Flags: review?(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.
(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.
(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.
Attachment #8857170 - Flags: review?(tnikkel) → review+
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 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 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 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;
Attachment #8857486 - Flags: review?(tnikkel) → review+
This will miss 55, right?
Flags: needinfo?(aosmond)
Assignee

Comment 61

2 years ago
(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
Flags: needinfo?(aosmond)
Assignee

Comment 62

2 years ago
Incorporate feedback, make PNG ICO test 512x512 instead of 256x256 -- r=me.
Attachment #8857176 - Attachment is obsolete: true
Attachment #8857176 - Flags: review?(tnikkel)
Attachment #8889017 - Flags: review+

Comment 63

2 years ago
Pushed by aosmond@gmail.com:
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
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)
Flags: needinfo?(aosmond)
Assignee

Comment 65

2 years ago
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.
Assignee

Comment 66

2 years ago
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
Flags: needinfo?(aosmond)
Attachment #8889044 - Flags: review+

Comment 67

2 years ago
Pushed by aosmond@gmail.com:
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
Assignee

Updated

2 years ago
Duplicate of this bug: 1284034
Assignee

Updated

2 years ago
Duplicate of this bug: 1351201
Flags: in-testsuite+
Version: 51 Branch → 43 Branch
Assignee

Updated

2 years ago
Depends on: 1383579
Depends on: 1385409
Assignee

Updated

2 years ago
Depends on: 1388590
Assignee

Updated

2 years ago
Duplicate of this bug: 1223319
Depends on: 1472520
You need to log in before you can comment on or make changes to this bug.