914027 - (DecodeOnlyPossibles) Don't attempt to decode favicons in unsupported formats

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Favicon Handling, defect)

Description

[Chris Kitching [:ckitching]]

Given a Favicon which declares itself (via the link type field) as being of a format we are incapable of decoding (eg. text/svg), we nevertheless boldly attempt to decode it, before failing.
Since multiple icons of different sizes may be specified, we should instead fall back to a different icon if the most attractive one is not appropriate. We can, for a start, disregard icons which contain a type field telling us we can't decode it.
If things start going wrong during the decoding phase, we should fallback again, until we have exhausted the possibilities[1]

[1]http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#rel-icon

Comment 2

[Chris Kitching [:ckitching]]

Attachment: Don't try decoding undecodables, fallback if you fail.

In this patch, we check the link type field to see if the reported mime type is one of the ones we know how to decode. If it isn't, we ignore it.

Where multiple Favicons are specified on the page, we now select the largest, most attractive one and if, despite being ostensibly of a type we can decode (Or of unspecified type), we fail to decode it, then we fall back to the next most attractive icon.
As such, webpages which specify SVG icons as well as bitmap icons will now no longer fail to display a Favicon.

Comment 3

[Chris Kitching [:ckitching]]

Attachment: V2 - Don't decode undecodable Favicons. Fallback where appropriate.

It helps if I upload the right patch.

Comment 4

[Chris Kitching [:ckitching]]

Attachment: V3 - Don't decode undecodable Favicons. Fallback where appropriate.

For real this time. It also helps if you hg add things...

Comment 5

[Chris Kitching [:ckitching]]

Attachment: V4 - Don't decode undecodable Favicons. Fallback where appropriate.

Unbitrot.

Comment 6

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

Comment on attachment 801761 [details] [diff] [review]
V4 - Don't decode undecodable Favicons. Fallback where appropriate.


>diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java

>     public void onTabChanged(Tab tab, Tabs.TabEvents msg, Object data) {

>             case LOCATION_CHANGE:
>-                if (Tabs.getInstance().isSelectedTab(tab)) {
>-                    maybeCancelFaviconLoad(tab);
>-                }

Are we doing this any more? I thought the purpose was to stop any in-progress downloads since we are changing the page.

>diff --git a/mobile/android/base/favicons/Favicons.java b/mobile/android/base/favicons/Favicons.java

>+    private static final HashSet<String> sContainerMimeTypes = new HashSet<String>();
>+    static {

>+        sContainerMimeTypes.add("application/ico");
>+        sDecodableMimeTypes.addAll(sContainerMimeTypes);

Add a blank line to separate the containers

>+        sDecodableMimeTypes.add("image/x-ms-bmp");
>+        sDecodableMimeTypes.add("image/ms-bmp");
>+        sDecodableMimeTypes.add("image/ms-bmp");

Last one is duped

>+    public static boolean canDecodeType(String aImgType) {

>+    public static boolean isContainerType(String aImgType) {

Can we use aMimeType to better convey what this is?

>diff --git a/mobile/android/base/favicons/RemoteFavicon.java b/mobile/android/base/favicons/RemoteFavicon.java

>+    public int compareTo(RemoteFavicon aObj) {

>+        if(mDeclaredSize == aObj.mDeclaredSize) {

if (


>diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js

>-            size: maxSize
>+            size: maxSize,
>+            imgType: target.hasAttribute("type") ? target.getAttribute("type") : ""

imgType -> mime  (we use it in a few other JSON message places)


I am wondering if this new favicon process (all the new patches, not just this one) have an adverse affect on performance, especially for page loading. Might be worthwhile to get some Try talos runs.

Comment 7

[Chris Kitching [:ckitching]]

Attachment: V5 - Don't decode undecodable Favicons. Fallback where appropriate.

Many thanks for the drive-by nitpick, mfinkle.

(In reply to Mark Finkle (:mfinkle) from comment #6)
> Are we doing this any more? I thought the purpose was to stop any
> in-progress downloads since we are changing the page.

Quite right, but this is now handled in Tabs, before the event handled in this changed code is dispatched. It seemed strange for an object to dispatch an event and have it handled just to call a method on the dispatcher, when the dispatcher could, more simply, have just called the method themselves at dispatch-time.

> I am wondering if this new favicon process (all the new patches, not just
> this one) have an adverse affect on performance, especially for page
> loading. Might be worthwhile to get some Try talos runs.

Are Talos runs sufficiently reliable to get meaningful information about changes in performance as small as what is likely to have been created here? (Although I could of course have done something really stupid that makes a huge difference)
I ask because last time I messed with Talos I was seeing consistent 11-18% improvements due to Proguard when measured one way, but when using Talos I found, essentially, noise. (some runs claimed 40% slowdown, some reported 60% speedup, etc. - Just madness that didn't demadnessify when averaged over 10 runs, at which point I gave up)

Still. I'll push a try run and retrigger talos a large number of times - see what we get. Certainly, need to ensure correctness has been sustained.

Comment 8

[Chris Kitching [:ckitching]]

Try push of the whole Favicon saga:
https://tbpl.mozilla.org/?tree=Try&rev=22902a49b391

Feel free to push the talos retrigger buttons if you happen to wander past when the buttons have appeared and I've yet to push them. (There's no way to request that try redoes something several times at submit-time, is there? You have to just go click the buttons?)

Comment 9

[Chris Kitching [:ckitching]]

My profuset apologies for that complete waste of resources.
Here's one that actually compiles:
https://tbpl.mozilla.org/?tree=Try&rev=1050f2cfea85

Comment 10

[Chris Kitching [:ckitching]]

Attachment: V6 - Don't decode undecodable Favicons. Fallback where appropriate.

Fixing the ensuing orange party...
https://tbpl.mozilla.org/?tree=Try&rev=c436a0f95ee9

Comment 11

[:Margaret Leibovic]

Comment on attachment 802153 [details] [diff] [review]
V6 - Don't decode undecodable Favicons. Fallback where appropriate.

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

Nice cleanup work. I just have some comments for you to address before we land this.

::: mobile/android/base/Tab.java
@@ +45,5 @@
>      private String mBaseDomain;
>      private String mUserSearch;
>      private String mTitle;
> +    // The set of all available Favicons for this tab, sorted by attractiveness.
> +    private TreeSet<RemoteFavicon> mAvailableFavicons = new TreeSet<RemoteFavicon>();

What are the benefits of a TreeSet? (I'm unfamiliar with the data type)

@@ +343,5 @@
>      public boolean getHasTouchListeners() {
>          return mHasTouchListeners;
>      }
>  
> +    public synchronized void addFavicon(String aFaviconURL, int aFaviconSize, String aMimeType) {

I've mentioned this in some other recent reviews, let drop the a prefix on parameter names.

@@ +344,5 @@
>          return mHasTouchListeners;
>      }
>  
> +    public synchronized void addFavicon(String aFaviconURL, int aFaviconSize, String aMimeType) {
> +        RemoteFavicon thisFavicon = new RemoteFavicon(aFaviconURL, aFaviconSize, aMimeType);

Is there a "not this" favicon? If not, I think this variable name could just be favicon.

@@ +351,5 @@
> +        mAvailableFavicons.add(thisFavicon);
> +
> +        if (mAvailableFavicons.first() == thisFavicon) {
> +            // We have a new "best" Favicon. If we were loading one, abort and do the new one.
> +            if (mFaviconLoadId != Favicons.NOT_LOADING) {

Combine these if conditions?

@@ +368,5 @@
> +            if (!newFavicon.mFaviconURL.equals(mFaviconUrl)) {
> +                Favicons.cancelFaviconLoad(mFaviconLoadId);
> +            } else {
> +                return;
> +            }

I think it would be clearer to re-arrange this as:

if (newFavicon.mFaviconURL.equals(mFaviconUrl) {
    return;
}

Favicons.cancelFaviconLoad(mFaviconLoadId);

@@ +379,2 @@
>  
> +        int flags = ((isPrivate() || mErrorType != ErrorType.NONE) ? 0 : LoadFaviconTask.FLAG_PERSIST);

You can get rid of the extra set of parens around the outside.

@@ +379,3 @@
>  
> +        int flags = ((isPrivate() || mErrorType != ErrorType.NONE) ? 0 : LoadFaviconTask.FLAG_PERSIST);
> +        mFaviconLoadId = Favicons.getFaviconForSize(mUrl, mFaviconUrl, 128, flags,

Let's move this 128 value into a constant, with a comment explaining what it's used for.

::: mobile/android/base/Tabs.java
@@ +466,1 @@
>                  notifyListeners(tab, TabEvents.LINK_FAVICON);

Do we still need this? It looks like we can get rid of this notification and just directly do the tab.loadFavicon business in here.

::: mobile/android/base/favicons/Favicons.java
@@ +47,5 @@
> +    // Mime types of things we are capable of decoding.
> +    private static final HashSet<String> sDecodableMimeTypes = new HashSet<String>();
> +
> +    // Mime types of things we are both capable of decoding and are container formats (May contain
> +    // multiple different sizes of image)

Where are you getting all of these types from? Please add a link to whatever documentation you were following, so that we know how to maintain this in the future.

@@ +439,5 @@
> +     * Helper function to determine if we can decode a particular mime type.
> +     * @param aImgType Mime type to check for decodability.
> +     * @return false if the given mime type is certainly not decodable, true if it might be.
> +     */
> +    public static boolean canDecodeType(String aImgType) {

Same nit about the a prefix. I'll stop repeating myself everywhere now, just remember to update this wherever applicable :)

::: mobile/android/base/favicons/RemoteFavicon.java
@@ +26,5 @@
> +        mMimeType = aMimeType;
> +    }
> +
> +    @Override
> +    public boolean equals(Object o) {

Add some documentation explaining what it takes for two RemoteFavicons to be considered equal.

@@ +39,5 @@
> +    /**
> +     * Establish ordering on Favicons. Lower value in the partial ordering is a "better" Favicon.
> +     * @param aObj Object to compare against.
> +     * @return -1 if this Favicon is "better" than the other, zero if they are equivalent in quality,
> +     *         based on the information here, 1 if the other appears "better" than this one.

You probably noticed that right now we only send one size per link tag:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#3428

A nice follow-up would be to properly process link tags with multiple sizes declared in them. This would probably depend on your ICO decoder work.

@@ +44,5 @@
> +     */
> +    @Override
> +    public int compareTo(RemoteFavicon aObj) {
> +        // Size "any" trumps everything else.
> +        if (mDeclaredSize == -1) {

Pull this into a constant, so it's easier to understand that -1 is a magic number.

::: mobile/android/chrome/content/browser.js
@@ +3441,5 @@
>              charset: target.ownerDocument.characterSet,
>              title: target.title,
>              rel: list.join(" "),
> +            size: maxSize,
> +            mime: target.hasAttribute("type") ? target.getAttribute("type") : ""

It would be more concise (and marginally more efficient) to just do:

mine: target.getAttribute("type") || ""

Comment 12

[Chris Kitching [:ckitching]]

Attachment: V7 - Don't decode undecodable Favicons. Fallback where appropriate.

(In reply to :Margaret Leibovic from comment #11)
> ::: mobile/android/base/Tab.java
> @@ +45,5 @@
> >      private String mBaseDomain;
> >      private String mUserSearch;
> >      private String mTitle;
> > +    // The set of all available Favicons for this tab, sorted by attractiveness.
> > +    private TreeSet<RemoteFavicon> mAvailableFavicons = new TreeSet<RemoteFavicon>();
> 
> What are the benefits of a TreeSet? (I'm unfamiliar with the data type)

It's a red-black tree wrapped up in some Java-esque-ness which implements the SortedSet interface. Elements are kept in an order defined by their natural ordering (Or a comparator), and the maximum/minimum can be extracted. We want these semantics because we're using this to keep a queue of favicons to load in order of attractiveness.

While a red-black tree is perhaps a tad overkill given the small number of elements we're going to be having in this set, the only other implementation of SortedSet available in the standard Java libaray is ConcurrentSkipListSet (A SortedSet backed by a ConcurrentSkipListMap), which is way, way more overkill (And hugely more expensive to use).

Using an implementation of SortedSet keeps the code nice and concise.

A small performance optimisation may result if you created a linked list based implementation of SortedSet and used that instead (Except in cases where websites define a large number of Favicons - which are very much in the minority).

 
> ::: mobile/android/base/Tabs.java
> @@ +466,1 @@
> >                  notifyListeners(tab, TabEvents.LINK_FAVICON);
> 
> Do we still need this? It looks like we can get rid of this notification and
> just directly do the tab.loadFavicon business in here.

Correct. This implements the concept from the patch Bug 759501 here. Let's do it.

> @@ +439,5 @@
> > +     * Helper function to determine if we can decode a particular mime type.
> > +     * @param aImgType Mime type to check for decodability.
> > +     * @return false if the given mime type is certainly not decodable, true if it might be.
> > +     */
> > +    public static boolean canDecodeType(String aImgType) {
> 
> Same nit about the a prefix. I'll stop repeating myself everywhere now, just
> remember to update this wherever applicable :)

Sorry for the repeats. I thought this was a pattern that was in active use in the codebase, not one that was dying out. You end up repeating yourself because you're working through a cluster of patches all of which were written before the first time you mentioned it - sorry about that.
 
> @@ +39,5 @@
> > +    /**
> > +     * Establish ordering on Favicons. Lower value in the partial ordering is a "better" Favicon.
> > +     * @param aObj Object to compare against.
> > +     * @return -1 if this Favicon is "better" than the other, zero if they are equivalent in quality,
> > +     *         based on the information here, 1 if the other appears "better" than this one.
> 
> You probably noticed that right now we only send one size per link tag:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/
> browser.js#3428
> 
> A nice follow-up would be to properly process link tags with multiple sizes
> declared in them. This would probably depend on your ICO decoder work.

Bug 914952.
Also, while researching this one I found Bug 914950 and Bug 914948.

Comment 13

[Chris Kitching [:ckitching]]

Attachment: V8 - Don't decode undecodable Favicons. Fallback where appropriate.

Unbreaking the try run, some cleanup, addressing your comments.

Comment 14

[Tom Most]

image/x-icon is accepted for ICO by IE.  It should be on the list.  It's also recommended by lots of webdev articles, e.g., http://www.sitepoint.com/favicon-a-changing-role/ so it's likely common.

Comment 15

[:Margaret Leibovic]

Comment on attachment 802835 [details] [diff] [review]
V8 - Don't decode undecodable Favicons. Fallback where appropriate.

Clearing review until bug 914296 lands. Don't worry about unbitrotting this until your patch for that bug actually makes its way into the tree :)

Comment 16

[Chris Kitching [:ckitching]]

Attachment: V9 - Don't decode undecodable Favicons. Fallback where appropriate.

Rebasing atop the latest underlying patch.

The objective of this patch is to prevent us from grabbing an undecodable favicon and failing.
Peviously, we'd start trying to load a favicon as soon as we see a link tag and then use it. There was no way of selecting the preferred favicon from the collection of link tags provided, and no support for trying a different one if the one we try has failed.

In this patch, we introduce a whitelist of supported mime-types. We will not bother trying to download anything that reports to not be of one of these types.
We do not try to fetch anything until we have seen every link tag on the page and arranged them into order of appealingness. We then try them sequentially, in descending order of niceness, until we find one that works.
If we exhaust the list, we try the default favicon URL, before finally giving up.

Comment 17

[Aaron Train [:aaronmt]]

Resolved?

Comment 18

[Chris Kitching [:ckitching]]

Attachment: V11: Don't decode undecodable favicons.

Well, this is a blast from the past.

This remains unresolved, though I have ten month old patches sitting around that fix this, Bug 914952, and Bug 920331 (at least as originally scoped).

Here's my patch (suitably updated for the present day)

Try:
https://tbpl.mozilla.org/?tree=Try&rev=752d60a4248f

Comment 19

[Richard Newman [:rnewman]]

Comment on attachment 8460736 [details] [diff] [review]
V11: Don't decode undecodable favicons.

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

Well, this is bigger than I expected... 'cos it fixes a bunch of bugs!

This doesn't seem particularly urgent. If anything, Bug 914952 seems more important for quality.

Will digest more this week, but I spotted one bug when skimming...

::: mobile/android/base/favicons/Favicons.java
@@ +522,5 @@
> +     * @param mimeType Mime type to check.
> +     * @return true if the given mime type is a container type, false otherwise.
> +     */
> +    public static boolean isContainerType(String mimeType) {
> +        return sDecodableMimeTypes.contains(mimeType);

sContainerMimeTypes

Comment 20

[Chris Kitching [:ckitching]]

(In reply to Richard Newman [:rnewman] from comment #19)
> This doesn't seem particularly urgent. If anything, Bug 914952 seems more
> important for quality.

You're right: I've got patches for both sitting about, I'll add unbitrotting the other one to the list of things to do when bored.

These two suffered from having the patches written right after I authored the mega favicon bug, that then took a month to get through review. This lot them bitrotted and got forgotten about :/

> Will digest more this week, but I spotted one bug when skimming...

Well spotted.. *facepalm*

Comment 21

[Chris Kitching [:ckitching]]

Attachment: Do not attempt to decode Favicons of unsupported types. Fallback on Favicon failure

Unbitrotted to do other work: patch got somewhat cleaner.

Comment 22

[Richard Newman [:rnewman]]

Is the "V11" patch obsolete, Chris?

Comment 23

[Chris Kitching [:ckitching]]

Comment on attachment 8460736 [details] [diff] [review]
V11: Don't decode undecodable favicons.

Yes. Sorry - started using bzexport, got my paperwork a bit muddled on this one.

Comment 24

[Richard Newman [:rnewman]]

Comment on attachment 8492499 [details] [diff] [review]
Do not attempt to decode Favicons of unsupported types. Fallback on Favicon failure

Margaret reviewed an earlier version of this, and I'm keen on not being the only sucker who does favicon reviews, so 303 to her :D

Comment 25

[:Margaret Leibovic]

Comment on attachment 8492499 [details] [diff] [review]
Do not attempt to decode Favicons of unsupported types. Fallback on Favicon failure

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

This is a nice improvement. r+ with comments addressed.

::: mobile/android/base/Tabs.java
@@ +503,5 @@
>                  notifyListeners(tab, Tabs.TabEvents.LOADED);
>              } else if (event.equals("DOMTitleChanged")) {
>                  tab.updateTitle(message.getString("title"));
>              } else if (event.equals("Link:Favicon")) {
> +                boolean mimeSpecified = message.has("mime");

The "Link:Favicon" message will always have a "mime" property, so we can skip this check.

@@ -504,5 @@
>              } else if (event.equals("DOMTitleChanged")) {
>                  tab.updateTitle(message.getString("title"));
>              } else if (event.equals("Link:Favicon")) {
> -                tab.updateFaviconURL(message.getString("href"), message.getInt("size"));
> -                notifyListeners(tab, TabEvents.LINK_FAVICON);

You should also delete LINK_FAVICON from the TabEvents enum.

@@ +511,5 @@
> +                    return;
> +                }
> +
> +                // Add the favicon to the set of available icons for this tab.
> +                tab.addFavicon(message.getString("href"), message.getInt("size"), mimeSpecified ? message.getString("mime") : "");

You can also get rid of the mimeSpecified check here.

@@ +514,5 @@
> +                // Add the favicon to the set of available icons for this tab.
> +                tab.addFavicon(message.getString("href"), message.getInt("size"), mimeSpecified ? message.getString("mime") : "");
> +
> +                // Load the favicon.
> +                tab.loadFavicon();

Given the current logic, we only want to load the favicon if the tab isn't currently loading. I think we should keep the behavior the same, and add a `tab.getState() != Tab.STATE_LOADING` check around here.

@@ -543,5 @@
> -            String tabURL = tab.getURL();
> -            if (pageURL.equals(tabURL)) {
> -                tab.setFaviconLoadId(Favicons.NOT_LOADING);
> -                if (tab.updateFavicon(favicon)) {
> -                    notifyListeners(tab, TabEvents.FAVICON);

I'm so happy to see this logic moving into Tab.

::: mobile/android/base/favicons/RemoteFavicon.java
@@ +56,5 @@
> +     * @return -1 if this Favicon is "better" than the other, zero if they are equivalent in quality,
> +     *         based on the information here, 1 if the other appears "better" than this one.
> +     */
> +    @Override
> +    public int compareTo(RemoteFavicon obj) {

Nice, this is a much better way to implement support for this "sizes" attribute.

::: mobile/android/chrome/content/browser.js
@@ +3908,5 @@
>              type: "Link:Favicon",
>              tabID: this.id,
>              href: resolveGeckoURI(target.href),
> +            size: maxSize,
> +            mime: target.getAttribute("type") || ""

I wonder how often websites specify the wrong thing here. But I suppose if they specify the wrong type it's their own fault if things don't work properly.

Comment 26

[Chris Kitching [:ckitching]]

(In reply to :Margaret Leibovic from comment #25)
> @@ +514,5 @@
> > +                // Add the favicon to the set of available icons for this tab.
> > +                tab.addFavicon(message.getString("href"), message.getInt("size"), mimeSpecified ? message.getString("mime") : "");
> > +
> > +                // Load the favicon.
> > +                tab.loadFavicon();
> 
> Given the current logic, we only want to load the favicon if the tab isn't
> currently loading. I think we should keep the behavior the same, and add a
> `tab.getState() != Tab.STATE_LOADING` check around here.

I've done that for now, but note that doing this is exactly what we need if we want to do Bug 759501. 
Unclear if it's a good idea: loading the icon earlier might have a detrimental effect on page load time. Hard to say.
Thoughts?

Comment 27

[Chris Kitching [:ckitching]]

https://hg.mozilla.org/integration/fx-team/rev/6c79ec580eec

Comment 28

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/6c79ec580eec

Comment 29

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

Backed out in an attempt to find the cause of bug 1088669
https://hg.mozilla.org/integration/fx-team/rev/fce4cd8717a4

Comment 30

[Chris Kitching [:ckitching]]

Should this (and Bug 961600) be relanded now their removal has not solved the problem?

Comment 31

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

(In reply to Chris Kitching [:ckitching] from comment #30)
> Should this (and Bug 961600) be relanded now their removal has not solved
> the problem?

Let's get a few more runs, but yes, I think we'll reland sometime on Monday.

Comment 32

[Mats Palmgren (inactive)]

FYI, see bug 1089940 comment 25.  I think this bug caused those warnings,
although they are fixed in that bug now AIUI.

Comment 33

[Chris Kitching [:ckitching]]

https://hg.mozilla.org/integration/fx-team/rev/aa136b053a76

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/aa136b053a76