Closed Bug 442731 Opened 14 years ago Closed 14 years ago

GIF favicons are not resampled in places.sqlite (large icons are stored)

Categories

(Firefox :: Bookmarks & History, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
Firefox 3.5

People

(Reporter: finny08, Assigned: mak)

References

()

Details

Attachments

(1 file, 1 obsolete file)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0

Arbitrarily large gif favicons are stored in the places.sqlite file without being resampled.  This could be used to bloat the places database to any size.  (Only seems to be a problem with gif images, but I haven't tested the gamut).

Reproducible: Always

Steps to Reproduce:
1. Create a local html file, and set the favicon to a very large gif file.  Or see http://signsofarmageddon.blogspot.com for a demo (19kb image stored in places.sqlite as 19kb of data).
Actual Results:  
Full GIF image is stored in places.sql, without resampling (19kb of BLOB data).

Expected Results:  
Resampled GIF image should be stored, it should be < 1kb.

Using default theme.
if i interpret correctly what happens, decodeImageData in OptimizeFaviconImage in nsFaviconService is not handling the  image correctly, it fails and we go on saving the larger image even if it exceeds our 1024 limit

  if (aDataLen > 1024) {
    rv = OptimizeFaviconImage(aData, aDataLen, aMimeType, newData, newMimeType);
    if (NS_SUCCEEDED(rv) && newData.Length() < aDataLen) {
      data = reinterpret_cast<PRUint8*>(const_cast<char*>(newData.get())),
      dataLen = newData.Length();
      mimeType = &newMimeType;
    }
  }

probably we should simply refuse to save the image at that point if we cannot get a good resampled one.
Confirming since i see the same thing (19KB image/gif saved to the db and not resampled to a 16x16 png).
Status: UNCONFIRMED → NEW
Ever confirmed: true
So the problem in decodeImageData appear to be related to nsGifDecoder2

imgTools::decodeImageData calls 
  rv = decoder->Flush();
  NS_ENSURE_SUCCESS(rv, rv);

and for gif that is
NS_IMETHODIMP nsGIFDecoder2::Flush()
{
    return NS_ERROR_NOT_IMPLEMENTED;
}

while usually for other decoders (bmp, ico, png, ...) it's simply doing a return NS_OK;

So appears for gifs we are returning failing from flush()
fixing gif decoder flush to return ns_ok and make the service refuse to save if optimizeImage is failing should be enough, i'll do some testing.

Don't know who is the image library peer actually though, trying CC Pavlov to have feedback if that change is possible or not.
I just realized that this is helpful when using animated gifs as a favicon.  Perhaps that was the reason in the first place?  Still, maybe there should be some maximum size.
Attached patch patch (obsolete) — Splinter Review
a possible solution, change gif decoder to return NS_OK in ::flush (like other decoders do actually), set a maximum size for favicons (10Kb in this patch), if we are over that, SetFaviconData will throw NS_ERROR_FAILURE (but we can still go on, i'm unsure if it wouldn't be better simply early returning NS_OK and document that the data *could* potentially not be saved if too bloated, Dietrich?).

(In reply to comment #3)
> I just realized that this is helpful when using animated gifs as a favicon. 
> Perhaps that was the reason in the first place?  Still, maybe there should be
> some maximum size.

Due to the kind of error here (a not expected failure in the decoder) i think that the animated gif saving was something that come as an unwanted interesting feature, if we want to maintain it we should try to skip the optimization for animated gifs and simply do a check on maximum size, could be filled as an enhancement.
Attachment #330740 - Flags: review?(dietrich)
Comment on attachment 330740 [details] [diff] [review]
patch

r=me. please get second review from the libpr0n module owner.
Attachment #330740 - Flags: review?(dietrich) → review+
Assignee: nobody → mak77
Status: NEW → ASSIGNED
Attachment #330740 - Flags: review?(pavlov)
asking review to pavlov for the gifDecoder change
Attachment #330740 - Flags: review?(pavlov) → review+
Keywords: checkin-needed
Comment on attachment 330740 [details] [diff] [review]
patch

this is rotted, due to changes that may affect it. can you please make a new patch?
Attached patch unbitrottedSplinter Review
Attachment #330740 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/98df12d0881e
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 3.1
OS: Windows XP → All
Hardware: PC → All
verified: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b2pre) Gecko/20081112 Minefield/3.1b2pre
Status: RESOLVED → VERIFIED
Target Milestone: Firefox 3.1 → Firefox 3.5
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.