Bug 697987 - Remove race when downloading favicons [r=sriram]
authorKartikaya Gupta <kgupta@mozilla.com>
Tue, 01 Nov 2011 13:15:08 -0400
changeset 81706 9e9b953e65433f35dfbb8e808fc6f2be758df1c8
parent 81705 8724de90b59fdfb95d9d0ea7c850b24aa0c66023
child 81707 d5da3bb90a1ec9826e988977286ad13f27926aee
push id21573
push userblassey@mozilla.com
push dateTue, 06 Dec 2011 18:57:07 +0000
treeherdermozilla-central@0e397568c71e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssriram
bugs697987
milestone10.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 697987 - Remove race when downloading favicons [r=sriram] Multiple DownloadFaviconTasks could get queued and run simultaneously, making the tab's final favicon anybody's guess. Instead, this patch ensures that existing favicon downloaders for a particular tab are cancelled before new ones are queued, eliminating the race condition.
embedding/android/GeckoApp.java
embedding/android/Tab.java
--- a/embedding/android/GeckoApp.java
+++ b/embedding/android/GeckoApp.java
@@ -38,17 +38,16 @@
  *
  * ***** END LICENSE BLOCK ***** */
 
 package org.mozilla.gecko;
 
 import java.io.*;
 import java.util.*;
 import java.util.zip.*;
-import java.net.MalformedURLException;
 import java.net.URL;
 import java.nio.*;
 import java.nio.channels.FileChannel;
 import java.util.concurrent.*;
 import java.lang.reflect.*;
 
 import org.json.*;
 
@@ -900,17 +899,17 @@ abstract public class GeckoApp
 
     void handleContentLoaded(int tabId, String uri, String title) {
         final Tab tab = Tabs.getInstance().getTab(tabId);
         if (tab == null)
             return;
 
         tab.updateTitle(title);
         if (tab.getFavicon() == null)
-            downloadDefaultFavicon(tabId);
+            tab.downloadFavicon(null);
 
         mMainHandler.post(new Runnable() {
             public void run() {
                 if (Tabs.getInstance().isSelectedTab(tab))
                     mBrowserToolbar.setTitle(tab.getDisplayTitle());
                 onTabsChanged();
             }
         });
@@ -929,83 +928,28 @@ abstract public class GeckoApp
                     mBrowserToolbar.setTitle(tab.getDisplayTitle());
                 onTabsChanged();
             }
         });
     }
 
     void handleLinkAdded(final int tabId, String rel, final String href) {
         if (rel.indexOf("icon") != -1) {
-            new DownloadFaviconTask(tabId).execute(href);
-        }
-    }
-
-    void downloadDefaultFavicon(final int tabId) {
-        Tab tab = Tabs.getInstance().getSelectedTab();
-        if (tab == null)
-            return;
-
-        try {
-            URL url = new URL(tab.getURL());
-            String faviconUrl = url.getProtocol() + "://" + url.getAuthority() + "/favicon.ico";
-            new DownloadFaviconTask(tabId).execute(faviconUrl);
-        } catch (MalformedURLException e) {
-            // Optional so not a real error
+            Tab tab = Tabs.getInstance().getTab(tabId);
+            if (tab != null)
+                tab.downloadFavicon(href);
         }
     }
 
-    private class DownloadFaviconTask extends AsyncTask<String, Void, Drawable> {
-        private final int mTabId;
-
-        public DownloadFaviconTask(int tabId) {
-            mTabId = tabId;
-        }
-
-        protected Drawable doInBackground(String... args) {
-            Drawable image = null;
-            
-            try {
-                URL url = new URL(args[0]);
-
-                InputStream is = (InputStream) url.getContent();
-                image = Drawable.createFromStream(is, "src");
-            } catch (IOException e) {
-                Log.d(LOG_NAME, "Error loading favicon: " + e);
-            }
-
-            return image;
-        }
+    void faviconUpdated(Tab tab) {
+        if (Tabs.getInstance().isSelectedTab(tab))
+            mBrowserToolbar.setFavicon(tab.getFavicon());
+        onTabsChanged();
+    }
 
-        protected void onPostExecute(Drawable image) {
-            if (image != null) {
-                Tab tab = Tabs.getInstance().getTab(mTabId);
-                if (tab == null)
-                    return;
-
-                tab.updateFavicon(image);
-
-                mMainHandler.post(new Runnable() {
-                    public void run() {
-                        onTabsChanged();
-                    }
-                });
-
-                if (!Tabs.getInstance().isSelectedTab(tab))
-                    return;
-
-                final Drawable postImage = image;
-                mMainHandler.post(new Runnable() {
-                    public void run() {
-                        mBrowserToolbar.setFavicon(postImage);
-                    }
-                });
-            }
-        }
-    }
-    
     void addPluginView(final View view,
                        final double x, final double y,
                        final double w, final double h) {
         mMainHandler.post(new Runnable() { 
             public void run() {
                 RelativeLayout.LayoutParams lp = new RelativeLayout.LayoutParams((int) w, (int) h);
                 lp.leftMargin = (int) x;
                 lp.topMargin = (int) y;
--- a/embedding/android/Tab.java
+++ b/embedding/android/Tab.java
@@ -44,31 +44,36 @@ import android.database.sqlite.SQLiteDat
 import android.graphics.drawable.Drawable;
 import android.os.AsyncTask;
 import android.provider.Browser;
 import android.util.Log;
 
 import org.json.JSONException;
 import org.json.JSONObject;
 
+import java.io.InputStream;
+import java.io.IOException;
+import java.net.MalformedURLException;
+import java.net.URL;
 import java.util.ArrayList;
 import java.util.List;
 
 public class Tab {
 
     private static final String LOG_NAME = "Tab";
     private int mId;
     private String mUrl;
     private String mTitle;
     private Drawable mFavicon;
     private Drawable mThumbnail;
     private List<HistoryEntry> mHistory;
     private int mHistoryIndex;
     private boolean mLoading;
     private boolean mBookmark;
+    private DownloadFaviconTask mFaviconDownloader;
 
     static class HistoryEntry {
         public final String mUri;
         public String mTitle;
 
         public HistoryEntry(String uri, String title) {
             mUri = uri;
             mTitle = title;
@@ -247,16 +252,65 @@ public class Tab {
             }
             mHistoryIndex = index;
         } else if (event.equals("Purge")) {
             mHistory.clear();
             mHistoryIndex = -1;
         }
     }
 
+    void downloadFavicon(String url) {
+        if (url == null) {
+            try {
+                URL urlObj = new URL(mUrl);
+                url = urlObj.getProtocol() + "://" + urlObj.getAuthority() + "/favicon.ico";
+            } catch (MalformedURLException e) {
+                // Optional so not a real error
+                return;
+            }
+        }
+
+        try {
+            URL urlObj = new URL(url);
+            // note that the above line may throw a MalformedURLException,
+            // in which case we abort and don't cancel the old download task.
+            if (mFaviconDownloader != null) {
+                mFaviconDownloader.cancel(false);
+                Log.d(LOG_NAME, "Cancelled old favicon downloader");
+            }
+
+            mFaviconDownloader = new DownloadFaviconTask();
+            mFaviconDownloader.execute(urlObj);
+        } catch (MalformedURLException e) {
+        }
+    }
+
+    private class DownloadFaviconTask extends AsyncTask<URL, Void, Drawable> {
+        protected Drawable doInBackground(URL... args) {
+            Drawable image = null;
+            try {
+                URL url = args[0];
+                InputStream is = (InputStream) url.getContent();
+                image = Drawable.createFromStream(is, "src");
+            } catch (IOException e) {
+                Log.d(LOG_NAME, "Error loading favicon: " + e);
+            }
+
+            return image;
+        }
+
+        protected void onPostExecute(Drawable image) {
+            if (image == null)
+                return;
+
+            updateFavicon(image);
+            GeckoApp.mAppContext.faviconUpdated(Tab.this);
+        }
+    }
+
     private class CheckBookmarkTask extends AsyncTask<Void, Void, Boolean> {
         @Override
         protected Boolean doInBackground(Void... unused) {
             ContentResolver resolver = Tabs.getInstance().getContentResolver();
             Cursor cursor = resolver.query(Browser.BOOKMARKS_URI,
                                            null,
                                            Browser.BookmarkColumns.URL + " = ? and " + Browser.BookmarkColumns.BOOKMARK + " = ?",
                                            new String[] { getURL(), "1" },