Closed Bug 765155 Opened 11 years ago Closed 10 years ago

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

Categories

(Firefox for Android Graveyard :: General, defect)

14 Branch
ARM
Android
defect
Not set
normal

Tracking

(firefox16 wontfix, firefox17 wontfix, firefox18 wontfix, firefox19 wontfix, firefox20 wontfix, firefox21 verified, firefox22 verified, firefox23 verified)

VERIFIED FIXED
Firefox 23
Tracking Status
firefox16 --- wontfix
firefox17 --- wontfix
firefox18 --- wontfix
firefox19 --- wontfix
firefox20 --- wontfix
firefox21 --- verified
firefox22 --- verified
firefox23 --- verified

People

(Reporter: AdrianT, Assigned: mfinkle)

References

Details

Attachments

(4 files, 2 obsolete files)

Attached image 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)
Errors in console?
Attached file startup logs
Attaching logs for Firefox startup and loading about:home
Kevin/Naoki, can you try out your Froyo device?
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.
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
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.
Summary: Addon icons are missing on about:home on Froyo devices → Addon icons are missing on about:home
Summary: Addon icons are missing on about:home → Missing associative icons for recommended add-on listing on about:home
For whatever reason different pairings are not showing icons; as well there should be one for 'Quit Now'.
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.
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.
Assignee: nobody → bnicholson
Status: NEW → ASSIGNED
Attachment #733580 - Flags: review?(mark.finkle)
Fixed so that we don't try to decode the image if we don't have one.
Attachment #733580 - Attachment is obsolete: true
Attachment #733580 - Flags: review?(mark.finkle)
Attachment #733584 - Flags: review?(mark.finkle)
Is UX planning to remove addon recommendations from about:home anyways?
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.
(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.
(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 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.
Attachment #733584 - Flags: review?(mark.finkle) → review-
Attached patch simple patchSplinter Review
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.
Assignee: bnicholson → mark.finkle
Attachment #734177 - Flags: review?(bnicholson)
Attachment #734177 - Flags: review?(bnicholson) → review+
Attachment #733584 - Attachment is obsolete: true
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.
Attachment #734177 - Flags: approval-mozilla-beta?
Attachment #734177 - Flags: approval-mozilla-aurora?
https://hg.mozilla.org/mozilla-central/rev/99ab49e3aebb
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 23
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.
Attachment #734177 - Flags: approval-mozilla-beta?
Attachment #734177 - Flags: approval-mozilla-beta+
Attachment #734177 - Flags: approval-mozilla-aurora?
Attachment #734177 - Flags: approval-mozilla-aurora+
Keywords: qawanted, verifyme
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
Blocks: 859800
Status: RESOLVED → VERIFIED
Keywords: qawanted, verifyme
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
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.