Bug 1387682 - Screenshots missing when thumbnailer Promise is garbage collected. r=ursula draft
authorEd Lee <edilee@mozilla.com>
Fri, 04 Aug 2017 20:38:00 -0700
changeset 642614 a035eae77600f6749e7b83be9fbcc8d27811616f
parent 641104 933a04a91ce3bd44b230937083a835cb60637084
child 725061 2cfe9649b18f56baa3a6f712772af699429e0f98
push id72826
push userbmo:edilee@mozilla.com
push dateTue, 08 Aug 2017 15:53:43 +0000
reviewersursula
bugs1387682
milestone57.0a1
Bug 1387682 - Screenshots missing when thumbnailer Promise is garbage collected. r=ursula Properly clean up on unload from newTab.js to release strong references in the background thumbnailer. MozReview-Commit-ID: IJNSYjwKUW3
browser/base/content/newtab/page.js
browser/base/content/newtab/sites.js
browser/extensions/activity-stream/test/functional/mochitest/browser.ini
toolkit/components/thumbnails/BackgroundPageThumbs.jsm
--- a/browser/base/content/newtab/page.js
+++ b/browser/base/content/newtab/page.js
@@ -14,18 +14,24 @@ const SCHEDULE_UPDATE_TIMEOUT_MS = 1000;
 var gPage = {
   /**
    * Initializes the page.
    */
   init: function Page_init() {
     // Add ourselves to the list of pages to receive notifications.
     gAllPages.register(this);
 
-    // Listen for 'unload' to unregister this page.
-    addEventListener("unload", this, false);
+    // Listen for 'unload' to unregister this page. Save a promise that can be
+    // passed to others to know when to clean up, e.g., background thumbnails.
+    this.unloadingPromise = new Promise(resolve => {
+      addEventListener("unload", () => {
+        resolve();
+        this._handleUnloadEvent();
+      });
+    });
 
     // XXX bug 991111 - Not all click events are correctly triggered when
     // listening from xhtml nodes -- in particular middle clicks on sites, so
     // listen from the xul window and filter then delegate
     addEventListener("click", this, false);
 
     // Check if the new tab feature is enabled.
     let enabled = gAllPages.enabled;
@@ -184,19 +190,16 @@ var gPage = {
   /**
    * Handles all page events.
    */
   handleEvent: function Page_handleEvent(aEvent) {
     switch (aEvent.type) {
       case "load":
         this.onPageVisibleAndLoaded();
         break;
-      case "unload":
-        this._handleUnloadEvent();
-        break;
       case "click":
         let {button, target} = aEvent;
         // Go up ancestors until we find a Site or not
         while (target) {
           if (target.hasOwnProperty("_newtabSite")) {
             target._newtabSite.onClick(aEvent);
             break;
           }
--- a/browser/base/content/newtab/sites.js
+++ b/browser/base/content/newtab/sites.js
@@ -163,17 +163,18 @@ Site.prototype = {
   },
 
   /**
    * Captures the site's thumbnail in the background, but only if there's no
    * existing thumbnail and the page allows background captures.
    */
   captureIfMissing: function Site_captureIfMissing() {
     if (!document.hidden && !this.link.imageURI) {
-      BackgroundPageThumbs.captureIfMissing(this.url);
+      const {unloadingPromise} = gPage;
+      BackgroundPageThumbs.captureIfMissing(this.url, {unloadingPromise});
     }
   },
 
   /**
    * Refreshes the thumbnail for the site.
    */
   refreshThumbnail: function Site_refreshThumbnail() {
     // Only enhance tiles if that feature is turned on
--- a/browser/extensions/activity-stream/test/functional/mochitest/browser.ini
+++ b/browser/extensions/activity-stream/test/functional/mochitest/browser.ini
@@ -1,9 +1,8 @@
 [DEFAULT]
 support-files =
   blue_page.html
   head.js
 
 [browser_as_load_location.js]
 [browser_as_render.js]
 [browser_getScreenshots.js]
-skip-if=true # issue 2851
--- a/toolkit/components/thumbnails/BackgroundPageThumbs.jsm
+++ b/toolkit/components/thumbnails/BackgroundPageThumbs.jsm
@@ -86,52 +86,60 @@ const BackgroundPageThumbs = {
 
   /**
    * Asynchronously captures a thumbnail of the given URL if one does not
    * already exist.  Otherwise does nothing.
    *
    * @param url      The URL to capture.
    * @param options  An optional object that configures the capture.  See
    *                 capture() for description.
+   *   unloadingPromise This option is resolved when the calling context is
+   *                    unloading, so things can be cleaned up to avoid leak.
    * @return {Promise} A Promise that resolves when this task completes
    */
   async captureIfMissing(url, options = {}) {
     // The fileExistsForURL call is an optimization, potentially but unlikely
     // incorrect, and no big deal when it is.  After the capture is done, we
     // atomically test whether the file exists before writing it.
     let exists = await PageThumbsStorage.fileExistsForURL(url);
     if (exists) {
       if (options.onDone) {
         options.onDone(url);
       }
       return url;
     }
     let thumbPromise = new Promise((resolve, reject) => {
-      let observer = {
-        observe(subject, topic, data) { // jshint ignore:line
-          if (data === url) {
-            switch (topic) {
-              case "page-thumbnail:create":
-                resolve();
-                break;
-              case "page-thumbnail:error":
-                reject(new Error("page-thumbnail:error"));
-                break;
-            }
-            Services.obs.removeObserver(observer, "page-thumbnail:create");
-            Services.obs.removeObserver(observer, "page-thumbnail:error");
+      let observe = (subject, topic, data) => {
+        if (data === url) {
+          switch (topic) {
+            case "page-thumbnail:create":
+              resolve();
+              break;
+            case "page-thumbnail:error":
+              reject(new Error("page-thumbnail:error"));
+              break;
           }
-        },
-        QueryInterface: XPCOMUtils.generateQI([Ci.nsIObserver,
-                                               Ci.nsISupportsWeakReference])
+          cleanup();
+        }
       };
-      // Use weak references to avoid leaking in tests where the promise we
-      // return is GC'ed before it has resolved.
-      Services.obs.addObserver(observer, "page-thumbnail:create", true);
-      Services.obs.addObserver(observer, "page-thumbnail:error", true);
+      Services.obs.addObserver(observe, "page-thumbnail:create");
+      Services.obs.addObserver(observe, "page-thumbnail:error");
+
+      // Make sure to clean up to avoid leaks by removing observers when
+      // observed or when our caller is unloading
+      function cleanup() {
+        if (observe) {
+          Services.obs.removeObserver(observe, "page-thumbnail:create");
+          Services.obs.removeObserver(observe, "page-thumbnail:error");
+          observe = null;
+        }
+      }
+      if (options.unloadingPromise) {
+        options.unloadingPromise.then(cleanup);
+      }
     });
     try {
       this.capture(url, options);
       await thumbPromise;
     } catch (err) {
       if (options.onDone) {
         options.onDone(url);
       }