Bug 1279208, r=mak, a=al
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Thu, 09 Jun 2016 13:25:07 +0100
changeset 339617 4ff1d42b56b92d23197d592f8419f314e3bf770d
parent 339616 541035fe27cb71fd6fdcd273c5190edd688f0731
child 339618 41ac93b90438d87642108e1c181196bdeb327703
push id6249
push userjlund@mozilla.com
push dateMon, 01 Aug 2016 13:59:36 +0000
treeherdermozilla-beta@bad9d4f5bf7e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak, al
bugs1279208
milestone49.0a2
Bug 1279208, r=mak, a=al MozReview-Commit-ID: KOuTjwn9MSx
browser/components/places/PlacesUIUtils.jsm
toolkit/components/places/FaviconHelpers.cpp
--- a/browser/components/places/PlacesUIUtils.jsm
+++ b/browser/components/places/PlacesUIUtils.jsm
@@ -86,17 +86,17 @@ function IsLivemark(aItemId) {
 let InternalFaviconLoader = {
   /**
    * This gets called for every inner window that is destroyed.
    * In the parent process, we process the destruction ourselves. In the child process,
    * we notify the parent which will then process it based on that message.
    */
   observe(subject, topic, data) {
     let innerWindowID = subject.QueryInterface(Ci.nsISupportsPRUint64).data;
-    this.onInnerDestroyed(innerWindowID);
+    this.removeRequestsForInner(innerWindowID);
   },
 
   /**
    * Actually cancel the request, and clear the timeout for cancelling it.
    */
   _cancelRequest({uri, innerWindowID, timerID, callback}, reason) {
     // Break cycle
     let request = callback.request;
@@ -108,22 +108,22 @@ let InternalFaviconLoader = {
     } catch (ex) {
       Cu.reportError("When cancelling a request for " + uri.spec + " because " + reason + ", it was already canceled!");
     }
   },
 
   /**
    * Called for every inner that gets destroyed, only in the parent process.
    */
-  onInnerDestroyed(innerID) {
+  removeRequestsForInner(innerID) {
     for (let [window, loadDataForWindow] of gFaviconLoadDataMap) {
       let newLoadDataForWindow = loadDataForWindow.filter(loadData => {
         let innerWasDestroyed = loadData.innerWindowID == innerID;
         if (innerWasDestroyed) {
-          this._cancelRequest(loadData, "the inner window was destroyed");
+          this._cancelRequest(loadData, "the inner window was destroyed or a new favicon was loaded for it");
         }
         // Keep the items whose inner is still alive.
         return !innerWasDestroyed;
       });
       // Map iteration with for...of is safe against modification, so
       // now just replace the old value:
       gFaviconLoadDataMap.set(window, newLoadDataForWindow);
     }
@@ -175,36 +175,39 @@ let InternalFaviconLoader = {
   ensureInitialized() {
     if (this._initialized) {
       return;
     }
     this._initialized = true;
 
     Services.obs.addObserver(this, "inner-window-destroyed", false);
     Services.ppmm.addMessageListener("Toolkit:inner-window-destroyed", msg => {
-      this.onInnerDestroyed(msg.data);
+      this.removeRequestsForInner(msg.data);
     });
   },
 
   loadFavicon(browser, principal, uri) {
     this.ensureInitialized();
     let win = browser.ownerDocument.defaultView;
     if (!gFaviconLoadDataMap.has(win)) {
       gFaviconLoadDataMap.set(win, []);
       let unloadHandler = event => {
         let doc = event.target;
-        let eventWin = doc.defaultview;
-        if (win == win.top && doc.documentURI != "about:blank") {
+        let eventWin = doc.defaultView;
+        if (eventWin == win) {
           win.removeEventListener("unload", unloadHandler);
           this.onUnload(win);
         }
       };
       win.addEventListener("unload", unloadHandler, true);
     }
 
+    // Immediately cancel any earlier requests
+    this.removeRequestsForInner(innerWindowID);
+
     // First we do the actual setAndFetch call:
     let {innerWindowID, currentURI} = browser;
     let loadType = PrivateBrowsingUtils.isWindowPrivate(win)
       ? PlacesUtils.favicons.FAVICON_LOAD_PRIVATE
       : PlacesUtils.favicons.FAVICON_LOAD_NON_PRIVATE;
     let callback = this._makeCompletionCallback(win, innerWindowID);
     let request = PlacesUtils.favicons.setAndFetchFaviconForPage(currentURI, uri, false,
                                                                  loadType, callback, principal);
--- a/toolkit/components/places/FaviconHelpers.cpp
+++ b/toolkit/components/places/FaviconHelpers.cpp
@@ -436,17 +436,21 @@ AsyncFetchAndSetIconForPage::FetchFromNe
     NS_ENSURE_SUCCESS(rv, rv);
   }
 
   nsCOMPtr<nsISupportsPriority> priorityChannel = do_QueryInterface(channel);
   if (priorityChannel) {
     priorityChannel->AdjustPriority(nsISupportsPriority::PRIORITY_LOWEST);
   }
 
-  return channel->AsyncOpen2(this);
+  rv = channel->AsyncOpen2(this);
+  if (NS_SUCCEEDED(rv)) {
+    mRequest = channel;
+  }
+  return rv;
 }
 
 NS_IMETHODIMP
 AsyncFetchAndSetIconForPage::Cancel()
 {
   MOZ_ASSERT(NS_IsMainThread());
   if (mCanceled) {
     return NS_ERROR_UNEXPECTED;
@@ -457,16 +461,19 @@ AsyncFetchAndSetIconForPage::Cancel()
   }
   return NS_OK;
 }
 
 NS_IMETHODIMP
 AsyncFetchAndSetIconForPage::OnStartRequest(nsIRequest* aRequest,
                                             nsISupports* aContext)
 {
+  // mRequest should already be set from ::FetchFromNetwork, but in the case of
+  // a redirect we might get a new request, and we should make sure we keep a
+  // reference to the most current request.
   mRequest = aRequest;
   if (mCanceled) {
     mRequest->Cancel(NS_BINDING_ABORTED);
   }
   return NS_OK;
 }
 
 NS_IMETHODIMP