705732 - only fetch favicons from http or https

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Doug Turner (:dougt)]

During our testing, we load lots of files over file:.  This results in extra cycles trying to fetch favicons from file:/ urls which tend to not exist.  In this patch, I simply test for a value uri that is http or https.

Comment 1

[Doug Turner (:dougt)]

Attachment: patch v.1

Comment 2

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

Comment on attachment 577296 [details] [diff] [review]
patch v.1

We can load favicons from chrome: and resource: too

The MalformedURIException check look good, but let's not add the rest. Shouldn't the malformed URI check be in Favicons.loadFavicon ?

Comment 3

[Doug Turner (:dougt)]

lucas, can you take a look?

Comment 4

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

Lucase, we'd like to minimize the need to do any performance-sensitive code. We should try to bail out as soon as possible. The current patch is a bit to restrictive though. We could look in Gecko to see if the nsFaviconService (.cpp) does anything to limit and bail out early.

Comment 5

[Lucas Rocha (:lucasr)]

(In reply to Doug Turner (:dougt) from comment #3)
> lucas, can you take a look?

Ideally, we'd check the protocol just after http://mxr.mozilla.org/projects-central/source/birch/mobile/android/base/Favicons.java#361 where we already check for malformed favicon URLs. The advantage would be that we would have less code duplication. However, if we really want to bail out asap, I agree with Mark that this code should be moved to Favicons.loadFavicon.

I'd prefer to blacklist the protocols we definitely don't want to load favicons for instead of whitelisting a couple of them (which is a bit too restrictive, as Mark said). Add a private method like "boolean isValidProtocol()" which is called at the top of loadFavicon().

Mark, if the favicon code is supposed to handle chrome: and resource:, I suggest to file a bug to track that.

Comment 6

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

(In reply to Lucas Rocha (:lucasr) from comment #5)

> Mark, if the favicon code is supposed to handle chrome: and resource:, I
> suggest to file a bug to track that.

My bad. We already convert chrome: and resource: URLs to file: URLs using resolveGeckoURI:
http://mxr.mozilla.org/projects-central/source/birch/mobile/android/chrome/content/browser.js#1257

resolveGeckoURI will return a file: URL, so we don't want to blacklist file: either.

Another thought: If a favicon URL gives us a 404, can we cache that for a short time so we stop trying to fetch a favicon from that URL for a short time?

Comment 7

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

Should we have favicons for about pages?

11-30 16:25:51.366: D/GeckoFavicons(2773): The provided URL is not valid: java.net.MalformedURLException: Unknown protocol: about

Comment 8

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

(In reply to Naoki Hirata :nhirata from comment #7)
> Should we have favicons for about pages?
> 
> 11-30 16:25:51.366: D/GeckoFavicons(2773): The provided URL is not valid:
> java.net.MalformedURLException: Unknown protocol: about

Yes, about: pages are allowed to supply favicons via a <link> but not by about:xxx/favicon.ico

We should not try to fetch default favicons for about: pages.

Comment 9

[Lucas Rocha (:lucasr)]

Given that we do handle favicons for about: pages now (bug 708525), what's left to be done here?