Bug 1075625 - AddonManager can race between provider startup / shutdown and methods that call back to providers. r=Mossop

toolkit/mozapps/extensions/AddonManager.jsm

@@ -286,7 +286,7 @@ function getLocale() {
  *         just the AsyncObjectCaller
  */
 function AsyncObjectCaller(aObjects, aMethod, aListener) {
-  this.objects = aObjects.slice(0);
+  this.objects = [...aObjects];
   this.method = aMethod;
   this.listener = aListener;
 
@@ -518,7 +518,8 @@ var AddonManagerInternal = {
   installListeners: [],
   addonListeners: [],
   typeListeners: [],
-  providers: [],
+  pendingProviders: new Set(),
+  providers: new Set(),
   providerShutdowns: new Map(),
   types: {},
   startupChanges: {},
@@ -666,10 +667,20 @@ var AddonManagerInternal = {
       throw Components.Exception("AddonManager is not initialized",
                                  Cr.NS_ERROR_NOT_INITIALIZED);
 
+    logger.debug(`Starting provider: ${providerName(aProvider)}`);
     callProvider(aProvider, "startup", null, aAppChanged, aOldAppVersion, aOldPlatformVersion);
     if ('shutdown' in aProvider) {
       let name = providerName(aProvider);
       let AMProviderShutdown = () => {
+        // If the provider has been unregistered, it will have been removed from
+        // this.providers. If it hasn't been unregistered, then this is a normal
+        // shutdown - and we move it to this.pendingProviders incase we're
+        // running in a test that will start AddonManager again.
+        if (this.providers.has(aProvider)) {
+          this.providers.delete(aProvider);
+          this.pendingProviders.add(aProvider);
+        }
+
         return new Promise((resolve, reject) => {
             logger.debug("Calling shutdown blocker for " + name);
             resolve(aProvider.shutdown());
@@ -683,6 +694,10 @@ var AddonManagerInternal = {
       this.providerShutdowns.set(aProvider, AMProviderShutdown);
       AddonManager.shutdown.addBlocker(name, AMProviderShutdown);
     }
+
+    this.pendingProviders.delete(aProvider);
+    this.providers.add(aProvider);
+    logger.debug(`Provider finished startup: ${providerName(aProvider)}`);
   },
 
   /**
@@ -805,6 +820,7 @@ var AddonManagerInternal = {
 
         try {
           Components.utils.import(url, {});
+          logger.debug(`Loaded provider scope for ${url}`);
         }
         catch (e) {
           AddonManagerPrivate.recordException("AMI", "provider " + url + " load failed", e);
@@ -822,7 +838,7 @@ var AddonManagerInternal = {
       // Once we start calling providers we must allow all normal methods to work.
       gStarted = true;
 
-      for (let provider of this.providers) {
+      for (let provider of this.pendingProviders) {
         this._startProvider(provider, appChanged, oldAppVersion, oldPlatformVersion);
       }
 
@@ -839,6 +855,9 @@ var AddonManagerInternal = {
       logger.error("startup failed", e);
       AddonManagerPrivate.recordException("AMI", "startup failed", e);
     }
+
+    logger.debug("Completed startup sequence");
+    this.callManagerListeners("onStartup");
   },
 
   /**
@@ -858,7 +877,7 @@ var AddonManagerInternal = {
       throw Components.Exception("aTypes must be an array or null",
                                  Cr.NS_ERROR_INVALID_ARG);
 
-    this.providers.push(aProvider);
+    this.pendingProviders.add(aProvider);
 
     if (aTypes) {
       aTypes.forEach(function(aType) {
@@ -906,13 +925,11 @@ var AddonManagerInternal = {
       throw Components.Exception("aProvider must be specified",
                                  Cr.NS_ERROR_INVALID_ARG);
 
-    let pos = 0;
-    while (pos < this.providers.length) {
-      if (this.providers[pos] == aProvider)
-        this.providers.splice(pos, 1);
-      else
-        pos++;
-    }
+    this.providers.delete(aProvider);
+    // The test harness will unregister XPIProvider *after* shutdown, which is
+    // after the provider will have been moved from providers to
+    // pendingProviders.
+    this.pendingProviders.delete(aProvider);
 
     for (let type in this.types) {
       this.types[type].providers = this.types[type].providers.filter(function filterProvider(p) p != aProvider);
@@ -944,6 +961,39 @@ var AddonManagerInternal = {
     return undefined;
   },
 
+  /**
+   * Mark a provider as safe to access via AddonManager APIs, before its
+   * startup has completed.
+   *
+   * Normally a provider isn't marked as safe until after its (synchronous)
+   * startup() method has returned. Until a provider has been marked safe,
+   * it won't be used by any of the AddonManager APIs. markProviderSafe()
+   * allows a provider to mark itself as safe during its startup; this can be
+   * useful if the provider wants to perform tasks that block startup, which
+   * happen after its required initialization tasks and therefore when the
+   * provider is in a safe state.
+   *
+   * @param aProvider Provider object to mark safe
+   */
+  markProviderSafe: function AMI_markProviderSafe(aProvider) {
+    if (!gStarted) {
+      throw Components.Exception("AddonManager is not initialized",
+                                 Cr.NS_ERROR_NOT_INITIALIZED);
+    }
+
+    if (!aProvider || typeof aProvider != "object") {
+      throw Components.Exception("aProvider must be specified",
+                                 Cr.NS_ERROR_INVALID_ARG);
+    }
+
+    if (!this.pendingProviders.has(aProvider)) {
+      return;
+    }
+
+    this.pendingProviders.delete(aProvider);
+    this.providers.add(aProvider);
+  },
+
   /**
    * Calls a method on all registered providers if it exists and consumes any
    * thrown exception. Return values are ignored. Any parameters after the
@@ -960,7 +1010,7 @@ var AddonManagerInternal = {
       throw Components.Exception("aMethod must be a non-empty string",
                                  Cr.NS_ERROR_INVALID_ARG);
 
-    let providers = this.providers.slice(0);
+    let providers = [...this.providers];
     for (let provider of providers) {
       try {
         if (aMethod in provider)
@@ -998,6 +1048,8 @@ var AddonManagerInternal = {
    */
   shutdownManager: Task.async(function* () {
     logger.debug("shutdown");
+    this.callManagerListeners("onShutdown");
+
     gRepoShutdownState = "pending";
     gShutdownInProgress = true;
     // Clean up listeners
@@ -1560,7 +1612,23 @@ var AddonManagerInternal = {
       throw Components.Exception("aType must be a non-empty string",
                                  Cr.NS_ERROR_INVALID_ARG);
 
-    this.callProviders("addonChanged", aID, aType, aPendingRestart);
+    // Temporary hack until bug 520124 lands.
+    // We can get here during synchronous startup, at which point it's
+    // considered unsafe (and therefore disallowed by AddonManager.jsm) to
+    // access providers that haven't been initialized yet. Since this is when
+    // XPIProvider is starting up, XPIProvider can't access itself via APIs
+    // going through AddonManager.jsm. Furthermore, LightweightThemeManager may
+    // not be initialized until after XPIProvider is, and therefore would also
+    // be unaccessible during XPIProvider startup. Thankfully, these are the
+    // only two uses of this API, and we know it's safe to use this API with
+    // both providers; so we have this hack to allow bypassing the normal
+    // safetey guard.
+    // The notifyAddonChanged/addonChanged API will be unneeded and therefore
+    // removed by bug 520124, so this is a temporary quick'n'dirty hack.
+    let providers = [...this.providers, ...this.pendingProviders];
+    for (let provider of providers) {
+      callProvider(provider, "addonChanged", null, aID, aType, aPendingRestart);
+    }
   },
 
   /**
@@ -1671,7 +1739,7 @@ var AddonManagerInternal = {
       throw Components.Exception("aBrowser must be a nsIDOMElement or null",
                                  Cr.NS_ERROR_INVALID_ARG);
 
-    let providers = this.providers.slice(0);
+    let providers = [...this.providers];
     for (let provider of providers) {
       if (callProvider(provider, "supportsMimetype", false, aMimetype)) {
         callProviderAsync(provider, "getInstallForURL",
@@ -1803,8 +1871,9 @@ var AddonManagerInternal = {
       throw Components.Exception("aURI is not a nsIURI",
                                  Cr.NS_ERROR_INVALID_ARG);
     }
+
     // Try all providers
-    let providers = this.providers.slice(0);
+    let providers = [...this.providers];
     for (let provider of providers) {
       var id = callProvider(provider, "mapURIToAddonID", null, aURI);
       if (id !== null) {
@@ -1831,7 +1900,7 @@ var AddonManagerInternal = {
       throw Components.Exception("aMimetype must be a non-empty string",
                                  Cr.NS_ERROR_INVALID_ARG);
 
-    let providers = this.providers.slice(0);
+    let providers = [...this.providers];
     for (let provider of providers) {
       if (callProvider(provider, "supportsMimetype", false, aMimetype) &&
           callProvider(provider, "isInstallEnabled"))
@@ -1863,7 +1932,7 @@ var AddonManagerInternal = {
       throw Components.Exception("aURI must be a nsIURI or null",
                                  Cr.NS_ERROR_INVALID_ARG);
 
-    let providers = this.providers.slice(0);
+    let providers = [...this.providers];
     for (let provider of providers) {
       if (callProvider(provider, "supportsMimetype", false, aMimetype) &&
           callProvider(provider, "isInstallAllowed", null, aURI))
@@ -2412,6 +2481,10 @@ this.AddonManagerPrivate = {
     AddonManagerInternal.unregisterProvider(aProvider);
   },
 
+  markProviderSafe: function AMP_markProviderSafe(aProvider) {
+    AddonManagerInternal.markProviderSafe(aProvider);
+  },
+
   backgroundUpdateCheck: function AMP_backgroundUpdateCheck() {
     return AddonManagerInternal.backgroundUpdateCheck();
   },
@@ -2718,6 +2791,10 @@ this.AddonManager = {
   },
 #endif
 
+  get isReady() {
+    return gStartupComplete && !gShutdownInProgress;
+  },
+
   getInstallForURL: function AM_getInstallForURL(aUrl, aCallback, aMimetype,
                                                  aHash, aName, aIcons,
                                                  aVersion, aBrowser) {

toolkit/mozapps/extensions/internal/XPIProvider.jsm

@@ -2094,6 +2094,8 @@ this.XPIProvider = {
       // Changes to installed extensions may have changed which theme is selected
       this.applyThemeChange();
 
+      AddonManagerPrivate.markProviderSafe(this);
+
       if (aAppChanged === undefined) {
         // For new profiles we will never need to show the add-on selection UI
         Services.prefs.setBoolPref(PREF_SHOWN_SELECTION_UI, true);

toolkit/mozapps/extensions/test/xpcshell/head_addons.js

@@ -911,6 +911,7 @@ function getExpectedInstall(aAddon) {
 
 const AddonListener = {
   onPropertyChanged: function(aAddon, aProperties) {
+    do_print(`Got onPropertyChanged event for ${aAddon.id}`);
     let [event, properties] = getExpectedEvent(aAddon.id);
     do_check_eq("onPropertyChanged", event);
     do_check_eq(aProperties.length, properties.length);
@@ -924,6 +925,7 @@ const AddonListener = {
   },
 
   onEnabling: function(aAddon, aRequiresRestart) {
+    do_print(`Got onEnabling event for ${aAddon.id}`);
     let [event, expectedRestart] = getExpectedEvent(aAddon.id);
     do_check_eq("onEnabling", event);
     do_check_eq(aRequiresRestart, expectedRestart);
@@ -934,6 +936,7 @@ const AddonListener = {
   },
 
   onEnabled: function(aAddon) {
+    do_print(`Got onEnabled event for ${aAddon.id}`);
     let [event, expectedRestart] = getExpectedEvent(aAddon.id);
     do_check_eq("onEnabled", event);
     do_check_false(hasFlag(aAddon.permissions, AddonManager.PERM_CAN_ENABLE));
@@ -941,6 +944,7 @@ const AddonListener = {
   },
 
   onDisabling: function(aAddon, aRequiresRestart) {
+    do_print(`Got onDisabling event for ${aAddon.id}`);
     let [event, expectedRestart] = getExpectedEvent(aAddon.id);
     do_check_eq("onDisabling", event);
     do_check_eq(aRequiresRestart, expectedRestart);
@@ -951,6 +955,7 @@ const AddonListener = {
   },
 
   onDisabled: function(aAddon) {
+    do_print(`Got onDisabled event for ${aAddon.id}`);
     let [event, expectedRestart] = getExpectedEvent(aAddon.id);
     do_check_eq("onDisabled", event);
     do_check_false(hasFlag(aAddon.permissions, AddonManager.PERM_CAN_DISABLE));
@@ -958,6 +963,7 @@ const AddonListener = {
   },
 
   onInstalling: function(aAddon, aRequiresRestart) {
+    do_print(`Got onInstalling event for ${aAddon.id}`);
     let [event, expectedRestart] = getExpectedEvent(aAddon.id);
     do_check_eq("onInstalling", event);
     do_check_eq(aRequiresRestart, expectedRestart);
@@ -967,12 +973,14 @@ const AddonListener = {
   },
 
   onInstalled: function(aAddon) {
+    do_print(`Got onInstalled event for ${aAddon.id}`);
     let [event, expectedRestart] = getExpectedEvent(aAddon.id);
     do_check_eq("onInstalled", event);
     return check_test_completed(arguments);
   },
 
   onUninstalling: function(aAddon, aRequiresRestart) {
+    do_print(`Got onUninstalling event for ${aAddon.id}`);
     let [event, expectedRestart] = getExpectedEvent(aAddon.id);
     do_check_eq("onUninstalling", event);
     do_check_eq(aRequiresRestart, expectedRestart);
@@ -982,12 +990,14 @@ const AddonListener = {
   },
 
   onUninstalled: function(aAddon) {
+    do_print(`Got onUninstalled event for ${aAddon.id}`);
     let [event, expectedRestart] = getExpectedEvent(aAddon.id);
     do_check_eq("onUninstalled", event);
     return check_test_completed(arguments);
   },
 
   onOperationCancelled: function(aAddon) {
+    do_print(`Got onOperationCancelled event for ${aAddon.id}`);
     let [event, expectedRestart] = getExpectedEvent(aAddon.id);
     do_check_eq("onOperationCancelled", event);
     return check_test_completed(arguments);

toolkit/mozapps/extensions/test/xpcshell/test_isReady.js

@@ -0,0 +1,49 @@
+createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1.9.2");
+
+function run_test() {
+  run_next_test();
+}
+
+add_task(function* () {
+  equal(AddonManager.isReady, false, "isReady should be false before startup");
+
+  let gotStartupEvent = false;
+  let gotShutdownEvent = false;
+  let listener = {
+    onStartup() {
+      gotStartupEvent = true;
+    },
+    onShutdown() {
+      gotShutdownEvent = true;
+    },
+  };
+  AddonManager.addManagerListener(listener);
+
+  do_print("Starting manager...");
+  startupManager();
+  equal(AddonManager.isReady, true, "isReady should be true after startup");
+  equal(gotStartupEvent, true, "Should have seen onStartup event after startup");
+  equal(gotShutdownEvent, false, "Should not have seen onShutdown event before shutdown");
+
+  gotStartupEvent = false;
+  gotShutdownEvent = false;
+
+  do_print("Shutting down manager...");
+  let shutdownPromise = promiseShutdownManager();
+  equal(AddonManager.isReady, false, "isReady should be false when shutdown commences");
+  yield shutdownPromise;
+
+  equal(AddonManager.isReady, false, "isReady should be false after shutdown");
+  equal(gotStartupEvent, false, "Should not have seen onStartup event after shutdown");
+  equal(gotShutdownEvent, true, "Should have seen onShutdown event after shutdown");
+
+  AddonManager.addManagerListener(listener);
+  gotStartupEvent = false;
+  gotShutdownEvent = false;
+
+  do_print("Starting manager again...");
+  startupManager();
+  equal(AddonManager.isReady, true, "isReady should be true after repeat startup");
+  equal(gotStartupEvent, true, "Should have seen onStartup event after repeat startup");
+  equal(gotShutdownEvent, false, "Should not have seen onShutdown event before shutdown, following repeat startup");
+});

toolkit/mozapps/extensions/test/xpcshell/test_provider_markSafe.js

@@ -0,0 +1,47 @@
+createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1.9.2");
+
+let startupOrder = [];
+
+function mockAddonProvider(name) {
+  let mockProvider = {
+    markSafe: false,
+    apiAccessed: false,
+
+    startup() {
+      if (this.markSafe)
+        AddonManagerPrivate.markProviderSafe(this);
+
+      let uri = Services.io.newURI("beard://long", null, null);
+      AddonManager.isInstallEnabled("made-up-mimetype");
+    },
+    supportsMimetype(mimetype) {
+      this.apiAccessed = true;
+      return false;
+    },
+
+    get name() name,
+  };
+
+  return mockProvider;
+};
+
+function run_test() {
+  run_next_test();
+}
+
+add_task(function* testMarkSafe() {
+  do_print("Starting with provider normally");
+  let provider = mockAddonProvider("Mock1");
+  AddonManagerPrivate.registerProvider(provider);
+  startupManager();
+  ok(!provider.apiAccessed, "Provider API should not have been accessed");
+  AddonManagerPrivate.unregisterProvider(provider);
+  yield promiseShutdownManager();
+
+  do_print("Starting with provider that marks itself safe");
+  provider.apiAccessed = false;
+  provider.markSafe = true;
+  AddonManagerPrivate.registerProvider(provider);
+  startupManager();
+  ok(provider.apiAccessed, "Provider API should have been accessed");
+});

toolkit/mozapps/extensions/test/xpcshell/test_provider_unsafe_access_shutdown.js

@@ -0,0 +1,61 @@
+createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1.9.2");
+
+let shutdownOrder = [];
+
+function mockAddonProvider(name) {
+  let mockProvider = {
+    hasShutdown: false,
+    unsafeAccess: false,
+
+    shutdownCallback: null,
+
+    startup() { },
+    shutdown() {
+      this.hasShutdown = true;
+      shutdownOrder.push(this.name);
+      if (this.shutdownCallback)
+        return this.shutdownCallback();
+    },
+    getAddonByID(id, callback) {
+      if (this.hasShutdown) {
+        unsafeAccess = true;
+      }
+      callback(null);
+    },
+
+    get name() name,
+  };
+
+  return mockProvider;
+};
+
+function run_test() {
+  run_next_test();
+}
+
+add_task(function* unsafeProviderShutdown() {
+  let firstProvider = mockAddonProvider("Mock1");
+  AddonManagerPrivate.registerProvider(firstProvider);
+  let secondProvider = mockAddonProvider("Mock2");
+  AddonManagerPrivate.registerProvider(secondProvider);
+
+  startupManager();
+
+  let shutdownPromise = null;
+  yield new Promise(resolve => {
+    secondProvider.shutdownCallback = function() {
+      return new Promise(shutdownResolve => {
+        AddonManager.getAddonByID("does-not-exist", () => {
+          shutdownResolve();
+          resolve();
+        });
+      });
+    };
+
+    shutdownPromise = promiseShutdownManager();
+  });
+  yield shutdownPromise;
+
+  equal(shutdownOrder.join(","), ["Mock1", "Mock2"].join(","), "Mock providers should have shutdown in expected order");
+  ok(!firstProvider.unsafeAccess, "First registered mock provider should not have been accessed unsafely");
+});

toolkit/mozapps/extensions/test/xpcshell/test_provider_unsafe_access_startup.js

@@ -0,0 +1,53 @@
+createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1.9.2");
+
+let startupOrder = [];
+
+function mockAddonProvider(name) {
+  let mockProvider = {
+    hasStarted: false,
+    unsafeAccess: false,
+
+    startupCallback: null,
+
+    startup() {
+      this.hasStarted = true;
+      startupOrder.push(this.name);
+      if (this.startupCallback)
+        this.startupCallback();
+    },
+    getAddonByID(id, callback) {
+      if (!this.hasStarted) {
+        unsafeAccess = true;
+      }
+      callback(null);
+    },
+
+    get name() name,
+  };
+
+  return mockProvider;
+};
+
+function run_test() {
+  run_next_test();
+}
+
+add_task(function* unsafeProviderStartup() {
+  let secondProvider = null;
+
+  yield new Promise(resolve => {
+    let firstProvider = mockAddonProvider("Mock1");
+    firstProvider.startupCallback = function() {
+      AddonManager.getAddonByID("does-not-exist", resolve);
+    };
+    AddonManagerPrivate.registerProvider(firstProvider);
+
+    secondProvider = mockAddonProvider("Mock2");
+    AddonManagerPrivate.registerProvider(secondProvider);
+
+    startupManager();
+  });
+
+  equal(startupOrder.join(","), ["Mock1", "Mock2"].join(","), "Mock providers should have hasStarted in expected order");
+  ok(!secondProvider.unsafeAccess, "Second registered mock provider should not have been accessed unsafely");
+});

toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini

@@ -11,11 +11,15 @@ support-files =
 [test_addon_path_service.js]
 [test_asyncBlocklistLoad.js]
 [test_DeferredSave.js]
+[test_isReady.js]
 [test_metadata_update.js]
 [test_openh264.js]
 run-if = appname == "firefox"
 [test_pluginInfoURL.js]
+[test_provider_markSafe.js]
 [test_provider_shutdown.js]
+[test_provider_unsafe_access_shutdown.js]
+[test_provider_unsafe_access_startup.js]
 [test_shutdown.js]
 [test_XPIcancel.js]
 [test_XPIStates.js]