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
--- 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);
}