Bug 757663 - AddonManagerInternal.callAddonlListeners() informs it's listeners unsafely, causing silent skips in the notification loop. r=Mossop,darktrojan

toolkit/mozapps/extensions/AddonManager.jsm

@@ -535,10 +535,8 @@ var AddonManagerInternal = {
       }
     }
 
-    this.providers.forEach(function(provider) {
-      callProvider(provider, "startup", null, appChanged, oldAppVersion,
-                   oldPlatformVersion);
-    });
+    this.callProviders("startup", appChanged, oldAppVersion,
+                       oldPlatformVersion);
 
     // If this is a new profile just pretend that there were no changes
     if (appChanged === undefined) {
@@ -581,11 +579,12 @@ var AddonManagerInternal = {
             providers: [aProvider]
           };
 
-          this.typeListeners.forEach(function(aListener) {
+          let typeListeners = this.typeListeners.slice(0);
+          for (let listener of typeListeners) {
             safeCall(function() {
-              aListener.onTypeAdded(aType);
+              listener.onTypeAdded(aType);
             });
-          });
+          }
         }
         else {
           this.types[aType.id].providers.push(aProvider);
@@ -623,11 +622,12 @@ var AddonManagerInternal = {
         let oldType = this.types[type].type;
         delete this.types[type];
 
-        this.typeListeners.forEach(function(aListener) {
+        let typeListeners = this.typeListeners.slice(0);
+        for (let listener of typeListeners) {
           safeCall(function() {
-            aListener.onTypeRemoved(oldType);
+            listener.onTypeRemoved(oldType);
           });
-        });
+        }
       }
     }
 
@@ -636,6 +636,32 @@ var AddonManagerInternal = {
       callProvider(aProvider, "shutdown");
   },
 
+  /**
+   * 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.
+   *
+   * @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",
+                                 Cr.NS_ERROR_INVALID_ARG);
+
+    let providers = this.providers.slice(0);
+    for (let provider of providers) {
+      try {
+        if (aMethod in provider)
+          provider[aMethod].apply(provider, aArgs);
+      }
+      catch (e) {
+        ERROR("Exception calling provider " + aMethod, e);
+      }
+    }
+  },
+
   /**
    * Shuts down the addon manager and all registered providers, this must clean
    * up everything in order for automated tests to fake restarts.
@@ -648,9 +674,7 @@ var AddonManagerInternal = {
     Services.prefs.removeObserver(PREF_EM_AUTOUPDATE_DEFAULT, this);
     Services.prefs.removeObserver(PREF_EM_HOTFIX_ID, this);
 
-    this.providers.forEach(function(provider) {
-      callProvider(provider, "shutdown");
-    });
+    this.callProviders("shutdown");
 
     this.managerListeners.splice(0, this.managerListeners.length);
     this.installListeners.splice(0, this.installListeners.length);
@@ -1055,12 +1079,13 @@ var AddonManagerInternal = {
    *         The method on the listeners to call
    */
   callManagerListeners: function AMI_callManagerListeners(aMethod) {
-     if (!aMethod || typeof aMethod != "string")
+    if (!aMethod || typeof aMethod != "string")
       throw Components.Exception("aMethod must be a non-empty string",
                                  Cr.NS_ERROR_INVALID_ARG);
 
     var args = Array.slice(arguments, 1);
-    this.managerListeners.forEach(function(listener) {
+    let managerListeners = this.managerListeners.slice(0);
+    for (let listener of managerListeners) {
       try {
         if (aMethod in listener)
           listener[aMethod].apply(listener, args);
@@ -1068,7 +1093,7 @@ var AddonManagerInternal = {
       catch (e) {
         WARN("AddonManagerListener threw exception when calling " + aMethod, e);
       }
-    });
+    }
   },
 
   /**
@@ -1082,7 +1107,7 @@ var AddonManagerInternal = {
    * @return false if any of the listeners returned false, true otherwise
    */
   callInstallListeners: function AMI_callInstallListeners(aMethod, aExtraListeners) {
-     if (!aMethod || typeof aMethod != "string")
+    if (!aMethod || typeof aMethod != "string")
       throw Components.Exception("aMethod must be a non-empty string",
                                  Cr.NS_ERROR_INVALID_ARG);
 
@@ -1091,12 +1116,14 @@ var AddonManagerInternal = {
                                  Cr.NS_ERROR_INVALID_ARG);
 
     let result = true;
-    let listeners = this.installListeners;
+    let listeners;
     if (aExtraListeners)
-      listeners = aExtraListeners.concat(listeners);
+      listeners = aExtraListeners.concat(this.installListeners);
+    else
+      listeners = this.installListeners.slice(0);
     let args = Array.slice(arguments, 2);
 
-    listeners.forEach(function(listener) {
+    for (let listener of listeners) {
       try {
         if (aMethod in listener) {
           if (listener[aMethod].apply(listener, args) === false)
@@ -1106,7 +1133,7 @@ var AddonManagerInternal = {
       catch (e) {
         WARN("InstallListener threw exception when calling " + aMethod, e);
       }
-    });
+    }
     return result;
   },
 
@@ -1118,12 +1145,13 @@ var AddonManagerInternal = {
    *         The method on the listeners to call
    */
   callAddonListeners: function AMI_callAddonListeners(aMethod) {
-     if (!aMethod || typeof aMethod != "string")
+    if (!aMethod || typeof aMethod != "string")
       throw Components.Exception("aMethod must be a non-empty string",
                                  Cr.NS_ERROR_INVALID_ARG);
 
     var args = Array.slice(arguments, 1);
-    this.addonListeners.forEach(function(listener) {
+    let addonListeners = this.addonListeners.slice(0);
+    for (let listener of addonListeners) {
       try {
         if (aMethod in listener)
           listener[aMethod].apply(listener, args);
@@ -1131,7 +1159,7 @@ var AddonManagerInternal = {
       catch (e) {
         WARN("AddonListener threw exception when calling " + aMethod, e);
       }
-    });
+    }
   },
 
   /**
@@ -1156,9 +1184,7 @@ var AddonManagerInternal = {
       throw Components.Exception("aType must be a non-empty string",
                                  Cr.NS_ERROR_INVALID_ARG);
 
-    this.providers.forEach(function(provider) {
-      callProvider(provider, "addonChanged", null, aID, aType, aPendingRestart);
-    });
+    this.callProviders("addonChanged", aID, aType, aPendingRestart);
   },
 
   /**
@@ -1167,9 +1193,7 @@ var AddonManagerInternal = {
    * update.
    */
   updateAddonAppDisabledStates: function AMI_updateAddonAppDisabledStates() {
-    this.providers.forEach(function(provider) {
-      callProvider(provider, "updateAddonAppDisabledStates");
-    });
+    this.callProviders("updateAddonAppDisabledStates");
   },
   
   /**
@@ -1253,7 +1277,8 @@ var AddonManagerInternal = {
       throw Components.Exception("aLoadGroup must be a nsILoadGroup or null",
                                  Cr.NS_ERROR_INVALID_ARG);
 
-    for (let provider of this.providers) {
+    let providers = this.providers.slice(0);
+    for (let provider of providers) {
       if (callProvider(provider, "supportsMimetype", false, aMimetype)) {
         callProvider(provider, "getInstallForURL", null,
                      aUrl, aHash, aName, aIconURL, aVersion, aLoadGroup,
@@ -1365,7 +1390,8 @@ var AddonManagerInternal = {
       throw Components.Exception("aMimetype must be a non-empty string",
                                  Cr.NS_ERROR_INVALID_ARG);
 
-    for (let provider of this.providers) {
+    let providers = this.providers.slice(0);
+    for (let provider of providers) {
       if (callProvider(provider, "supportsMimetype", false, aMimetype) &&
           callProvider(provider, "isInstallEnabled"))
         return true;
@@ -1392,7 +1418,8 @@ var AddonManagerInternal = {
       throw Components.Exception("aURI must be a nsIURI or null",
                                  Cr.NS_ERROR_INVALID_ARG);
 
-    for (let provider of this.providers) {
+    let providers = this.providers.slice(0);
+    for (let provider of providers) {
       if (callProvider(provider, "supportsMimetype", false, aMimetype) &&
           callProvider(provider, "isInstallAllowed", null, aURI))
         return true;

toolkit/mozapps/extensions/test/addons/test_bug757663/install.rdf

@@ -0,0 +1,24 @@
+<?xml version="1.0"?>
+
+<RDF xmlns="http://www.w3.org/1999/02/22-rdf-syntax-ns#"
+     xmlns:em="http://www.mozilla.org/2004/em-rdf#">
+
+  <Description about="urn:mozilla:install-manifest">
+    <em:id>bug757663@tests.mozilla.org</em:id>
+    <em:version>1.0</em:version>
+    <em:bootstrap>true</em:bootstrap>
+
+    <!-- Front End MetaData -->
+    <em:name>Test Bootstrap 1</em:name>
+    <em:description>Test Description</em:description>
+
+    <em:targetApplication>
+      <Description>
+        <em:id>xpcshell@tests.mozilla.org</em:id>
+        <em:minVersion>1</em:minVersion>
+        <em:maxVersion>1</em:maxVersion>
+      </Description>
+    </em:targetApplication>
+
+  </Description>
+</RDF>

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

@@ -0,0 +1,112 @@
+/* Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/publicdomain/zero/1.0/
+ */
+
+// This test verifies that removing a listener during a callback for that type
+// of listener still results in all listeners being called.
+
+var addon1 = {
+  id: "addon1@tests.mozilla.org",
+  version: "2.0",
+  name: "Test 1",
+  bootstrap: "true",
+  targetApplications: [{
+    id: "xpcshell@tests.mozilla.org",
+    minVersion: "1",
+    maxVersion: "1"
+  }]
+};
+
+var listener1 = {
+  sawEvent: false,
+  onDisabling: function() {
+    this.sawEvent = true;
+    AddonManager.removeAddonListener(this);
+  },
+  onNewInstall: function() {
+    this.sawEvent = true;
+    AddonManager.removeInstallListener(this);
+  }
+};
+var listener2 = {
+  sawEvent: false,
+  onDisabling: function() {
+    this.sawEvent = true;
+  },
+  onNewInstall: function() {
+    this.sawEvent = true;
+  }
+};
+var listener3 = {
+  sawEvent: false,
+  onDisabling: function() {
+    this.sawEvent = true;
+  },
+  onNewInstall: function() {
+    this.sawEvent = true;
+  }
+};
+
+const profileDir = gProfD.clone();
+profileDir.append("extensions");
+
+
+function run_test() {
+  do_test_pending();
+  createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "1", "1.9.2");
+
+  writeInstallRDFForExtension(addon1, profileDir);
+  startupManager();
+
+  run_test_1();
+}
+
+function run_test_1() {
+  AddonManager.addAddonListener(listener1);
+  AddonManager.addAddonListener(listener2);
+  AddonManager.addAddonListener(listener3);
+
+  AddonManager.getAddonsByIDs(["addon1@tests.mozilla.org"], function([a1]) {
+    do_check_neq(a1, null);
+    do_check_false(a1.userDisabled);
+    do_check_true(a1.isActive);
+
+    a1.userDisabled = true;
+
+    do_check_true(listener1.sawEvent);
+    listener1.sawEvent = false;
+    do_check_true(listener2.sawEvent);
+    listener2.sawEvent = false;
+    do_check_true(listener3.sawEvent);
+    listener3.sawEvent = false;
+
+    AddonManager.removeAddonListener(listener1);
+    AddonManager.removeAddonListener(listener2);
+    AddonManager.removeAddonListener(listener3);
+
+    a1.uninstall();
+    run_test_2();
+  });
+}
+
+function run_test_2() {
+  AddonManager.addInstallListener(listener1);
+  AddonManager.addInstallListener(listener2);
+  AddonManager.addInstallListener(listener3);
+
+  AddonManager.getInstallForFile(do_get_addon("test_bug757663"), function(aInstall) {
+
+    do_check_true(listener1.sawEvent);
+    listener1.sawEvent = false;
+    do_check_true(listener2.sawEvent);
+    listener2.sawEvent = false;
+    do_check_true(listener3.sawEvent);
+    listener3.sawEvent = false;
+
+    AddonManager.removeInstallListener(listener1);
+    AddonManager.removeInstallListener(listener2);
+    AddonManager.removeInstallListener(listener3);
+
+    do_test_finished();
+  });
+}

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

@@ -129,6 +129,7 @@ fail-if = os == "android"
 [test_bug659772.js]
 [test_bug675371.js]
 [test_bug740612.js]
+[test_bug757663.js]
 [test_cacheflush.js]
 [test_checkcompatibility.js]
 [test_ChromeManifestParser.js]