858872 - Make Favicon service a little smarter about failed favicons

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

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

We can be a little smarter about times when we fail to download or decode a favicon. Right now, if we fail to get a favicon, for whatever reason, we still attempt to fetch it again the next time the UI wants it, even if we are fairly certain we'll just fail again.

The initial thought is to:
1. Remember times when we do not successfully (HTTP 200) fetch a favicon and store the failure and the timestamp for the attempt. If we request the favicon again, within a short time span, just ignore the request. After the short time span, try the fetch again.

2. If we successful fetch the favicon, but the mimetype is wrong, like getting HTML from a captive portal, either do the same as #1 with a much shorter timeout or just try again next time. It would be nice if we got an event/callback on captive portal logins so we could use that as a "flush the failure" event.

Comment 1

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

It looks like desktop holds onto failed favicons until a force reload is needed or the cache of failed favicons fills up. Otherwise there is no "timeout".

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsFaviconService.cpp#222
http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/nsFaviconService.cpp#440

Comment 2

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

Attachment: patch

This patch adds a simple failed favicon cache to the favicon service. It includes:
* Store any pageUrl that fails to download as "failed"
  * HTTP failed favicons are never dropped from the cache unless we need more space and LruCache should handle that
  * Invalid content type failed favicons are given a short 5 minute expiration, assuming that it might be a captive portal issue.
* Valid content-type is defined as any content-type with "image" in it (Weak I know)
* The LruCache holds 64 items. Desktop holds 256 in theirs. 64 seems like a lot of failed favicons though.

Should we fail other situations? Like the pageUrl is not http/https?

Comment 3

[Brian Nicholson (:bnicholson)]

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

Review of attachment 734200 [details] [diff] [review]:
-----------------------------------------------------------------

Could we just reuse the existing mFaviconsCache, using some error type as the value for failed favicons, and a size of 1 for sizeOf? It seems like overkill to create a whole new data structure for essentially the same thing -- a mapping of favicon URLs to their results. We can't set null in LruCache directly, but we could easily create a wrapper type that stores a bitmap reference (and we could set that to null). Then we wouldn't need most of this other code; if the favicon is null, just add it to the cache like we do for all other favicon results.

To keep things simple, we could just ignore failures for likely captive portal responses and always try to refetch them. Sites serving real responses with the wrong favicon MIME type are probably rare, and because favicons are exposed so prominently in our UI, not having a favicon for Google or some other site for 5 minutes as a consequence doesn't seem worth it to me. Then again, maybe I'm biased because I abhor arbitrary timeouts...

If you really want timeouts for captive portal favicons, I would still keep the single mFaviconsCache, and you could just add a timeout field to the wrapper type. You could even add an isExpired() method to the wrapper class to encapsulate that logic.

But in case you don't like any of these ideas, some comments below:

::: mobile/android/base/Favicons.java
@@ +71,5 @@
>                  return image.getRowBytes() * image.getHeight();
>              }
>          };
> +
> +        // Create a failed favicon memory cache that have up to 64 entries

Nit: s/have/has/

@@ +113,5 @@
>          }
>  
> +        // Check if favicon has failed
> +        if (isFailedFavicon(pageUrl)) {
> +            dispatchResult(null, null, listener);

This should still return the pageUrl as the first argument.

@@ +296,5 @@
>              Bitmap image = null;
>              try {
>                  HttpGet request = new HttpGet(faviconUrl.toURI());
> +                HttpResponse response = getHttpClient().execute(request);
> +                if (response != null && response.getStatusLine() != null) {

If you're adding null checks, you should probably just return early if response is null. Otherwise, you'll just hit a NPE below (at "response.getEntity();").

@@ +303,5 @@
> +                    if (status >= 400)
> +                        putFaviconInFailedCache(mPageUrl, FAILED_EXPIRY_NEVER);
> +                }
> +                HttpEntity entity = response.getEntity();
> +                if (entity != null && entity.getContentType() != null) {

Same thing here; you'll hit a NPE with "new BufferedHttpEntity(entity);" if entity is null, so you could just return.

Comment 4

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

Attachment: patch v2

I knida like having a separate cache. I don't like overloading the main cache for failed entries too. I removed the timeout for captive portals (bad content types). I like that LruCache manages the list for me, but since it wants a key/value, I left the Long for now. We can use it for a flag. I'm open to other approaches too, but I like the explicit cache.

Comment 5

[Brian Nicholson (:bnicholson)]

Comment on attachment 734312 [details] [diff] [review]
patch v2

Review of attachment 734312 [details] [diff] [review]:
-----------------------------------------------------------------

Looks fine other than remaining null check issues.

::: mobile/android/base/Favicons.java
@@ +297,5 @@
> +                        putFaviconInFailedCache(mPageUrl, FAILED_EXPIRY_NEVER);
> +                        return null;
> +                    }
> +                }
> +                HttpEntity entity = response.getEntity();

If response is null above, this will still result in a NPE. It will be caught by the surrounding try/catch block, but it can be easily avoided. I would do something like "if (response == null) { return null; }" above.

@@ +308,1 @@
>                  BufferedHttpEntity bufferedEntity = new BufferedHttpEntity(entity);

Same thing here with entity.

Comment 6

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

pushed with null checks added
https://hg.mozilla.org/integration/mozilla-inbound/rev/7f5c58257d65

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7f5c58257d65