Bug 1279650 - kill off useless warning for timed out favicon requests that have already been cancelled when the window closes, r=mikedeboer
authorGijs Kruitbosch <gijskruitbosch@gmail.com>
Wed, 17 Aug 2016 18:34:50 +0100
changeset 310241 fbc71dcb173ac4f83d24653e692b2b2599205d48
parent 310240 169273f710d8519ff36bb740935759d84d3df3ee
child 310242 eaed9150655ae93349c937ff38cc321edbff0e07
push id20348
push userryanvm@gmail.com
push dateFri, 19 Aug 2016 13:56:01 +0000
treeherderfx-team@8dfc2fdb7ae3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmikedeboer
bugs1279650
milestone51.0a1
Bug 1279650 - kill off useless warning for timed out favicon requests that have already been cancelled when the window closes, r=mikedeboer As it is, when favicon requests time out, the timeout handler doesn't remove the loadData and request from gFaviconLoadDataMap after cancelling the request, which means that when the tab or the parent window is closed we try and cancel the same thing again, which leads to errors in the console. This patch addresses that issue by making timeouts also remove the loadData from gFaviconLoadDataMap. MozReview-Commit-ID: FCeRFXjeXdJ
browser/components/places/PlacesUIUtils.jsm
--- a/browser/components/places/PlacesUIUtils.jsm
+++ b/browser/components/places/PlacesUIUtils.jsm
@@ -140,37 +140,60 @@ let InternalFaviconLoader = {
       for (let loadData of loadDataForWindow) {
         this._cancelRequest(loadData, "the chrome window went away");
       }
     }
     gFaviconLoadDataMap.delete(win);
   },
 
   /**
+   * Remove a particular favicon load's loading data from our map tracking
+   * load data per chrome window.
+   *
+   * @param win
+   *        the chrome window in which we should look for this load
+   * @param filterData ({innerWindowID, uri, callback})
+   *        the data we should use to find this particular load to remove.
+   *
+   * @return the loadData object we removed, or null if we didn't find any.
+   */
+  _removeLoadDataFromWindowMap(win, {innerWindowID, uri, callback}) {
+    let loadDataForWindow = gFaviconLoadDataMap.get(win);
+    if (loadDataForWindow) {
+      let itemIndex = loadDataForWindow.findIndex(loadData => {
+        return loadData.innerWindowID == innerWindowID &&
+               loadData.uri.equals(uri) &&
+               loadData.callback.request == callback.request;
+      });
+      if (itemIndex != -1) {
+        let loadData = loadDataForWindow[itemIndex];
+        loadDataForWindow.splice(itemIndex, 1);
+        return loadData;
+      }
+    }
+    return null;
+  },
+
+  /**
    * Create a function to use as a nsIFaviconDataCallback, so we can remove cancelling
    * information when the request succeeds. Note that right now there are some edge-cases,
    * such as about: URIs with chrome:// favicons where the success callback is not invoked.
    * This is OK: we will 'cancel' the request after the timeout (or when the window goes
    * away) but that will be a no-op in such cases.
    */
   _makeCompletionCallback(win, id) {
     return {
       onComplete(uri) {
-        let loadDataForWindow = gFaviconLoadDataMap.get(win);
-        if (loadDataForWindow) {
-          let itemIndex = loadDataForWindow.findIndex(loadData => {
-            return loadData.innerWindowID == id &&
-                   loadData.uri.equals(uri) &&
-                   loadData.callback.request == this.request;
-          });
-          if (itemIndex != -1) {
-            let loadData = loadDataForWindow[itemIndex];
-            clearTimeout(loadData.timerID);
-            loadDataForWindow.splice(itemIndex, 1);
-          }
+        let loadData = InternalFaviconLoader._removeLoadDataFromWindowMap(win, {
+          uri,
+          innerWindowID: id,
+          callback: this,
+        });
+        if (loadData) {
+          clearTimeout(loadData.timerID);
         }
         delete this.request;
       },
     };
   },
 
   ensureInitialized() {
     if (this._initialized) {
@@ -219,16 +242,17 @@ let InternalFaviconLoader = {
       // as null) if the icon is the same as the page (e.g. for images) or if it is
       // the favicon for an error page. In this case, we do not need to do anything else.
       return;
     }
     callback.request = request;
     let loadData = {innerWindowID, uri, callback};
     loadData.timerID = setTimeout(() => {
       this._cancelRequest(loadData, "it timed out");
+      this._removeLoadDataFromWindowMap(win, loadData);
     }, FAVICON_REQUEST_TIMEOUT);
     let loadDataForWindow = gFaviconLoadDataMap.get(win);
     loadDataForWindow.push(loadData);
   },
 };
 
 this.PlacesUIUtils = {
   ORGANIZER_LEFTPANE_VERSION: 7,