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

Status: VERIFIED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[finny08]

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.

Comment 1

[Marco Bonardo [:mak]]

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).

Comment 2

[Marco Bonardo [:mak]]

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.

Comment 3

[finny08]

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.

Comment 4

[Marco Bonardo [:mak]]

Attachment: patch

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.

Comment 5

[Dietrich Ayala (:dietrich)]

Comment on attachment 330740 [details] [diff] [review]
patch

r=me. please get second review from the libpr0n module owner.

Comment 6

[Marco Bonardo [:mak]]

asking review to pavlov for the gifDecoder change

Comment 7

[Dietrich Ayala (:dietrich)]

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?

Comment 8

[Marco Bonardo [:mak]]

Attachment: unbitrotted

Comment 9

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/98df12d0881e

Comment 10

[Tracy Walker [:tracy]]

verified: Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.4; en-US; rv:1.9.1b2pre) Gecko/20081112 Minefield/3.1b2pre

Comment 11

[Gervase Markham [:gerv]]

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