858293 - Don't show favicons for addons listed on about:home

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Brian Nicholson (:bnicholson)]

We should drop favicons from about:home. A number of things can prevent favicons from loading correctly, including the user being behind a captive portal, the user having no network signal, the hosting site is down, the favicon moved (see bug 765155), 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.

In general, I think it's a bad idea to have about:home fire off any network requests. It will slow down startup since the requests go through the same background thread we use for other tasks. And if there are any errors as outlined above, things will be even worse.

Comment 1

[Wesley Johnston (:wesj)]

(In reply to Brian Nicholson (:bnicholson) from comment #0)
> In general, I think it's a bad idea to have about:home fire off any network
> requests. It will slow down startup since the requests go through the same
> background thread we use for other tasks. And if there are any errors as
> outlined above, things will be even worse.

To be fair, both these issues are solvable.

Comment 2

[Brian Nicholson (:bnicholson)]

Attachment: Don't fetch favicons for about:home addons

Comment 3

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #1)
> (In reply to Brian Nicholson (:bnicholson) from comment #0)
> > In general, I think it's a bad idea to have about:home fire off any network
> > requests. It will slow down startup since the requests go through the same
> > background thread we use for other tasks. And if there are any errors as
> > outlined above, things will be even worse.
> 
> To be fair, both these issues are solvable.

Both? The fact that everything goes through the same background thread, yes. But we don't have control over the multiple factors listed in comment 0 that could prevent the favicon from loading.

Comment 4

[Wesley Johnston (:wesj)]

No, but be could store a timestamp and not try to request known bad urls frequently (or even just maintain a small in memory HashMap or urls to ignore).

Comment 5

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #4)
> No, but be could store a timestamp and not try to request known bad urls
> frequently (or even just maintain a small in memory HashMap or urls to
> ignore).

Define "bad URL"? If the user is behind a captive portal, or the site is down, or the network isn't connected, the favicon will fail to load in all of these cases. But if the user signs into the captive portal or regains network connectivity, we should try again. It's impossible to distinguish an invalid favicon being served by the site from a bad favicon given by the captive portal.

Comment 6

[Wesley Johnston (:wesj)]

(In reply to Brian Nicholson (:bnicholson) from comment #5)
> (In reply to Wesley Johnston (:wesj) from comment #4)
> > No, but be could store a timestamp and not try to request known bad urls
> > frequently (or even just maintain a small in memory HashMap or urls to
> > ignore).
> 
> But if the user signs into the captive portal or regains
> network connectivity, we should try again.

That's why I said we could try to do check less frequently and not never. We can/should also monitor network connectivity changes and flush these sorts of caches. And not bother even trying if we're offline.

I think 'network might fail so don't use network' is a valid argument when you're talking about things integral to Fennec (I wouldn't load about:home or browser.js over the network), but this isn't that case. That said, these are so below the fold in about:home that I think they're basically useless.

Comment 7

[Brian Nicholson (:bnicholson)]

(In reply to Wesley Johnston (:wesj) from comment #6)
>
> I think 'network might fail so don't use network' is a valid argument when
> you're talking about things integral to Fennec (I wouldn't load about:home
> or browser.js over the network), but this isn't that case.

Actually, it is kind of the case at the moment (which is why I think this bug is important). We have QuitNow as a recommended add-on. The favicon for QuitNow doesn't load. This means we do a network request for the favicon every time about:home is inflated, which is done on our background thread. We also do all other about:home updates on this same background thread. So our showing about:home is effectively queued behind a network request which prevents anything from appearing until finished (which is why I filed this to begin with).

As you said, we could fix this in part by changing the thread we use for fetching favicons. And we could also try to figure out some caching system for favicons (which we should tie into bug 699785). But since about:home is a critical component, a more reliable/much easier fix would be to prevent these requests from happening in the first place.

Comment 8

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

(In reply to Wesley Johnston (:wesj) from comment #1)
> (In reply to Brian Nicholson (:bnicholson) from comment #0)
> > In general, I think it's a bad idea to have about:home fire off any network
> > requests. It will slow down startup since the requests go through the same
> > background thread we use for other tasks. And if there are any errors as
> > outlined above, things will be even worse.
> 
> To be fair, both these issues are solvable.

Yes, and we could be smarter about NOT trying to re-download a favicon that we failed to get last time. Maybe use a timeout? Desktop does this too, IIRC.

Comment 9

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

Comment on attachment 733600 [details] [diff] [review]
Don't fetch favicons for about:home addons

>diff --git a/mobile/android/base/widget/AddonsSection.java b/mobile/android/base/widget/AddonsSection.java
>             zip = new ZipFile(applicationPackage);
>-            if (zip == null)
>-                return null;

Why remove this check?

>-                        Favicons favicons = Favicons.getInstance();
>-                        favicons.loadFavicon(pageUrl, iconUrl, true,
>-                                    new Favicons.OnFaviconLoadedListener() {
>-                            @Override
>-                            public void onFaviconLoaded(String url, Bitmap favicon) {
>-                                if (favicon != null) {
>-                                    Drawable drawable = new BitmapDrawable(favicon);
>-                                    drawable.setBounds(sIconBounds);
>-                                    row.setCompoundDrawables(drawable, null, null, null);
>-                                }
>-                            }
>-                        });
>-

What if we add a flag here to only return a favicon if it is in the cache already? Then we just need a way to get the favicon in the cache, but not during about:home layout.

In general, we need to check with Ian about this.

Comment 10

[Brian Nicholson (:bnicholson)]

(In reply to Mark Finkle (:mfinkle) from comment #9)
> Comment on attachment 733600 [details] [diff] [review]
> Don't fetch favicons for about:home addons
> 
> >diff --git a/mobile/android/base/widget/AddonsSection.java b/mobile/android/base/widget/AddonsSection.java
> >             zip = new ZipFile(applicationPackage);
> >-            if (zip == null)
> >-                return null;
> 
> Why remove this check?

Just cleanup; that's dead code since "new" can never return null.

> 
> >-                        Favicons favicons = Favicons.getInstance();
> >-                        favicons.loadFavicon(pageUrl, iconUrl, true,
> >-                                    new Favicons.OnFaviconLoadedListener() {
> >-                            @Override
> >-                            public void onFaviconLoaded(String url, Bitmap favicon) {
> >-                                if (favicon != null) {
> >-                                    Drawable drawable = new BitmapDrawable(favicon);
> >-                                    drawable.setBounds(sIconBounds);
> >-                                    row.setCompoundDrawables(drawable, null, null, null);
> >-                                }
> >-                            }
> >-                        });
> >-
> 
> What if we add a flag here to only return a favicon if it is in the cache
> already? Then we just need a way to get the favicon in the cache, but not
> during about:home layout.

I think that will basically happen if we move the favicon fetching to a new thread (it only does the request if it's in the DB, and the loading will happen in parallel with about:home loading instead of blocking the background thread). But for the case of QuitNow or other missing favicons, we'll still end up doing lots of requests for the favicon (every time we open about:home, every time we rotate, and even every time we close the AwesomeScreen since that triggers an onResume()).

Comment 11

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

(In reply to Brian Nicholson (:bnicholson) from comment #10)

> But for the case of QuitNow or other missing favicons,
> we'll still end up doing lots of requests for the favicon (every time we
> open about:home, every time we rotate, and even every time we close the
> AwesomeScreen since that triggers an onResume()).

What if we make a HashMap of failed favicons and the timestamp we last checked. After 60 minutes, we can try again.

Comment 12

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

(In reply to Mark Finkle (:mfinkle) from comment #11)
> (In reply to Brian Nicholson (:bnicholson) from comment #10)
> 
> > But for the case of QuitNow or other missing favicons,
> > we'll still end up doing lots of requests for the favicon (every time we
> > open about:home, every time we rotate, and even every time we close the
> > AwesomeScreen since that triggers an onResume()).
> 
> What if we make a HashMap of failed favicons and the timestamp we last
> checked. After 60 minutes, we can try again.

Oh and sorry, we should be talking about this in the "Make Favicon service smarter" bug, not here. This bug is all about removing the icon from add-ons on about:home.

If the "Make Favicon service smarter" bug makes this bug less of an issue, then OK. Also, we should still stop changing HTTPS to HTTP:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/AddonUpdateService.js#173
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/AddonUpdateService.js#179
http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/AddonUpdateService.js#180

Comment 13

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

Ian - Thoughts on removing the icon from the add-on rows? I supposed we'd leave the generic "add-on" icon we see now.

Comment 14

[Lucas Rocha (:lucasr)]

(In reply to Brian Nicholson (:bnicholson) from comment #0)
> In general, I think it's a bad idea to have about:home fire off any network
> requests. It will slow down startup since the requests go through the same
> background thread we use for other tasks. And if there are any errors as
> outlined above, things will be even worse.

Could you please file a bug for the everything-runs-on-the-same-background-thread issue? We should probably use a thread pool instead a single background thread to allow more parallel work whenever possible.

We should probably keep the current background thread for the set of tasks that require to be run in the same background thread. DB access calls come to mind, there might be others. But there many other types of tasks for which we don't need to know which thread they'll be running on (as long as it's not the UI thread).

Comment 15

[Ian Barlow (:ibarlow)]

(In reply to Mark Finkle (:mfinkle) from comment #13)
> Ian - Thoughts on removing the icon from the add-on rows? I supposed we'd
> leave the generic "add-on" icon we see now.

I'm not really crazy about that idea tbh... The icons are a nice identifier and add some interest to the list. 

This will likely come up again with the multipage about:home redesign... I wonder if we can make add-ons more interesting in a different way. Maybe by pulling in part of the description to explain what they do.

Comment 16

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

I filed bug 858872 to make the Favicon service a little smarter. I also added a patch to bug 765155 that stops the add-on icons from failing to load, making us hit the cache.

WONTFIXing this bug based on the other bugs and comment 15