765155 - Missing associative icons for recommended add-on listing on about:home

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Adrian Tamas (:AdrianT)]

Attachment: screenshot

Firefox Mobile Native 14 beta 7 build 3/ Nightly 16.0a1 2012-06-14
Device: HTC Desire (Andoid 2.2), LG Optimus 2X (Android 2.2)

Steps to reproduce:
1. Install the latest build and open the app.
2. Observe the about:home page.
3. Load a few pages in different tabs. Close the app.
4. Reopen the app and observe the about home page.

Expected results:
The icons for the addons are displayed.

Actual results:
The icons are displayed with a new profile but after the default addon suggestions are changed the icons are no longer displayed.

Notes:
The issue is not reproducible on Android 2.3 and above - HTC Desire Z (Andorid 2.3.3) and Galaxy Nexus (Android 4.0.2)

Comment 1

[Aaron Train [:aaronmt]]

Errors in console?

Comment 2

[Adrian Tamas (:AdrianT)]

Attachment: startup logs

Attaching logs for Firefox startup and loading about:home

Comment 3

[Aaron Train [:aaronmt]]

Kevin/Naoki, can you try out your Froyo device?

Comment 4

[Naoki Hirata :nhirata (please use needinfo instead of cc)]

reproduced on the htc desire 2.2 device, 6/18/2012 nightly

3 tabs open, quit, reopened to about:home

It pushes the addons section down and only one shows, which doesn't show the icon.

Comment 5

[Adrian Tamas (:AdrianT)]

I am able to reproduce the issue also on Samsung Galaxy R running Android 2.3.4 on Firefox Mobile 15 Beta 3 build 1

Comment 6

[Adrian Tamas (:AdrianT)]

Issue is also reproducible on Samsung Galaxy Tab (Android 3.2) and Asus Transformer TF101 (Android 4.0.3). Removing Froyo from the title since this does not seem to be restricted to Froyo devices anymore.

Comment 8

[Aaron Train [:aaronmt]]

Attachment: Nightly (04/03) - Screenshot

For whatever reason different pairings are not showing icons; as well there should be one for 'Quit Now'.

Comment 9

[Brian Nicholson (:bnicholson)]

The requests for these favicons result in an HTTP 302 redirect. For example,

http://addons.cdn.mozilla.net/img/uploads/addon_icons/413/413482-32.png?modified=1365005325

redirects to

https://static.addons.mozilla.net/img/uploads/addon_icons/413/413482-32.png?modified=1365005325

To fix this, we need to handle redirects in our Favicons class (having to deal with these special HTTP cases is another reason we should consider using Necko for favicon requests).

But even with this fixed, I think we should consider dropping favicons from about:home altogether. Even if our favicon implementation is updated to support redirects, a number of things can prevent them from loading correctly (the user is behind a captive portal, the user currently has no network signal, the hosting site is down, etc.). If any of these things happen, we don't store the result in the database. And that means that we try to fetch them again every time we inflate about:home, including orientation changes.

Comment 10

[Brian Nicholson (:bnicholson)]

Attachment: Handle HTTP redirects for favicons

This may not be the cause for all of them, but this at least fixes the QuitNow favicon for me.

Even if we decide to drop favicons on about:home, we should still handle this for any pages that link to favicons that redirect.

Comment 11

[Brian Nicholson (:bnicholson)]

Attachment: Handle HTTP redirects for favicons, v2

Fixed so that we don't try to decode the image if we don't have one.

Comment 12

[Aaron Train [:aaronmt]]

Is UX planning to remove addon recommendations from about:home anyways?

Comment 13

[Brian Nicholson (:bnicholson)]

Filed bug 858293 to remove addon favicons from about:home.

(In reply to Aaron Train [:aaronmt] from comment #12)
> Is UX planning to remove addon recommendations from about:home anyways?

Not sure, but I think we should land and uplift bug 858293 until they do.

Comment 14

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

(In reply to Brian Nicholson (:bnicholson) from comment #9)
> The requests for these favicons result in an HTTP 302 redirect. For example,
> 
> http://addons.cdn.mozilla.net/img/uploads/addon_icons/413/413482-32.
> png?modified=1365005325
> 
> redirects to
> 
> https://static.addons.mozilla.net/img/uploads/addon_icons/413/413482-32.
> png?modified=1365005325

Crap. We are changing from HTTP to HTTPS and Java won't automatically do the redirect.

We actually change HTTPS to HTTP when we write out the recommended-addons.json file. We do that because in XUL Fennec using HTTPS means loading NSS, which slowed down startup time.

We could probably stop converting HTTPS to HTTP since we load in Java.

Question: Since _we_ are the ones breaking this due to the protocol change, should we even take this patch? The Java HTTP code should auto redirect on it's own as long as the protocol stays the same. I would vote to stop changing the protocol and not land this patch.

Thoughts?

> But even with this fixed, I think we should consider dropping favicons from
> about:home altogether. Even if our favicon implementation is updated to
> support redirects, a number of things can prevent them from loading
> correctly (the user is behind a captive portal, the user currently has no
> network signal, the hosting site is down, etc.). If any of these things
> happen, we don't store the result in the database. And that means that we
> try to fetch them again every time we inflate about:home, including
> orientation changes.

Good point. Looking at the other bug now.

Comment 15

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #14)
> (In reply to Brian Nicholson (:bnicholson) from comment #9)
> > The requests for these favicons result in an HTTP 302 redirect. For example,
> > 
> > http://addons.cdn.mozilla.net/img/uploads/addon_icons/413/413482-32.
> > png?modified=1365005325
> > 
> > redirects to
> > 
> > https://static.addons.mozilla.net/img/uploads/addon_icons/413/413482-32.
> > png?modified=1365005325
> 
> Crap. We are changing from HTTP to HTTPS and Java won't automatically do the
> redirect.
> 
> We actually change HTTPS to HTTP when we write out the
> recommended-addons.json file. We do that because in XUL Fennec using HTTPS
> means loading NSS, which slowed down startup time.
> 
> We could probably stop converting HTTPS to HTTP since we load in Java.
> 
> Question: Since _we_ are the ones breaking this due to the protocol change,
> should we even take this patch? The Java HTTP code should auto redirect on
> it's own as long as the protocol stays the same. I would vote to stop
> changing the protocol and not land this patch.
> 
> Thoughts?

Note that this fixes redirections for *all* favicons, and not just the add-on favicons listed on about:home. It's not common enough to be a big deal if don't want this patch, but I just wanted to point out that this covers more than just about:home.

For about:home I agree we should look into pointing to the correct favicon URL (or not loading favicons at all!).

Comment 16

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

Comment on attachment 733584 [details] [diff] [review]
Handle HTTP redirects for favicons, v2

Let's hold off on the full blown redirect handling for now. We hope it should not be necessary.

Comment 17

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

Attachment: simple patch

The simple fix here is to stop changing the icon URL protocol from https to http. Then the normal redirect should work.

I plan to file other bugs for other fixes to improve the favicon service and about:home.

Comment 18

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

https://hg.mozilla.org/integration/mozilla-inbound/rev/99ab49e3aebb

Comment 19

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

Comment on attachment 734177 [details] [diff] [review]
simple patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Java re-write and AMO redirect changes
User impact if declined: We will download and try to redirect the 1 or 2 add-ons on about:home many, many times while the browser is used.
Testing completed (on m-c, etc.): just landed
Risk to taking this patch (and alternatives if risky): very low risk
String or IDL/UUID changes made by this patch: none

This patch should result in a responsiveness win and is pretty simple.

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/99ab49e3aebb

Comment 21

[bhavana bajaj [:bajaj]]

Comment on attachment 734177 [details] [diff] [review]
simple patch

Very low risk patch with a responsiveness win.

Requesting QA verification here once the patch lands on aurora & beta.

Comment 22

[Teodora Vermesan (:TeoVermesan)]

The icons for the addons are displayed on about:home
Verified fixed on:
-build: Firefox for Android 23.0a1(2013-04-08)
-device: Samsung Galaxy Nexus
-OS: Android 4.1.1

Comment 23

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

https://hg.mozilla.org/releases/mozilla-aurora/rev/bb8a8984c520
https://hg.mozilla.org/releases/mozilla-beta/rev/cefb5e955455

Comment 24

[Teodora Vermesan (:TeoVermesan)]

Verified fixed on:
-build: Firefox for Android 22.0a2(2013-04-17) and Firefox for Android 21.0b3(2013-04-16)
-device: LG  Nexus 4
-OS: Android 4.2.2