From a4961c55d5ae8fb66da38d9f29124ae11fa087b5 Mon Sep 17 00:00:00 2001 From: Chris Kitching Date: Wed, 22 Oct 2014 14:55:53 +0100 Subject: [PATCH] Bug 914027: Do not attempt to decode Favicons of unsupported types. Fallback on Favicon failure. r=margaret --- mobile/android/base/BrowserApp.java | 49 +--------- mobile/android/base/Tab.java | 94 ++++++++++++++----- mobile/android/base/Tabs.java | 51 ++++------ mobile/android/base/favicons/Favicons.java | 77 +++++++++++++++ .../android/base/favicons/RemoteFavicon.java | 89 ++++++++++++++++++ mobile/android/base/moz.build | 1 + .../base/toolbar/ToolbarDisplayLayout.java | 3 +- mobile/android/chrome/content/browser.js | 3 +- 8 files changed, 257 insertions(+), 110 deletions(-) create mode 100644 mobile/android/base/favicons/RemoteFavicon.java diff --git a/mobile/android/base/BrowserApp.java b/mobile/android/base/BrowserApp.java index 64a81b95f84..d39918818e9 100644 --- a/mobile/android/base/BrowserApp.java +++ b/mobile/android/base/BrowserApp.java @@ -267,9 +267,6 @@ public class BrowserApp extends GeckoApp Log.d(LOGTAG, "BrowserApp.onTabChanged: " + tab.getId() + ": " + msg); switch(msg) { case LOCATION_CHANGE: - if (Tabs.getInstance().isSelectedTab(tab)) { - maybeCancelFaviconLoad(tab); - } // fall through case SELECTED: if (Tabs.getInstance().isSelectedTab(tab)) { @@ -293,16 +290,7 @@ public class BrowserApp extends GeckoApp } break; case PAGE_SHOW: - loadFavicon(tab); - break; - case LINK_FAVICON: - // If tab is not loading and the favicon is updated, we - // want to load the image straight away. If tab is still - // loading, we only load the favicon once the page's content - // is fully loaded. - if (tab.getState() != Tab.STATE_LOADING) { - loadFavicon(tab); - } + tab.loadFavicon(); break; case BOOKMARK_ADDED: showBookmarkAddedToast(); @@ -1815,41 +1803,6 @@ public class BrowserApp extends GeckoApp && mHomePagerContainer != null && mHomePagerContainer.getVisibility() == View.VISIBLE); } - /* Favicon stuff. */ - private static OnFaviconLoadedListener sFaviconLoadedListener = new OnFaviconLoadedListener() { - @Override - public void onFaviconLoaded(String pageUrl, String faviconURL, Bitmap favicon) { - // If we failed to load a favicon, we use the default favicon instead. - Tabs.getInstance() - .updateFaviconForURL(pageUrl, - (favicon == null) ? Favicons.defaultFavicon : favicon); - } - }; - - private void loadFavicon(final Tab tab) { - maybeCancelFaviconLoad(tab); - - final int tabFaviconSize = getResources().getDimensionPixelSize(R.dimen.browser_toolbar_favicon_size); - - int flags = (tab.isPrivate() || tab.getErrorType() != Tab.ErrorType.NONE) ? 0 : LoadFaviconTask.FLAG_PERSIST; - int id = Favicons.getSizedFavicon(getContext(), tab.getURL(), tab.getFaviconURL(), tabFaviconSize, flags, sFaviconLoadedListener); - - tab.setFaviconLoadId(id); - } - - private void maybeCancelFaviconLoad(Tab tab) { - int faviconLoadId = tab.getFaviconLoadId(); - - if (Favicons.NOT_LOADING == faviconLoadId) { - return; - } - - // Cancel load task and reset favicon load state if it wasn't already - // in NOT_LOADING state. - Favicons.cancelFaviconLoad(faviconLoadId); - tab.setFaviconLoadId(Favicons.NOT_LOADING); - } - /** * Enters editing mode with the current tab's URL. There might be no * tabs loaded by the time the user enters editing mode e.g. just after diff --git a/mobile/android/base/Tab.java b/mobile/android/base/Tab.java index ca2fde80ed5..1eeac241c50 100644 --- a/mobile/android/base/Tab.java +++ b/mobile/android/base/Tab.java @@ -9,6 +9,7 @@ import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; import java.util.Map; +import java.util.TreeSet; import java.util.regex.Matcher; import java.util.regex.Pattern; @@ -17,6 +18,10 @@ import org.json.JSONObject; import org.mozilla.gecko.db.BrowserContract.Bookmarks; import org.mozilla.gecko.db.BrowserDB; import org.mozilla.gecko.db.URLMetadata; +import org.mozilla.gecko.favicons.Favicons; +import org.mozilla.gecko.favicons.LoadFaviconTask; +import org.mozilla.gecko.favicons.OnFaviconLoadedListener; +import org.mozilla.gecko.favicons.RemoteFavicon; import org.mozilla.gecko.gfx.Layer; import org.mozilla.gecko.util.ThreadUtils; @@ -42,7 +47,9 @@ public class Tab { private String mTitle; private Bitmap mFavicon; private String mFaviconUrl; - private int mFaviconSize; + + // The set of all available Favicons for this tab, sorted by attractiveness. + final TreeSet mAvailableFavicons = new TreeSet<>(); private boolean mHasFeeds; private boolean mHasOpenSearch; private final SiteIdentity mSiteIdentity; @@ -365,46 +372,81 @@ public class Tab { return mHasTouchListeners; } - public void setFaviconLoadId(int faviconLoadId) { - mFaviconLoadId = faviconLoadId; - } + public synchronized void addFavicon(String faviconURL, int faviconSize, String mimeType) { + RemoteFavicon favicon = new RemoteFavicon(faviconURL, faviconSize, mimeType); - public int getFaviconLoadId() { - return mFaviconLoadId; - } - - /** - * Returns true if the favicon changed. - */ - public boolean updateFavicon(Bitmap favicon) { - if (mFavicon == favicon) { - return false; + // Add this Favicon to the set of available Favicons. + synchronized (mAvailableFavicons) { + mAvailableFavicons.add(favicon); } - mFavicon = favicon; - return true; } - public synchronized void updateFaviconURL(String faviconUrl, int size) { - // If we already have an "any" sized icon, don't update the icon. - if (mFaviconSize == -1) - return; + public void loadFavicon() { + // If we have a Favicon explicitly set, load it. + if (!mAvailableFavicons.isEmpty()) { + RemoteFavicon newFavicon = mAvailableFavicons.first(); - // Only update the favicon if it's bigger than the current favicon. - // We use -1 to represent icons with sizes="any". - if (size == -1 || size >= mFaviconSize) { - mFaviconUrl = faviconUrl; - mFaviconSize = size; + // If the new Favicon is different, cancel the old load. Else, abort. + if (newFavicon.faviconUrl.equals(mFaviconUrl)) { + return; + } + + Favicons.cancelFaviconLoad(mFaviconLoadId); + mFaviconUrl = newFavicon.faviconUrl; + } else { + // Otherwise, fallback to the default Favicon. + mFaviconUrl = null; } + + int flags = (isPrivate() || mErrorType != ErrorType.NONE) ? 0 : LoadFaviconTask.FLAG_PERSIST; + mFaviconLoadId = Favicons.getSizedFavicon(mAppContext, mUrl, mFaviconUrl, Favicons.browserToolbarFaviconSize, flags, + new OnFaviconLoadedListener() { + @Override + public void onFaviconLoaded(String pageUrl, String faviconURL, Bitmap favicon) { + // The tab might be pointing to another URL by the time the + // favicon is finally loaded, in which case we simply ignore it. + if (!pageUrl.equals(mUrl)) { + return; + } + + // That one failed. Try the next one. + if (favicon == null) { + // If what we just tried to load originated from the set of declared icons.. + if (!mAvailableFavicons.isEmpty()) { + // Discard it. + mAvailableFavicons.remove(mAvailableFavicons.first()); + + // Load the next best, if we have one. If not, it'll fall back to the + // default Favicon URL, before giving up. + loadFavicon(); + + return; + } + + // Total failure: display the default favicon. + favicon = Favicons.defaultFavicon; + } + + mFavicon = favicon; + mFaviconLoadId = Favicons.NOT_LOADING; + Tabs.getInstance().notifyListeners(Tab.this, Tabs.TabEvents.FAVICON); + } + } + ); } public synchronized void clearFavicon() { + // Cancel any ongoing favicon load (if we never finished downloading the old favicon before + // we changed page). + Favicons.cancelFaviconLoad(mFaviconLoadId); + // Keep the favicon unchanged while entering reader mode if (mEnteringReaderMode) return; mFavicon = null; mFaviconUrl = null; - mFaviconSize = 0; + mAvailableFavicons.clear(); } public void setHasFeeds(boolean hasFeeds) { diff --git a/mobile/android/base/Tabs.java b/mobile/android/base/Tabs.java index 6476eed4b7c..1b059c5a973 100644 --- a/mobile/android/base/Tabs.java +++ b/mobile/android/base/Tabs.java @@ -11,6 +11,7 @@ import java.util.List; import java.util.concurrent.CopyOnWriteArrayList; import java.util.concurrent.atomic.AtomicInteger; +import android.graphics.Bitmap; import org.json.JSONException; import org.json.JSONObject; @@ -29,7 +30,6 @@ import android.accounts.OnAccountsUpdateListener; import android.content.ContentResolver; import android.content.Context; import android.database.ContentObserver; -import android.graphics.Bitmap; import android.graphics.Color; import android.net.Uri; import android.os.Handler; @@ -506,8 +506,20 @@ public class Tabs implements GeckoEventListener { } 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); + // Don't bother if the type isn't one we can decode. + if (!Favicons.canDecodeType(message.getString("mime"))) { + return; + } + + // Add the favicon to the set of available icons for this tab. + tab.addFavicon(message.getString("href"), message.getInt("size"), message.getString("mime")); + + // Load the favicon. If the tab is still loading, we actually do the load once the + // page has loaded, in an attempt to prevent the favicon load from having a + // detrimental effect on page load time. + if (tab.getState() != Tab.STATE_LOADING) { + tab.loadFavicon(); + } } else if (event.equals("Link:Feed")) { tab.setHasFeeds(true); notifyListeners(tab, TabEvents.LINK_FEED); @@ -538,24 +550,6 @@ public class Tabs implements GeckoEventListener { } } - /** - * Set the favicon for any tabs loaded with this page URL. - */ - public void updateFaviconForURL(String pageURL, Bitmap favicon) { - // The tab might be pointing to another URL by the time the - // favicon is finally loaded, in which case we won't find the tab. - // See also: Bug 920331. - for (Tab tab : mOrder) { - String tabURL = tab.getURL(); - if (pageURL.equals(tabURL)) { - tab.setFaviconLoadId(Favicons.NOT_LOADING); - if (tab.updateFavicon(favicon)) { - notifyListeners(tab, TabEvents.FAVICON); - } - } - } - } - public void refreshThumbnails() { final ThumbnailHelper helper = ThumbnailHelper.getInstance(); ThreadUtils.postToBackgroundThread(new Runnable() { @@ -598,7 +592,6 @@ public class Tabs implements GeckoEventListener { LOCATION_CHANGE, MENU_UPDATED, PAGE_SHOW, - LINK_FAVICON, LINK_FEED, SECURITY_CHANGE, READER_ENABLED, @@ -842,7 +835,8 @@ public class Tabs implements GeckoEventListener { // TODO: surely we could just fetch *any* cached icon? if (AboutPages.isBuiltinIconPage(url)) { Log.d(LOGTAG, "Setting about: tab favicon inline."); - added.updateFavicon(getAboutPageFavicon(url)); + added.addFavicon(url, Favicons.browserToolbarFaviconSize, ""); + added.loadFavicon(); } return added; @@ -856,17 +850,6 @@ public class Tabs implements GeckoEventListener { return loadUrl(AboutPages.PRIVATEBROWSING, Tabs.LOADURL_NEW_TAB | Tabs.LOADURL_PRIVATE); } - /** - * These favicons are only used for the URL bar, so - * we fetch with that size. - * - * This method completes on the calling thread. - */ - private Bitmap getAboutPageFavicon(final String url) { - int faviconSize = Math.round(mAppContext.getResources().getDimension(R.dimen.browser_toolbar_favicon_size)); - return Favicons.getSizedFaviconForPageFromCache(url, faviconSize); - } - /** * Open the url as a new tab, and mark the selected tab as its "parent". * diff --git a/mobile/android/base/favicons/Favicons.java b/mobile/android/base/favicons/Favicons.java index 8a1cf8afc98..4f2abfebdb2 100644 --- a/mobile/android/base/favicons/Favicons.java +++ b/mobile/android/base/favicons/Favicons.java @@ -32,6 +32,7 @@ import java.net.URI; import java.net.URISyntaxException; import java.util.Arrays; import java.util.Collections; +import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.concurrent.ExecutorService; @@ -65,6 +66,9 @@ public class Favicons { // The density-adjusted maximum Favicon dimensions. public static int largestFaviconSize; + // The density-adjusted desired size for a browser-toolbar favicon. + public static int browserToolbarFaviconSize; + // Used to prevent multiple-initialisation. public static final AtomicBoolean isInitialized = new AtomicBoolean(false); @@ -77,6 +81,57 @@ public class Favicons { // doing so is not necessary. private static final NonEvictingLruCache pageURLMappings = new NonEvictingLruCache<>(NUM_PAGE_URL_MAPPINGS_TO_STORE); + // Mime types of things we are capable of decoding. + private static final HashSet sDecodableMimeTypes = new HashSet<>(); + + // Mime types of things we are both capable of decoding and are container formats (May contain + // multiple different sizes of image) + private static final HashSet sContainerMimeTypes = new HashSet<>(); + static { + // MIME types extracted from http://filext.com - ostensibly all in-use mime types for the + // corresponding formats. + // ICO + sContainerMimeTypes.add("image/vnd.microsoft.icon"); + sContainerMimeTypes.add("image/ico"); + sContainerMimeTypes.add("image/icon"); + sContainerMimeTypes.add("image/x-icon"); + sContainerMimeTypes.add("text/ico"); + sContainerMimeTypes.add("application/ico"); + + // Add supported container types to the set of supported types. + sDecodableMimeTypes.addAll(sContainerMimeTypes); + + // PNG + sDecodableMimeTypes.add("image/png"); + sDecodableMimeTypes.add("application/png"); + sDecodableMimeTypes.add("application/x-png"); + + // GIF + sDecodableMimeTypes.add("image/gif"); + + // JPEG + sDecodableMimeTypes.add("image/jpeg"); + sDecodableMimeTypes.add("image/jpg"); + sDecodableMimeTypes.add("image/pipeg"); + sDecodableMimeTypes.add("image/vnd.swiftview-jpeg"); + sDecodableMimeTypes.add("application/jpg"); + sDecodableMimeTypes.add("application/x-jpg"); + + // BMP + sDecodableMimeTypes.add("application/bmp"); + sDecodableMimeTypes.add("application/x-bmp"); + sDecodableMimeTypes.add("application/x-win-bitmap"); + sDecodableMimeTypes.add("image/bmp"); + sDecodableMimeTypes.add("image/x-bmp"); + sDecodableMimeTypes.add("image/x-bitmap"); + sDecodableMimeTypes.add("image/x-xbitmap"); + sDecodableMimeTypes.add("image/x-win-bitmap"); + sDecodableMimeTypes.add("image/x-windows-bitmap"); + sDecodableMimeTypes.add("image/x-ms-bitmap"); + sDecodableMimeTypes.add("image/x-ms-bmp"); + sDecodableMimeTypes.add("image/ms-bmp"); + } + public static String getFaviconURLForPageURLFromCache(String pageURL) { return pageURLMappings.get(pageURL); } @@ -415,6 +470,9 @@ public class Favicons { // Screen-density-adjusted upper limit on favicon size. Favicons larger than this are // downscaled to this size or discarded. largestFaviconSize = context.getResources().getDimensionPixelSize(R.dimen.favicon_largest_interesting_size); + + browserToolbarFaviconSize = context.getResources().getDimensionPixelSize(R.dimen.browser_toolbar_favicon_size); + faviconsCache = new FaviconCache(FAVICON_CACHE_SIZE_BYTES, largestFaviconSize); // Initialize page mappings for each of our special pages. @@ -482,6 +540,25 @@ public class Favicons { } } + /** + * Helper function to determine if we can decode a particular mime type. + * @param imgType 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 imgType) { + return "".equals(imgType) || sDecodableMimeTypes.contains(imgType); + } + + /** + * Helper function to determine if the provided mime type is that of a format that can contain + * multiple image types. At time of writing, the only such type is ICO. + * @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); + } + public static void removeLoadTask(int taskId) { synchronized(loadTasks) { loadTasks.delete(taskId); diff --git a/mobile/android/base/favicons/RemoteFavicon.java b/mobile/android/base/favicons/RemoteFavicon.java new file mode 100644 index 00000000000..978a2a5862c --- /dev/null +++ b/mobile/android/base/favicons/RemoteFavicon.java @@ -0,0 +1,89 @@ +/* This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ + +package org.mozilla.gecko.favicons; + +/** + * Class to represent a Favicon declared on a webpage which we may or may not have downloaded. + * Tab objects maintain a list of these and attempt to load them in descending order of quality + * until one works. This enables us to fallback if something fails trying to decode a Favicon. + */ +public class RemoteFavicon implements Comparable { + public static final int FAVICON_SIZE_ANY = -1; + + // URL of the Favicon referred to by this object. + public String faviconUrl; + + // The size of the Favicon, as given in the tag, if any. Zero if unspecified. -1 if "any". + public int declaredSize; + + // Mime type of the Favicon, as given in the link tag. + public String mimeType; + + public RemoteFavicon(String faviconURL, int givenSize, String mime) { + faviconUrl = faviconURL; + declaredSize = givenSize; + mimeType = mime; + } + + /** + * Determine equality of two RemoteFavicons. + * Two RemoteFavicons are considered equal if they have the same URL, size, and mime type. + * @param o The object to compare to this one. + * @return true if o is of type RemoteFavicon and is considered equal to this one, false otherwise. + */ + @Override + public boolean equals(Object o) { + if (!(o instanceof RemoteFavicon)) { + return false; + } + RemoteFavicon oCast = (RemoteFavicon) o; + + return oCast.faviconUrl.equals(faviconUrl) && + oCast.declaredSize == declaredSize && + oCast.mimeType.equals(mimeType); + } + + @Override + public int hashCode() { + return super.hashCode(); + } + + /** + * Establish ordering on Favicons. Lower value in the partial ordering is a "better" Favicon. + * @param obj 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. + */ + @Override + public int compareTo(RemoteFavicon obj) { + // Size "any" trumps everything else. + if (declaredSize == FAVICON_SIZE_ANY) { + if (obj.declaredSize == FAVICON_SIZE_ANY) { + return 0; + } + + return -1; + } + + if (obj.declaredSize == FAVICON_SIZE_ANY) { + return 1; + } + + // Unspecified sizes are "worse". + if (declaredSize > obj.declaredSize) { + return -1; + } + + if (declaredSize == obj.declaredSize) { + // If there's no other way to choose, we prefer container types. They *might* contain + // an image larger than the size given in the tag. + if (Favicons.isContainerType(mimeType)) { + return -1; + } + return 0; + } + return 1; + } +} diff --git a/mobile/android/base/moz.build b/mobile/android/base/moz.build index 702f37ad832..3784b54128f 100644 --- a/mobile/android/base/moz.build +++ b/mobile/android/base/moz.build @@ -177,6 +177,7 @@ gbjar.sources += [ 'favicons/Favicons.java', 'favicons/LoadFaviconTask.java', 'favicons/OnFaviconLoadedListener.java', + 'favicons/RemoteFavicon.java', 'FilePicker.java', 'FilePickerResultHandler.java', 'FindInPageBar.java', diff --git a/mobile/android/base/toolbar/ToolbarDisplayLayout.java b/mobile/android/base/toolbar/ToolbarDisplayLayout.java index c189e9d2ff7..f1e2286c408 100644 --- a/mobile/android/base/toolbar/ToolbarDisplayLayout.java +++ b/mobile/android/base/toolbar/ToolbarDisplayLayout.java @@ -19,6 +19,7 @@ import org.mozilla.gecko.SiteIdentity.SecurityMode; import org.mozilla.gecko.Tab; import org.mozilla.gecko.animation.PropertyAnimator; import org.mozilla.gecko.animation.ViewHelper; +import org.mozilla.gecko.favicons.Favicons; import org.mozilla.gecko.toolbar.BrowserToolbarTabletBase.ForwardButtonAnimation; import org.mozilla.gecko.util.StringUtils; import org.mozilla.gecko.widget.ThemedLinearLayout; @@ -178,7 +179,7 @@ public class ToolbarDisplayLayout extends ThemedLinearLayout if (Versions.feature16Plus) { mFavicon.setImportantForAccessibility(View.IMPORTANT_FOR_ACCESSIBILITY_NO); } - mFaviconSize = Math.round(res.getDimension(R.dimen.browser_toolbar_favicon_size)); + mFaviconSize = Math.round(Favicons.browserToolbarFaviconSize); } mSiteSecurityVisible = (mSiteSecurity.getVisibility() == View.VISIBLE); diff --git a/mobile/android/chrome/content/browser.js b/mobile/android/chrome/content/browser.js index 9af9cf6f5ce..21056a94a7c 100644 --- a/mobile/android/chrome/content/browser.js +++ b/mobile/android/chrome/content/browser.js @@ -3991,7 +3991,8 @@ Tab.prototype = { type: "Link:Favicon", tabID: this.id, href: resolveGeckoURI(target.href), - size: maxSize + size: maxSize, + mime: target.getAttribute("type") || "" }; Messaging.sendRequest(json); } else if (list.indexOf("[alternate]") != -1 && aEvent.type == "DOMLinkAdded") {