Bug 1074135 - Callback after exceptions when calling async provider methods. r=Unfocused, r=Mossop, a=sledru
authorIrving Reid <irving@mozilla.com>
Mon, 29 Sep 2014 13:01:50 -0400
changeset 218105 b3ce9237bb9a
parent 218104 bda37eb8a921
child 218108 f7b82b004588
push id542
push userryanvm@gmail.com
push date2014-10-22 19:27 +0000
treeherdermozilla-release@b3ce9237bb9a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersUnfocused, Mossop, sledru
bugs1074135
milestone33.0.1
Bug 1074135 - Callback after exceptions when calling async provider methods. r=Unfocused, r=Mossop, a=sledru
toolkit/mozapps/extensions/AddonManager.jsm
toolkit/mozapps/extensions/test/xpcshell/test_provider_shutdown.js
--- a/toolkit/mozapps/extensions/AddonManager.jsm
+++ b/toolkit/mozapps/extensions/AddonManager.jsm
@@ -118,16 +118,19 @@ let logger = Log.repository.getLogger(LO
 // missing or 'false', messages at the WARNING and higher
 // severity should be logged to the JS console and standard error.
 // If "extensions.logging.enabled" is set to 'true', messages
 // at DEBUG and higher should go to JS console and standard error.
 const PREF_LOGGING_ENABLED = "extensions.logging.enabled";
 const NS_PREFBRANCH_PREFCHANGE_TOPIC_ID = "nsPref:changed";
 
 const UNNAMED_PROVIDER = "<unnamed-provider>";
+function providerName(aProvider) {
+  return aProvider.name || UNNAMED_PROVIDER;
+}
 
 /**
  * Preference listener which listens for a change in the
  * "extensions.logging.enabled" preference and changes the logging level of the
  * parent 'addons' level logger accordingly.
  */
 var PrefObserver = {
     init: function PrefObserver_init() {
@@ -172,43 +175,80 @@ function safeCall(aCallback, ...aArgs) {
     aCallback.apply(null, aArgs);
   }
   catch (e) {
     logger.warn("Exception calling callback", e);
   }
 }
 
 /**
+ * Report an exception thrown by a provider API method.
+ */
+function reportProviderError(aProvider, aMethod, aError) {
+  let method = "provider " + providerName(aProvider) + "." + aMethod;
+  AddonManagerPrivate.recordException("AMI", method, aError);
+  logger.error("Exception calling " + method, aError);
+}
+
+/**
  * Calls a method on a provider if it exists and consumes any thrown exception.
- * Any parameters after the dflt parameter are passed to the provider's method.
+ * Any parameters after the aDefault parameter are passed to the provider's method.
  *
  * @param  aProvider
  *         The provider to call
  * @param  aMethod
  *         The method name to call
  * @param  aDefault
  *         A default return value if the provider does not implement the named
  *         method or throws an error.
- * @return the return value from the provider or dflt if the provider does not
+ * @return the return value from the provider, or aDefault if the provider does not
  *         implement method or throws an error
  */
 function callProvider(aProvider, aMethod, aDefault, ...aArgs) {
   if (!(aMethod in aProvider))
     return aDefault;
 
   try {
     return aProvider[aMethod].apply(aProvider, aArgs);
   }
   catch (e) {
-    logger.error("Exception calling provider " + aMethod, e);
+    reportProviderError(aProvider, aMethod, e);
     return aDefault;
   }
 }
 
 /**
+ * Calls a method on a provider if it exists and consumes any thrown exception.
+ * Parameters after aMethod are passed to aProvider.aMethod().
+ * The last parameter must be a callback function.
+ * If the provider does not implement the method, or the method throws, calls
+ * the callback with 'undefined'.
+ *
+ * @param  aProvider
+ *         The provider to call
+ * @param  aMethod
+ *         The method name to call
+ */
+function callProviderAsync(aProvider, aMethod, ...aArgs) {
+  let callback = aArgs[aArgs.length - 1];
+  if (!(aMethod in aProvider)) {
+    callback(undefined);
+    return;
+  }
+  try {
+    return aProvider[aMethod].apply(aProvider, aArgs);
+  }
+  catch (e) {
+    reportProviderError(aProvider, aMethod, e);
+    callback(undefined);
+    return;
+  }
+}
+
+/**
  * Gets the currently selected locale for display.
  * @return  the selected locale or "en-US" if none is selected
  */
 function getLocale() {
   try {
     if (Services.prefs.getBoolPref(PREF_MATCH_OS_LOCALE))
       return Services.locale.getLocaleComponentForUserAgent();
   }
@@ -623,17 +663,17 @@ var AddonManagerInternal = {
    */
   _startProvider: function(aProvider, aAppChanged, aOldAppVersion, aOldPlatformVersion) {
     if (!gStarted)
       throw Components.Exception("AddonManager is not initialized",
                                  Cr.NS_ERROR_NOT_INITIALIZED);
 
     callProvider(aProvider, "startup", null, aAppChanged, aOldAppVersion, aOldPlatformVersion);
     if ('shutdown' in aProvider) {
-      let name = aProvider.name || "Provider";
+      let name = providerName(aProvider);
       let AMProviderShutdown = () => {
         return new Promise((resolve, reject) => {
             logger.debug("Calling shutdown blocker for " + name);
             resolve(aProvider.shutdown());
           })
           .catch(err => {
             logger.warn("Failure during shutdown of " + name, err);
             AddonManagerPrivate.recordException("AMI", "Async shutdown of " + name, err);
@@ -888,31 +928,33 @@ var AddonManagerInternal = {
         }
       }
     }
 
     // If we're unregistering after startup but before shutting down,
     // remove the blocker for this provider's shutdown and call it.
     // If we're already shutting down, just let gShutdownBarrier call it to avoid races.
     if (gStarted && !gShutdownInProgress) {
-      logger.debug("Unregistering shutdown blocker for " + (aProvider.name || "Provider"));
+      logger.debug("Unregistering shutdown blocker for " + providerName(aProvider));
       let shutter = this.providerShutdowns.get(aProvider);
       if (shutter) {
         this.providerShutdowns.delete(aProvider);
         gShutdownBarrier.client.removeBlocker(shutter);
         return shutter();
       }
     }
     return undefined;
   },
 
   /**
    * Calls a method on all registered providers if it exists and consumes any
    * thrown exception. Return values are ignored. Any parameters after the
    * method parameter are passed to the provider's method.
+   * WARNING: Do not use for asynchronous calls; callProviders() does not
+   * invoke callbacks if provider methods throw synchronous exceptions.
    *
    * @param  aMethod
    *         The method name to call
    * @see    callProvider
    */
   callProviders: function AMI_callProviders(aMethod, ...aArgs) {
     if (!aMethod || typeof aMethod != "string")
       throw Components.Exception("aMethod must be a non-empty string",
@@ -920,18 +962,17 @@ var AddonManagerInternal = {
 
     let providers = this.providers.slice(0);
     for (let provider of providers) {
       try {
         if (aMethod in provider)
           provider[aMethod].apply(provider, aArgs);
       }
       catch (e) {
-        AddonManagerPrivate.recordException("AMI", "provider " + aMethod, e);
-        logger.error("Exception calling provider " + aMethod, e);
+        reportProviderError(aProvider, aMethod, e);
       }
     }
   },
 
   /**
    * Report the current state of asynchronous shutdown
    */
   shutdownState: function() {
@@ -971,17 +1012,17 @@ var AddonManagerInternal = {
     // Only shut down providers if they've been started.
     if (gStarted) {
       try {
         yield gShutdownBarrier.wait();
       }
       catch(err) {
         savedError = err;
         logger.error("Failure during wait for shutdown barrier", err);
-        AddonManagerPrivate.recordException("AMI", "Async shutdown of AddonRepository", err);
+        AddonManagerPrivate.recordException("AMI", "Async shutdown of AddonManager providers", err);
       }
     }
 
     // Shut down AddonRepository after providers (if any).
     try {
       gRepoShutdownState = "in progress";
       yield AddonRepository.shutdown();
       gRepoShutdownState = "done";
@@ -1548,20 +1589,18 @@ var AddonManagerInternal = {
                                  Cr.NS_ERROR_NOT_INITIALIZED);
 
     if (typeof aCallback != "function")
       throw Components.Exception("aCallback must be a function",
                                  Cr.NS_ERROR_INVALID_ARG);
 
     new AsyncObjectCaller(this.providers, "updateAddonRepositoryData", {
       nextObject: function updateAddonRepositoryData_nextObject(aCaller, aProvider) {
-        callProvider(aProvider,
-                     "updateAddonRepositoryData",
-                     null,
-                     aCaller.callNext.bind(aCaller));
+        callProviderAsync(aProvider, "updateAddonRepositoryData",
+                          aCaller.callNext.bind(aCaller));
       },
       noMoreObjects: function updateAddonRepositoryData_noMoreObjects(aCaller) {
         safeCall(aCallback);
         // only tests should care about this
         Services.obs.notifyObservers(null, "TEST:addon-repository-data-updated", null);
       }
     });
   },
@@ -1630,19 +1669,19 @@ var AddonManagerInternal = {
 
     if (aLoadGroup && (!(aLoadGroup instanceof Ci.nsILoadGroup)))
       throw Components.Exception("aLoadGroup must be a nsILoadGroup or null",
                                  Cr.NS_ERROR_INVALID_ARG);
 
     let providers = this.providers.slice(0);
     for (let provider of providers) {
       if (callProvider(provider, "supportsMimetype", false, aMimetype)) {
-        callProvider(provider, "getInstallForURL", null,
-                     aUrl, aHash, aName, aIcons, aVersion, aLoadGroup,
-                     function  getInstallForURL_safeCall(aInstall) {
+        callProviderAsync(provider, "getInstallForURL",
+                          aUrl, aHash, aName, aIcons, aVersion, aLoadGroup,
+                          function  getInstallForURL_safeCall(aInstall) {
           safeCall(aCallback, aInstall);
         });
         return;
       }
     }
     safeCall(aCallback, null);
   },
 
@@ -1671,18 +1710,18 @@ var AddonManagerInternal = {
                                  Cr.NS_ERROR_INVALID_ARG);
 
     if (aMimetype && typeof aMimetype != "string")
       throw Components.Exception("aMimetype must be a string or null",
                                  Cr.NS_ERROR_INVALID_ARG);
 
     new AsyncObjectCaller(this.providers, "getInstallForFile", {
       nextObject: function getInstallForFile_nextObject(aCaller, aProvider) {
-        callProvider(aProvider, "getInstallForFile", null, aFile,
-                     function getInstallForFile_safeCall(aInstall) {
+        callProviderAsync(aProvider, "getInstallForFile", aFile,
+                          function getInstallForFile_safeCall(aInstall) {
           if (aInstall)
             safeCall(aCallback, aInstall);
           else
             aCaller.callNext();
         });
       },
 
       noMoreObjects: function getInstallForFile_noMoreObjects(aCaller) {
@@ -1713,18 +1752,18 @@ var AddonManagerInternal = {
     if (typeof aCallback != "function")
       throw Components.Exception("aCallback must be a function",
                                  Cr.NS_ERROR_INVALID_ARG);
 
     let installs = [];
 
     new AsyncObjectCaller(this.providers, "getInstallsByTypes", {
       nextObject: function getInstallsByTypes_nextObject(aCaller, aProvider) {
-        callProvider(aProvider, "getInstallsByTypes", null, aTypes,
-                     function getInstallsByTypes_safeCall(aProviderInstalls) {
+        callProviderAsync(aProvider, "getInstallsByTypes", aTypes,
+                          function getInstallsByTypes_safeCall(aProviderInstalls) {
           installs = installs.concat(aProviderInstalls);
           aCaller.callNext();
         });
       },
 
       noMoreObjects: function getInstallsByTypes_noMoreObjects(aCaller) {
         safeCall(aCallback, installs);
       }
@@ -1965,18 +2004,18 @@ var AddonManagerInternal = {
                                  Cr.NS_ERROR_INVALID_ARG);
 
     if (typeof aCallback != "function")
       throw Components.Exception("aCallback must be a function",
                                  Cr.NS_ERROR_INVALID_ARG);
 
     new AsyncObjectCaller(this.providers, "getAddonByID", {
       nextObject: function getAddonByID_nextObject(aCaller, aProvider) {
-        callProvider(aProvider, "getAddonByID", null, aID,
-                    function getAddonByID_safeCall(aAddon) {
+        callProviderAsync(aProvider, "getAddonByID", aID,
+                          function getAddonByID_safeCall(aAddon) {
           if (aAddon)
             safeCall(aCallback, aAddon);
           else
             aCaller.callNext();
         });
       },
 
       noMoreObjects: function getAddonByID_noMoreObjects(aCaller) {
@@ -2004,18 +2043,18 @@ var AddonManagerInternal = {
                                  Cr.NS_ERROR_INVALID_ARG);
 
     if (typeof aCallback != "function")
       throw Components.Exception("aCallback must be a function",
                                  Cr.NS_ERROR_INVALID_ARG);
 
     new AsyncObjectCaller(this.providers, "getAddonBySyncGUID", {
       nextObject: function getAddonBySyncGUID_nextObject(aCaller, aProvider) {
-        callProvider(aProvider, "getAddonBySyncGUID", null, aGUID,
-                    function getAddonBySyncGUID_safeCall(aAddon) {
+        callProviderAsync(aProvider, "getAddonBySyncGUID", aGUID,
+                          function getAddonBySyncGUID_safeCall(aAddon) {
           if (aAddon) {
             safeCall(aCallback, aAddon);
           } else {
             aCaller.callNext();
           }
         });
       },
 
@@ -2085,18 +2124,18 @@ var AddonManagerInternal = {
     if (typeof aCallback != "function")
       throw Components.Exception("aCallback must be a function",
                                  Cr.NS_ERROR_INVALID_ARG);
 
     let addons = [];
 
     new AsyncObjectCaller(this.providers, "getAddonsByTypes", {
       nextObject: function getAddonsByTypes_nextObject(aCaller, aProvider) {
-        callProvider(aProvider, "getAddonsByTypes", null, aTypes,
-                     function getAddonsByTypes_concatAddons(aProviderAddons) {
+        callProviderAsync(aProvider, "getAddonsByTypes", aTypes,
+                          function getAddonsByTypes_concatAddons(aProviderAddons) {
           addons = addons.concat(aProviderAddons);
           aCaller.callNext();
         });
       },
 
       noMoreObjects: function getAddonsByTypes_noMoreObjects(aCaller) {
         safeCall(aCallback, addons);
       }
@@ -2145,19 +2184,19 @@ var AddonManagerInternal = {
       throw Components.Exception("aCallback must be a function",
                                  Cr.NS_ERROR_INVALID_ARG);
 
     let addons = [];
 
     new AsyncObjectCaller(this.providers, "getAddonsWithOperationsByTypes", {
       nextObject: function getAddonsWithOperationsByTypes_nextObject
                            (aCaller, aProvider) {
-        callProvider(aProvider, "getAddonsWithOperationsByTypes", null, aTypes,
-                     function getAddonsWithOperationsByTypes_concatAddons
-                              (aProviderAddons) {
+        callProviderAsync(aProvider, "getAddonsWithOperationsByTypes", aTypes,
+                          function getAddonsWithOperationsByTypes_concatAddons
+                                   (aProviderAddons) {
           addons = addons.concat(aProviderAddons);
           aCaller.callNext();
         });
       },
 
       noMoreObjects: function getAddonsWithOperationsByTypes_noMoreObjects(caller) {
         safeCall(aCallback, addons);
       }
--- a/toolkit/mozapps/extensions/test/xpcshell/test_provider_shutdown.js
+++ b/toolkit/mozapps/extensions/test/xpcshell/test_provider_shutdown.js
@@ -22,17 +22,17 @@ function mockAddonProvider(aName) {
 
     shutdown: function() {
       this.shutdownResolve();
       return this.donePromise;
     },
   };
   mockProvider.donePromise = new Promise((resolve, reject) => {
     mockProvider.doneResolve = resolve;
-    mockProvider.doneResject = reject;
+    mockProvider.doneReject = reject;
   });
   mockProvider.shutdownPromise = new Promise((resolve, reject) => {
     mockProvider.shutdownResolve = resolve;
   });
   return mockProvider;
 };
 
 function run_test() {
@@ -76,17 +76,16 @@ add_task(function* blockRepoShutdown() {
   equal(status[1].state, "pending");
   // let the provider finish
   mockProvider.doneResolve();
 
   // Wait for manager to call repo shutdown and start waiting for it
   yield mockRepo.shutdownPromise;
   // Check the shutdown state
   status = MockAsyncShutdown.status();
-  do_print(JSON.stringify(status));
   equal(status[0].name, "AddonManager: Waiting for providers to shut down.");
   equal(status[0].state, "Complete");
   equal(status[1].name, "AddonRepository: async shutdown");
   equal(status[1].state, "in progress");
 
   // Now finish our shutdown, and wait for the manager to wrap up
   mockRepo.doneResolve();
   yield managerDown;