Bug 1248846: [webext] Don't call callbacks after a context is destroyed. r=aswan
authorKris Maglione <maglione.k@gmail.com>
Mon, 04 Apr 2016 14:14:35 -0700
changeset 352242 2a95760bd9cca8d1912a14158380ce1f0d84766d
parent 352241 29782c3286ca0d496b16cd3a7a0edd0741641d99
child 352243 472ef14f6a4e2c01712bd025ef65a854ade92d22
child 352244 ab0585b6880580f5ec26cc986cfc657d0ffabd04
child 352333 42a0a7f409d1b9a40770e36a66af6ccf67539cc4
child 352800 58e7d5d26bee68002d53ce4f617a2f6d404e0768
push id15658
push usermaglione.k@gmail.com
push dateSat, 16 Apr 2016 02:36:08 +0000
reviewersaswan
bugs1248846
milestone48.0a1
Bug 1248846: [webext] Don't call callbacks after a context is destroyed. r=aswan MozReview-Commit-ID: 5O2YKGONODH
browser/components/extensions/ext-utils.js
toolkit/components/extensions/Extension.jsm
toolkit/components/extensions/ExtensionUtils.jsm
--- a/browser/components/extensions/ext-utils.js
+++ b/browser/components/extensions/ext-utils.js
@@ -207,22 +207,25 @@ class BasePopup {
 
   handleEvent(event) {
     switch (event.type) {
       case this.DESTROY_EVENT:
         this.destroy();
         break;
 
       case "DOMWindowCreated":
-        let winUtils = this.browser.contentWindow
-            .QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
-        for (let stylesheet of global.stylesheets) {
-          winUtils.addSheet(stylesheet, winUtils.AGENT_SHEET);
+        if (event.target === this.browser.contentDocument) {
+          let winUtils = this.browser.contentWindow
+              .QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
+          for (let stylesheet of global.stylesheets) {
+            winUtils.addSheet(stylesheet, winUtils.AGENT_SHEET);
+          }
         }
         break;
+
       case "DOMWindowClose":
         if (event.target === this.browser.contentWindow) {
           event.preventDefault();
           this.closePopup();
         }
         break;
 
       case "DOMTitleChanged":
--- a/toolkit/components/extensions/Extension.jsm
+++ b/toolkit/components/extensions/Extension.jsm
@@ -235,17 +235,16 @@ ExtensionPage = class extends BaseContex
     super();
 
     let {type, contentWindow, uri} = params;
     this.extension = extension;
     this.type = type;
     this.contentWindow = contentWindow || null;
     this.uri = uri || extension.baseURI;
     this.incognito = params.incognito || false;
-    this.unloaded = false;
 
     // This is the MessageSender property passed to extension.
     // It can be augmented by the "page-open" hook.
     let sender = {id: extension.uuid};
     if (uri) {
       sender.url = uri.spec;
     }
     let delegate = {
@@ -280,18 +279,16 @@ ExtensionPage = class extends BaseContex
   unload() {
     // Note that without this guard, we end up running unload code
     // multiple times for tab pages closed by the "page-unload" handlers
     // triggered below.
     if (this.unloaded) {
       return;
     }
 
-    this.unloaded = true;
-
     super.unload();
 
     Management.emit("page-unload", this);
 
     this.extension.views.delete(this);
   }
 };
 
--- a/toolkit/components/extensions/ExtensionUtils.jsm
+++ b/toolkit/components/extensions/ExtensionUtils.jsm
@@ -49,16 +49,21 @@ function runSafeWithoutClone(f, ...args)
   Promise.resolve().then(() => {
     runSafeSyncWithoutClone(f, ...args);
   });
 }
 
 // Run a function, cloning arguments into context.cloneScope, and
 // report exceptions. |f| is expected to be in context.cloneScope.
 function runSafeSync(context, f, ...args) {
+  if (context.unloaded) {
+    Cu.reportError("runSafeSync called after context unloaded");
+    return;
+  }
+
   try {
     args = Cu.cloneInto(args, context.cloneScope);
   } catch (e) {
     Cu.reportError(e);
     dump(`runSafe failure: cloning into ${context.cloneScope}: ${e}\n\n${filterStack(Error())}`);
   }
   return runSafeSyncWithoutClone(f, ...args);
 }
@@ -67,16 +72,20 @@ function runSafeSync(context, f, ...args
 // report exceptions. |f| is expected to be in context.cloneScope.
 function runSafe(context, f, ...args) {
   try {
     args = Cu.cloneInto(args, context.cloneScope);
   } catch (e) {
     Cu.reportError(e);
     dump(`runSafe failure: cloning into ${context.cloneScope}: ${e}\n\n${filterStack(Error())}`);
   }
+  if (context.unloaded) {
+    dump(`runSafe failure: context is already unloaded ${filterStack(new Error())}\n`);
+    return undefined;
+  }
   return runSafeWithoutClone(f, ...args);
 }
 
 // Return true if the given value is an instance of the given
 // native type.
 function instanceOf(value, type) {
   return {}.toString.call(value) == `[object ${type}]`;
 }
@@ -130,26 +139,43 @@ class SpreadArgs extends Array {
 let gContextId = 0;
 
 class BaseContext {
   constructor() {
     this.onClose = new Set();
     this.checkedLastError = false;
     this._lastError = null;
     this.contextId = ++gContextId;
+    this.unloaded = false;
   }
 
   get cloneScope() {
     throw new Error("Not implemented");
   }
 
   get principal() {
     throw new Error("Not implemented");
   }
 
+  runSafe(...args) {
+    if (this.unloaded) {
+      Cu.reportError("context.runSafe called after context unloaded");
+    } else {
+      return runSafeSync(this, ...args);
+    }
+  }
+
+  runSafeWithoutClone(...args) {
+    if (this.unloaded) {
+      Cu.reportError("context.runSafeWithoutClone called after context unloaded");
+    } else {
+      return runSafeSyncWithoutClone(...args);
+    }
+  }
+
   checkLoadURL(url, options = {}) {
     let ssm = Services.scriptSecurityManager;
 
     let flags = ssm.STANDARD;
     if (!options.allowScript) {
       flags |= ssm.DISALLOW_SCRIPT;
     }
     if (!options.allowInheritsPrincipal) {
@@ -266,47 +292,65 @@ class BaseContext {
    * @param {function} [callback] The callback function to wrap
    *
    * @returns {Promise|undefined} If callback is null, a promise object
    *     belonging to the target scope. Otherwise, undefined.
    */
   wrapPromise(promise, callback = null) {
     // Note: `promise instanceof this.cloneScope.Promise` returns true
     // here even for promises that do not belong to the content scope.
-    let runSafe = runSafeSync.bind(null, this);
+    let runSafe = this.runSafe.bind(this);
     if (promise.constructor === this.cloneScope.Promise) {
-      runSafe = runSafeSyncWithoutClone;
+      runSafe = this.runSafeWithoutClone.bind(this);
     }
 
     if (callback) {
       promise.then(
         args => {
-          if (args instanceof SpreadArgs) {
+          if (this.unloaded) {
+            dump(`Promise resolved after context unloaded\n`);
+          } else if (args instanceof SpreadArgs) {
             runSafe(callback, ...args);
           } else {
             runSafe(callback, args);
           }
         },
         error => {
           this.withLastError(error, () => {
-            runSafeSyncWithoutClone(callback);
+            if (this.unloaded) {
+              dump(`Promise rejected after context unloaded\n`);
+            } else {
+              this.runSafeWithoutClone(callback);
+            }
           });
         });
     } else {
       return new this.cloneScope.Promise((resolve, reject) => {
         promise.then(
-          value => { runSafe(resolve, value); },
           value => {
-            runSafeSyncWithoutClone(reject, this.normalizeError(value));
+            if (this.unloaded) {
+              dump(`Promise resolved after context unloaded\n`);
+            } else {
+              runSafe(resolve, value);
+            }
+          },
+          value => {
+            if (this.unloaded) {
+              dump(`Promise rejected after context unloaded\n`);
+            } else {
+              this.runSafeWithoutClone(reject, this.normalizeError(value));
+            }
           });
       });
     }
   }
 
   unload() {
+    this.unloaded = true;
+
     MessageChannel.abortResponses({
       extensionId: this.extension.id,
       contextId: this.contextId,
     });
 
     for (let obj of this.onClose) {
       obj.close();
     }
@@ -565,23 +609,29 @@ EventManager.prototype = {
   },
 
   hasListener(callback) {
     return this.callbacks.has(callback);
   },
 
   fire(...args) {
     for (let callback of this.callbacks) {
-      runSafe(this.context, callback, ...args);
+      Promise.resolve(callback).then(callback => {
+        if (this.context.unloaded) {
+          dump(`${this.name} event fired after context unloaded.`);
+        } else if (this.callbacks.has(callback)) {
+          this.context.runSafe(callback, ...args);
+        }
+      });
     }
   },
 
   fireWithoutClone(...args) {
     for (let callback of this.callbacks) {
-      runSafeSyncWithoutClone(callback, ...args);
+      this.context.runSafeWithoutClone(callback, ...args);
     }
   },
 
   close() {
     if (this.callbacks.size) {
       this.unregister();
     }
     this.callbacks = null;
@@ -604,17 +654,25 @@ function SingletonEventManager(context, 
   this.name = name;
   this.register = register;
   this.unregister = new Map();
   context.callOnClose(this);
 }
 
 SingletonEventManager.prototype = {
   addListener(callback, ...args) {
-    let unregister = this.register(callback, ...args);
+    let wrappedCallback = (...args) => {
+      if (this.context.unloaded) {
+        dump(`${this.name} event fired after context unloaded.`);
+      } else if (this.unregister.has(callback)) {
+        return callback(...args);
+      }
+    };
+
+    let unregister = this.register(wrappedCallback, ...args);
     this.unregister.set(callback, unregister);
   },
 
   removeListener(callback) {
     if (!this.unregister.has(callback)) {
       return;
     }
 
@@ -928,17 +986,17 @@ Messenger.prototype = {
 
         receiveMessage: ({target, data: message, sender, recipient}) => {
           let {name, portId} = message;
           let mm = getMessageManager(target);
           if (this.delegate) {
             this.delegate.getSender(this.context, target, sender);
           }
           let port = new Port(this.context, mm, name, portId, sender);
-          runSafeSyncWithoutClone(callback, port.api());
+          this.context.runSafeWithoutClone(callback, port.api());
           return true;
         },
       };
 
       MessageChannel.addListener(this.messageManagers, "Extension:Connect", listener);
       return () => {
         MessageChannel.removeListener(this.messageManagers, "Extension:Connect", listener);
       };