From 51d9e20da8f4af56f0dc8ffe7f6786929c625a32 Mon Sep 17 00:00:00 2001
From: Ed Lee <edilee@mozilla.com>
Date: Fri, 4 Aug 2017 20:38:00 -0700
Subject: [PATCH] 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

--HG--
extra : rebase_source : c0d9b87ee6b8dd7f3b57f4c3a89fe9a2e324f1a9
---
 browser/base/content/newtab/page.js           | 13 +++--
 browser/base/content/newtab/sites.js          |  3 +-
 .../test/functional/mochitest/browser.ini     |  1 -
 .../thumbnails/BackgroundPageThumbs.jsm       | 48 +++++++++++--------
 4 files changed, 38 insertions(+), 27 deletions(-)

diff --git a/browser/base/content/newtab/page.js b/browser/base/content/newtab/page.js
index 8db377a1620b..88cff8fe06b1 100644
--- a/browser/base/content/newtab/page.js
+++ b/browser/base/content/newtab/page.js
@@ -19,8 +19,14 @@ var gPage = {
     // 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
@@ -189,9 +195,6 @@ var gPage = {
       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
diff --git a/browser/base/content/newtab/sites.js b/browser/base/content/newtab/sites.js
index ce371670c5a5..2f5da458d45e 100644
--- a/browser/base/content/newtab/sites.js
+++ b/browser/base/content/newtab/sites.js
@@ -168,7 +168,8 @@ Site.prototype = {
    */
   captureIfMissing: function Site_captureIfMissing() {
     if (!document.hidden && !this.link.imageURI) {
-      BackgroundPageThumbs.captureIfMissing(this.url);
+      const {unloadingPromise} = gPage;
+      BackgroundPageThumbs.captureIfMissing(this.url, {unloadingPromise});
     }
   },
 
diff --git a/browser/extensions/activity-stream/test/functional/mochitest/browser.ini b/browser/extensions/activity-stream/test/functional/mochitest/browser.ini
index f30e27fddb48..41eb117c783f 100644
--- a/browser/extensions/activity-stream/test/functional/mochitest/browser.ini
+++ b/browser/extensions/activity-stream/test/functional/mochitest/browser.ini
@@ -6,4 +6,3 @@ support-files =
 [browser_as_load_location.js]
 [browser_as_render.js]
 [browser_getScreenshots.js]
-skip-if=true # issue 2851
diff --git a/toolkit/components/thumbnails/BackgroundPageThumbs.jsm b/toolkit/components/thumbnails/BackgroundPageThumbs.jsm
index b85b5b243c95..185c00d5a6fa 100644
--- a/toolkit/components/thumbnails/BackgroundPageThumbs.jsm
+++ b/toolkit/components/thumbnails/BackgroundPageThumbs.jsm
@@ -91,6 +91,8 @@ const BackgroundPageThumbs = {
    * @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 = {}) {
@@ -105,28 +107,34 @@ const BackgroundPageThumbs = {
       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);