819973 - Some Favicons disappear shortly after awesomebar opens

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: Awesomescreen, defect)

Description

[Gian-Carlo Pascutto [:gcp]]

Cloning the original; something was fixed, but this bug still reproduces for me.

+++ This bug was initially created as a clone of Bug #808510 +++

Galaxy Tab 10.1, Android 3.2

About half of the time the Awesomebar is opened, some favicons for the topsites are visible, but then disappear a split second later. They never come back. Most sites are missing their favicons.

Sometimes the reverse happens: no favicons are visible, and after a split second some appear.

My impression is that there are two disjoint set of favicons and I'm seeing a switch between the two.

Comment 1

[Gian-Carlo Pascutto [:gcp]]

This bug now made it to beta - I'm seeing it in Beta from the Store on the Galaxy 10.1/3.2 and also on an new Galaxy 3/Android 4.1.2 with a new, barely used profile.

I've attempted to see if the problem produces with only the fix for https://bugzilla.mozilla.org/show_bug.cgi?id=808510#c5 and no other commits, but the database format has evolved since then so the profile I have isn't usable for it.

This is an annoying problem which is at risk of making it to release, so increasing severity.

Comment 2

[Lukas Blakk [:lsblakk] use ?needinfo]

This is indeed annoying but that does not make it a severe issue, it is still cosmetic.  We would consider a low risk uplift if one was available but will not track for release.

Comment 3

[Brian Nicholson (:bnicholson)]

Using gcp's profile, I've discovered that the problem favicons being evicted from the LRU cache. The favicon for reddit is a massive 255kB; storing just 4 favicons of this size would exceed our cache capacity (1MB). And since favicons are stored per page URL and not per favicon URL, we do in fact try to store many huge identical favicons in the cache.

To fix this bug, we should make sure favicons are already downsized before storing them in the cache (we should also see some performance improvements from doing this). We can do that either by downsizing favicons before they're stored in the database, or we can store them at their original size and downsize them later when they're accessed. I think the latter option might be the best so we don't have to worry about downsizing synced icons, and it also gives us access to the original favicon in case we decide to change their sizes in the future.

To prevent redundancy, we should also store favicons in the cache per favicon URL, not page URL. I filed bug 834536 for this.

Comment 4

[Wesley Johnston (:wesj)]

The reddit bit seems strange. What are we storing? The only favicon I can see there is a 3kb ico file (with a 16x16 and a 32x32 icon horribly compressed in it). A 128x128 bmp is 65kB, but we can compress that before we store it.

I wouldn't be surprised that some sites provide extra large, huge favicons every now and then. I think we need to support that to some extent. i.e. We want to encourage sites to use nice high res favicons. I worry we're just going to throw them away.

Comment 5

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #4)
> The reddit bit seems strange. What are we storing? The only favicon I can
> see there is a 3kb ico file (with a 16x16 and a 32x32 icon horribly
> compressed in it). A 128x128 bmp is 65kB, but we can compress that before we
> store it.

Digging into the database, it's actually only a 12kB PNG (but it's 256x256, which will be 256kB as an uncompressed bitmap). That's good - that means we just need to worry about making them a reasonable size before caching them.

The image being used is http://www.redditstatic.com/icon.png, which is given in a <link> on reddit.

Comment 6

[Brian Nicholson (:bnicholson)]

Attachment: Scale favicons before storing them in the cache

This fixes the main issue, but it'd be nice to fix bug 834536 too.

Comment 7

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 706237 [details] [diff] [review]
Scale favicons before storing them in the cache

Could Favicons.scaleImage return null? I think it could. I see other parts of the code like:

>                 if (image != null)
>-                    return image;
>+                    return scaleImage(image);
>             }

Where returning null is probably not good.

Also happens here:

>                 if (favicon == null)
>                     continue;
> 
>+                favicon = Favicons.getInstance().scaleImage(favicon);

In general I like the change, but I have this nagging feeling that Favicons is not doing enough behind the scenes, like a good encapsulating utility service, but I can't offer up any ways to improve it yet.

It would be nice if the scaling call was never needed externally, but we might not be able to make that happen.

r- for dealing with the null return from scaleImage

Comment 8

[Brian Nicholson (:bnicholson)]

Comment on attachment 706237 [details] [diff] [review]
Scale favicons before storing them in the cache

We looked at this and it didn't seem like createScaledBitmap() could return null, so re-requesting review.

Comment 9

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 706237 [details] [diff] [review]
Scale favicons before storing them in the cache

I still think we should look for ways to encapsulate more favicon stuff into the Favicon class. That's do that in future bugs, as we find ways to improve it.

Comment 10

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4e0b83a23621

Comment 11

[Brian Nicholson (:bnicholson)]

Comment on attachment 706237 [details] [diff] [review]
Scale favicons before storing them in the cache

[Approval Request Comment]
Bug caused by (feature/regressing bug #): bug 785945 (Fx19)
User impact if declined: after visiting certain sites that supply large favicons (such as reddit), favicons will no longer appear in the AwesomeScreen for any sites
Testing completed (on m-c, etc.): just landed m-i
Risk to taking this patch (and alternatives if risky): low risk
String or UUID changes made by this patch: none

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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

Comment 13

[bhavana bajaj [:bajaj]]

Comment on attachment 706237 [details] [diff] [review]
Scale favicons before storing them in the cache

Low risk, cosmetic polish for a Fx 19 meta bug(785945)..please land asap to get it into 19.0b4

Comment 14

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/81838a4b5052

Comment 15

[Brian Nicholson (:bnicholson)]

https://hg.mozilla.org/releases/mozilla-beta/rev/329574982da3