Bug 1203330 Part 1 Fix SingletonEventManager draft
authorAndrew Swan <aswan@mozilla.com>
Thu, 26 Jan 2017 20:00:33 -0800
changeset 467097 7f3116c0c84cf48b9d8e1656f451c4e764299917
parent 466204 c989c7b352279925edf138373e4ca3f1540dbd5f
child 467098 aa0b2a64e4f4d3976b216721e21ae9ed95dde3f6
push id43100
push useraswan@mozilla.com
push dateFri, 27 Jan 2017 04:03:36 +0000
bugs1203330
milestone54.0a1
Bug 1203330 Part 1 Fix SingletonEventManager This patch adds the ability to run SingletonEventManager handlers in different modes: sync, async, raw (no exception handling, arg cloning, or asynchrony), or asyncWithoutClone. When you call the handler, you're required to specify which variant you want. Existing uses of SingletonEventManager are all converted to async calls. Note that some of them were previously synchronous, but it didn't appear to be necessary. Also added a callOnClose for SingletonEventManager when the last listener is removed. MozReview-Commit-ID: ATHO97dWf3X
browser/components/extensions/ext-bookmarks.js
browser/components/extensions/ext-c-omnibox.js
browser/components/extensions/ext-history.js
browser/components/extensions/ext-omnibox.js
browser/components/extensions/ext-sessions.js
mobile/android/components/extensions/ext-pageAction.js
toolkit/components/extensions/ExtensionChild.jsm
toolkit/components/extensions/ExtensionUtils.jsm
toolkit/components/extensions/ext-c-test.js
toolkit/components/extensions/ext-downloads.js
toolkit/components/extensions/ext-idle.js
toolkit/components/extensions/ext-runtime.js
toolkit/components/extensions/ext-webNavigation.js
toolkit/components/extensions/ext-webRequest.js
toolkit/components/extensions/test/xpcshell/test_ext_contexts.js
--- a/browser/components/extensions/ext-bookmarks.js
+++ b/browser/components/extensions/ext-bookmarks.js
@@ -315,56 +315,56 @@ extensions.registerSchemaAPI("bookmarks"
             .catch(error => Promise.reject({message: error.message}));
         } catch (e) {
           return Promise.reject({message: `Invalid bookmark: ${JSON.stringify(info)}`});
         }
       },
 
       onCreated: new SingletonEventManager(context, "bookmarks.onCreated", fire => {
         let listener = (event, bookmark) => {
-          context.runSafe(fire, bookmark.id, bookmark);
+          fire.sync(bookmark.id, bookmark);
         };
 
         observer.on("created", listener);
         incrementListeners();
         return () => {
           observer.off("created", listener);
           decrementListeners();
         };
       }).api(),
 
       onRemoved: new SingletonEventManager(context, "bookmarks.onRemoved", fire => {
         let listener = (event, data) => {
-          context.runSafe(fire, data.guid, data.info);
+          fire.sync(data.guid, data.info);
         };
 
         observer.on("removed", listener);
         incrementListeners();
         return () => {
           observer.off("removed", listener);
           decrementListeners();
         };
       }).api(),
 
       onChanged: new SingletonEventManager(context, "bookmarks.onChanged", fire => {
         let listener = (event, data) => {
-          context.runSafe(fire, data.guid, data.info);
+          fire.sync(data.guid, data.info);
         };
 
         observer.on("changed", listener);
         incrementListeners();
         return () => {
           observer.off("changed", listener);
           decrementListeners();
         };
       }).api(),
 
       onMoved: new SingletonEventManager(context, "bookmarks.onMoved", fire => {
         let listener = (event, data) => {
-          context.runSafe(fire, data.guid, data.info);
+          fire.sync(data.guid, data.info);
         };
 
         observer.on("moved", listener);
         incrementListeners();
         return () => {
           observer.off("moved", listener);
           decrementListeners();
         };
--- a/browser/components/extensions/ext-c-omnibox.js
+++ b/browser/components/extensions/ext-c-omnibox.js
@@ -1,25 +1,24 @@
 /* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
 /* vim: set sts=2 sw=2 et tw=80: */
 "use strict";
 
 Cu.import("resource://gre/modules/ExtensionUtils.jsm");
 
 var {
-  runSafeSyncWithoutClone,
   SingletonEventManager,
 } = ExtensionUtils;
 
 extensions.registerSchemaAPI("omnibox", "addon_child", context => {
   return {
     omnibox: {
       onInputChanged: new SingletonEventManager(context, "omnibox.onInputChanged", fire => {
         let listener = (text, id) => {
-          runSafeSyncWithoutClone(fire, text, suggestions => {
+          fire.asyncWithoutClone(text, suggestions => {
             // TODO: Switch to using callParentFunctionNoReturn once bug 1314903 is fixed.
             context.childManager.callParentAsyncFunction("omnibox_internal.addSuggestions", [
               id,
               suggestions,
             ]);
           });
         };
         context.childManager.getParentEvent("omnibox_internal.onInputChanged").addListener(listener);
--- a/browser/components/extensions/ext-history.js
+++ b/browser/components/extensions/ext-history.js
@@ -217,28 +217,28 @@ extensions.registerSchemaAPI("history", 
         historyQuery.uri = NetUtil.newURI(url);
         let queryResult = PlacesUtils.history.executeQuery(historyQuery, options).root;
         let results = convertNavHistoryContainerResultNode(queryResult, convertNodeToVisitItem);
         return Promise.resolve(results);
       },
 
       onVisited: new SingletonEventManager(context, "history.onVisited", fire => {
         let listener = (event, data) => {
-          context.runSafe(fire, data);
+          fire.sync(data);
         };
 
         getObserver().on("visited", listener);
         return () => {
           getObserver().off("visited", listener);
         };
       }).api(),
 
       onVisitRemoved: new SingletonEventManager(context, "history.onVisitRemoved", fire => {
         let listener = (event, data) => {
-          context.runSafe(fire, data);
+          fire.sync(data);
         };
 
         getObserver().on("visitRemoved", listener);
         return () => {
           getObserver().off("visitRemoved", listener);
         };
       }).api(),
     },
--- a/browser/components/extensions/ext-omnibox.js
+++ b/browser/components/extensions/ext-omnibox.js
@@ -45,37 +45,37 @@ extensions.registerSchemaAPI("omnibox", 
           ExtensionSearchHandler.setDefaultSuggestion(keyword, suggestion);
         } catch (e) {
           return Promise.reject(e.message);
         }
       },
 
       onInputStarted: new SingletonEventManager(context, "omnibox.onInputStarted", fire => {
         let listener = (eventName) => {
-          fire();
+          fire.sync();
         };
         extension.on(ExtensionSearchHandler.MSG_INPUT_STARTED, listener);
         return () => {
           extension.off(ExtensionSearchHandler.MSG_INPUT_STARTED, listener);
         };
       }).api(),
 
       onInputCancelled: new SingletonEventManager(context, "omnibox.onInputCancelled", fire => {
         let listener = (eventName) => {
-          fire();
+          fire.sync();
         };
         extension.on(ExtensionSearchHandler.MSG_INPUT_CANCELLED, listener);
         return () => {
           extension.off(ExtensionSearchHandler.MSG_INPUT_CANCELLED, listener);
         };
       }).api(),
 
       onInputEntered: new SingletonEventManager(context, "omnibox.onInputEntered", fire => {
         let listener = (eventName, text, disposition) => {
-          fire(text, disposition);
+          fire.sync(text, disposition);
         };
         extension.on(ExtensionSearchHandler.MSG_INPUT_ENTERED, listener);
         return () => {
           extension.off(ExtensionSearchHandler.MSG_INPUT_ENTERED, listener);
         };
       }).api(),
     },
 
@@ -87,17 +87,17 @@ extensions.registerSchemaAPI("omnibox", 
         } catch (e) {
           // Silently fail because the extension developer can not know for sure if the user
           // has already invalidated the callback when asynchronously providing suggestions.
         }
       },
 
       onInputChanged: new SingletonEventManager(context, "omnibox_internal.onInputChanged", fire => {
         let listener = (eventName, text, id) => {
-          fire(text, id);
+          fire.sync(text, id);
         };
         extension.on(ExtensionSearchHandler.MSG_INPUT_CHANGED, listener);
         return () => {
           extension.off(ExtensionSearchHandler.MSG_INPUT_CHANGED, listener);
         };
       }).api(),
     },
   };
--- a/browser/components/extensions/ext-sessions.js
+++ b/browser/components/extensions/ext-sessions.js
@@ -89,17 +89,17 @@ extensions.registerSchemaAPI("sessions",
           closedId = recentlyClosedTabs[0].closedId;
           session = SessionStore.undoCloseById(closedId);
         }
         return createSession(session, extension, closedId);
       },
 
       onChanged: new SingletonEventManager(context, "sessions.onChanged", fire => {
         let observer = () => {
-          context.runSafe(fire);
+          fire.async();
         };
 
         Services.obs.addObserver(observer, SS_ON_CLOSED_OBJECTS_CHANGED, false);
         return () => {
           Services.obs.removeObserver(observer, SS_ON_CLOSED_OBJECTS_CHANGED);
         };
       }).api(),
     },
--- a/mobile/android/components/extensions/ext-pageAction.js
+++ b/mobile/android/components/extensions/ext-pageAction.js
@@ -127,17 +127,17 @@ extensions.on("shutdown", (type, extensi
 /* eslint-enable mozilla/balanced-listeners */
 
 extensions.registerSchemaAPI("pageAction", "addon_parent", context => {
   let {extension} = context;
   return {
     pageAction: {
       onClicked: new SingletonEventManager(context, "pageAction.onClicked", fire => {
         let listener = (event) => {
-          fire();
+          fire.async();
         };
         pageActionMap.get(extension).on("click", listener);
         return () => {
           pageActionMap.get(extension).off("click", listener);
         };
       }).api(),
 
       show(tabId) {
--- a/toolkit/components/extensions/ExtensionChild.jsm
+++ b/toolkit/components/extensions/ExtensionChild.jsm
@@ -325,17 +325,17 @@ class Messenger {
   }
 
   sendNativeMessage(messageManager, msg, recipient, responseCallback) {
     msg = NativeApp.encodeMessage(this.context, msg);
     return this.sendMessage(messageManager, msg, recipient, responseCallback);
   }
 
   onMessage(name) {
-    return new SingletonEventManager(this.context, name, callback => {
+    return new SingletonEventManager(this.context, name, fire => {
       let listener = {
         messageFilterPermissive: this.optionalFilter,
         messageFilterStrict: this.filter,
 
         filterMessage: (sender, recipient) => {
           // Ignore the message if it was sent by this Messenger.
           return sender.contextId !== this.context.contextId;
         },
@@ -355,17 +355,17 @@ class Messenger {
           });
 
           message = Cu.cloneInto(message, this.context.cloneScope);
           sender = Cu.cloneInto(sender, this.context.cloneScope);
           sendResponse = Cu.exportFunction(sendResponse, this.context.cloneScope);
 
           // Note: We intentionally do not use runSafe here so that any
           // errors are propagated to the message sender.
-          let result = callback(message, sender, sendResponse);
+          let result = fire.raw(message, sender, sendResponse);
           if (result instanceof this.context.cloneScope.Promise) {
             return result;
           } else if (result === true) {
             return promise;
           }
           return response;
         },
       };
@@ -407,17 +407,17 @@ class Messenger {
     let portId = getUniqueId();
 
     let port = new NativePort(this.context, messageManager, this.messageManagers, name, portId, null, recipient);
 
     return this._connect(messageManager, port, recipient);
   }
 
   onConnect(name) {
-    return new SingletonEventManager(this.context, name, callback => {
+    return new SingletonEventManager(this.context, name, fire => {
       let listener = {
         messageFilterPermissive: this.optionalFilter,
         messageFilterStrict: this.filter,
 
         filterMessage: (sender, recipient) => {
           // Ignore the port if it was created by this Messenger.
           return sender.contextId !== this.context.contextId;
         },
@@ -426,17 +426,17 @@ class Messenger {
           let {name, portId} = message;
           let mm = getMessageManager(target);
           let recipient = Object.assign({}, sender);
           if (recipient.tab) {
             recipient.tabId = recipient.tab.id;
             delete recipient.tab;
           }
           let port = new Port(this.context, mm, this.messageManagers, name, portId, sender, recipient);
-          this.context.runSafeWithoutClone(callback, port.api());
+          fire.asyncWithoutClone(port.api());
           return true;
         },
       };
 
       MessageChannel.addListener(this.messageManagers, "Extension:Connect", listener);
       return () => {
         MessageChannel.removeListener(this.messageManagers, "Extension:Connect", listener);
       };
--- a/toolkit/components/extensions/ExtensionUtils.jsm
+++ b/toolkit/components/extensions/ExtensionUtils.jsm
@@ -695,37 +695,76 @@ function SingletonEventManager(context, 
   this.context = context;
   this.name = name;
   this.register = register;
   this.unregister = new Map();
 }
 
 SingletonEventManager.prototype = {
   addListener(callback, ...args) {
-    let wrappedCallback = (...args) => {
+    if (this.unregister.has(callback)) {
+      return;
+    }
+
+    let shouldFire = () => {
       if (this.context.unloaded) {
         dump(`${this.name} event fired after context unloaded.\n`);
+      } else if (!this.context.active) {
+        dump(`${this.name} event fired while context is inactive.\n`);
       } else if (this.unregister.has(callback)) {
-        return callback(...args);
+        return true;
       }
+      return false;
     };
 
-    let unregister = this.register(wrappedCallback, ...args);
+    let fire = {
+      sync: (...args) => {
+        if (shouldFire()) {
+          return this.context.runSafe(callback, ...args);
+        }
+      },
+      async: (...args) => {
+        return Promise.resolve().then(() => {
+          if (shouldFire()) {
+            return this.context.runSafe(callback, ...args);
+          }
+        });
+      },
+      raw: (...args) => {
+        if (!shouldFire()) {
+          throw new Error("Called raw() on unloaded/inactive context");
+        }
+        return callback(...args);
+      },
+      asyncWithoutClone: (...args) => {
+        return Promise.resolve().then(() => {
+          if (shouldFire()) {
+            return this.context.runSafeWithoutClone(callback, ...args);
+          }
+        });
+      },
+    };
+
+
+    let unregister = this.register(fire, ...args);
     this.unregister.set(callback, unregister);
     this.context.callOnClose(this);
   },
 
   removeListener(callback) {
     if (!this.unregister.has(callback)) {
       return;
     }
 
     let unregister = this.unregister.get(callback);
     this.unregister.delete(callback);
     unregister();
+    if (this.unregister.size == 0) {
+      this.context.forgetOnClose(this);
+    }
   },
 
   hasListener(callback) {
     return this.unregister.has(callback);
   },
 
   close() {
     for (let unregister of this.unregister.values()) {
--- a/toolkit/components/extensions/ext-c-test.js
+++ b/toolkit/components/extensions/ext-c-test.js
@@ -166,17 +166,17 @@ function makeTestAPI(context) {
           assertTrue(errorMatches(error, expectedError, context),
                      `Function threw, expecting error to match ${toSource(expectedError)}` +
                      `got ${errorMessage}${msg}`);
         }
       },
 
       onMessage: new SingletonEventManager(context, "test.onMessage", fire => {
         let handler = (event, ...args) => {
-          context.runSafe(fire, ...args);
+          fire.async(...args);
         };
 
         extension.on("test-harness-message", handler);
         return () => {
           extension.off("test-harness-message", handler);
         };
       }).api(),
     },
--- a/toolkit/components/extensions/ext-downloads.js
+++ b/toolkit/components/extensions/ext-downloads.js
@@ -16,17 +16,16 @@ XPCOMUtils.defineLazyModuleGetter(this, 
                                   "resource://gre/modules/NetUtil.jsm");
 XPCOMUtils.defineLazyModuleGetter(this, "EventEmitter",
                                   "resource://devtools/shared/event-emitter.js");
 
 Cu.import("resource://gre/modules/ExtensionUtils.jsm");
 const {
   ignoreEvent,
   normalizeTime,
-  runSafeSync,
   SingletonEventManager,
   PlatformInfo,
 } = ExtensionUtils;
 
 const DOWNLOAD_ITEM_FIELDS = ["id", "url", "referrer", "filename", "incognito",
                               "danger", "mime", "startTime", "endTime",
                               "estimatedEndTime", "state",
                               "paused", "canResume", "error",
@@ -746,47 +745,47 @@ extensions.registerSchemaAPI("downloads"
               changes[fld] = {
                 previous: noundef(item.prechange[fld]),
                 current: noundef(item[fld]),
               };
             }
           });
           if (Object.keys(changes).length > 0) {
             changes.id = item.id;
-            runSafeSync(context, fire, changes);
+            fire.async(changes);
           }
         };
 
         let registerPromise = DownloadMap.getDownloadList().then(() => {
           DownloadMap.on("change", handler);
         });
         return () => {
           registerPromise.then(() => {
             DownloadMap.off("change", handler);
           });
         };
       }).api(),
 
       onCreated: new SingletonEventManager(context, "downloads.onCreated", fire => {
         const handler = (what, item) => {
-          runSafeSync(context, fire, item.serialize());
+          fire.async(item.serialize());
         };
         let registerPromise = DownloadMap.getDownloadList().then(() => {
           DownloadMap.on("create", handler);
         });
         return () => {
           registerPromise.then(() => {
             DownloadMap.off("create", handler);
           });
         };
       }).api(),
 
       onErased: new SingletonEventManager(context, "downloads.onErased", fire => {
         const handler = (what, item) => {
-          runSafeSync(context, fire, item.id);
+          fire.async(item.id);
         };
         let registerPromise = DownloadMap.getDownloadList().then(() => {
           DownloadMap.on("erase", handler);
         });
         return () => {
           registerPromise.then(() => {
             DownloadMap.off("erase", handler);
           });
--- a/toolkit/components/extensions/ext-idle.js
+++ b/toolkit/components/extensions/ext-idle.js
@@ -76,17 +76,17 @@ extensions.registerSchemaAPI("idle", "ad
         }
         return Promise.resolve("idle");
       },
       setDetectionInterval: function(detectionIntervalInSeconds) {
         setDetectionInterval(extension, context, detectionIntervalInSeconds);
       },
       onStateChanged: new SingletonEventManager(context, "idle.onStateChanged", fire => {
         let listener = (event, data) => {
-          context.runSafe(fire, data);
+          fire.sync(data);
         };
 
         getObserver(extension, context).on("stateChanged", listener);
         return () => {
           getObserver(extension, context).off("stateChanged", listener);
         };
       }).api(),
     },
--- a/toolkit/components/extensions/ext-runtime.js
+++ b/toolkit/components/extensions/ext-runtime.js
@@ -23,55 +23,55 @@ extensions.registerSchemaAPI("runtime", 
     runtime: {
       onStartup: new SingletonEventManager(context, "runtime.onStartup", fire => {
         if (context.incognito) {
           // This event should not fire if we are operating in a private profile.
           return () => {};
         }
         let listener = () => {
           if (extension.startupReason === "APP_STARTUP") {
-            fire();
+            fire.sync();
           }
         };
         extension.on("startup", listener);
         return () => {
           extension.off("startup", listener);
         };
       }).api(),
 
       onInstalled: new SingletonEventManager(context, "runtime.onInstalled", fire => {
         let listener = () => {
           switch (extension.startupReason) {
             case "APP_STARTUP":
               if (Extension.browserUpdated) {
-                fire({reason: "browser_update"});
+                fire.sync({reason: "browser_update"});
               }
               break;
             case "ADDON_INSTALL":
-              fire({reason: "install"});
+              fire.sync({reason: "install"});
               break;
             case "ADDON_UPGRADE":
-              fire({reason: "update"});
+              fire.sync({reason: "update"});
               break;
           }
         };
         extension.on("startup", listener);
         return () => {
           extension.off("startup", listener);
         };
       }).api(),
 
       onUpdateAvailable: new SingletonEventManager(context, "runtime.onUpdateAvailable", fire => {
         let instanceID = extension.addonData.instanceID;
         AddonManager.addUpgradeListener(instanceID, upgrade => {
           extension.upgrade = upgrade;
           let details = {
             version: upgrade.version,
           };
-          context.runSafe(fire, details);
+          fire.sync(details);
         });
         return () => {
           AddonManager.removeUpgradeListener(instanceID);
         };
       }).api(),
 
       reload: () => {
         if (extension.upgrade) {
--- a/toolkit/components/extensions/ext-webNavigation.js
+++ b/toolkit/components/extensions/ext-webNavigation.js
@@ -93,17 +93,17 @@ function fillTransitionProperties(eventN
     dst.transitionType = transitionType;
     dst.transitionQualifiers = transitionQualifiers;
   }
 }
 
 // Similar to WebRequestEventManager but for WebNavigation.
 function WebNavigationEventManager(context, eventName) {
   let name = `webNavigation.${eventName}`;
-  let register = (callback, urlFilters) => {
+  let register = (fire, urlFilters) => {
     // Don't create a MatchURLFilters instance if the listener does not include any filter.
     let filters = urlFilters ?
           new MatchURLFilters(urlFilters.url) : null;
 
     let listener = data => {
       if (!data.browser) {
         return;
       }
@@ -122,17 +122,17 @@ function WebNavigationEventManager(conte
       // Fills in tabId typically.
       extensions.emit("fill-browser-data", data.browser, data2);
       if (data2.tabId < 0) {
         return;
       }
 
       fillTransitionProperties(eventName, data, data2);
 
-      context.runSafe(callback, data2);
+      fire.async(data2);
     };
 
     WebNavigation[eventName].addListener(listener, filters);
     return () => {
       WebNavigation[eventName].removeListener(listener);
     };
   };
 
--- a/toolkit/components/extensions/ext-webRequest.js
+++ b/toolkit/components/extensions/ext-webRequest.js
@@ -15,17 +15,17 @@ var {
   SingletonEventManager,
 } = ExtensionUtils;
 
 // EventManager-like class specifically for WebRequest. Inherits from
 // SingletonEventManager. Takes care of converting |details| parameter
 // when invoking listeners.
 function WebRequestEventManager(context, eventName) {
   let name = `webRequest.${eventName}`;
-  let register = (callback, filter, info) => {
+  let register = (fire, filter, info) => {
     let listener = data => {
       // Prevent listening in on requests originating from system principal to
       // prevent tinkering with OCSP, app and addon updates, etc.
       if (data.isSystemPrincipal) {
         return;
       }
       let browserData = {};
       extensions.emit("fill-browser-data", data.browser, browserData);
@@ -60,17 +60,17 @@ function WebRequestEventManager(context,
       let optional = ["requestHeaders", "responseHeaders", "statusCode", "statusLine", "error", "redirectUrl",
                       "requestBody"];
       for (let opt of optional) {
         if (opt in data) {
           data2[opt] = data[opt];
         }
       }
 
-      return context.runSafe(callback, data2);
+      return fire.sync(data2);
     };
 
     let filter2 = {};
     filter2.urls = new MatchPattern(filter.urls);
     if (filter.types) {
       filter2.types = filter.types;
     }
     if (filter.tabId) {
--- a/toolkit/components/extensions/test/xpcshell/test_ext_contexts.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_contexts.js
@@ -67,19 +67,19 @@ add_task(function* test_post_unload_list
 
   let fireEvent;
   let onEvent = new EventManager(context, "onEvent", fire => {
     fireEvent = fire;
     return () => {};
   });
 
   let fireSingleton;
-  let onSingleton = new SingletonEventManager(context, "onSingleton", callback => {
+  let onSingleton = new SingletonEventManager(context, "onSingleton", fire => {
     fireSingleton = () => {
-      Promise.resolve().then(callback);
+      fire.async();
     };
     return () => {};
   });
 
   let fail = event => {
     ok(false, `Unexpected event: ${event}`);
   };