Bug 757663 - AddonManagerInternal.callAddonlListeners() informs it's listeners unsafely, causing silent skips in the notification loop. r=Mossop,darktrojan
authorBlair McBride <bmcbride@mozilla.com>
Fri, 01 Jun 2012 18:12:26 +1200
changeset 97654 8cb94b792009f96a0fc74b6e228418559217920a
parent 97653 96a7cf29e58881f9a53f869d8e3e3f76e4a9c081
child 97655 66f127caa6fae9e3983f29680f6d64d85ca7cf28
push idunknown
push userunknown
push dateunknown
reviewersMossop, darktrojan
bugs757663
milestone15.0a1
Bug 757663 - AddonManagerInternal.callAddonlListeners() informs it's listeners unsafely, causing silent skips in the notification loop. r=Mossop,darktrojan
toolkit/mozapps/extensions/AddonManager.jsm
toolkit/mozapps/extensions/test/addons/test_bug757663/install.rdf
toolkit/mozapps/extensions/test/xpcshell/test_bug757663.js
toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini
--- a/toolkit/mozapps/extensions/AddonManager.jsm
+++ b/toolkit/mozapps/extensions/AddonManager.jsm
@@ -530,20 +530,18 @@ var AddonManagerInternal = {
         Components.utils.import(url, {});
       }
       catch (e) {
         ERROR("Exception loading provider " + entry + " from category \"" +
               url + "\"", e);
       }
     }
 
-    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) {
       for (let type in this.startupChanges)
         delete this.startupChanges[type];
     }
 
     gStarted = true;
@@ -576,21 +574,22 @@ var AddonManagerInternal = {
             return;
           }
 
           this.types[aType.id] = {
             type: aType,
             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);
         }
       }, this);
     }
 
     // If we're registering after startup call this provider's startup.
@@ -618,44 +617,69 @@ var AddonManagerInternal = {
     }
 
     for (let type in this.types) {
       this.types[type].providers = this.types[type].providers.filter(function(p) p != aProvider);
       if (this.types[type].providers.length == 0) {
         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);
           });
-        });
+        }
       }
     }
 
     // If we're unregistering after startup call this provider's shutdown.
     if (gStarted)
       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.
    */
   shutdown: function AMI_shutdown() {
     Services.prefs.removeObserver(PREF_EM_CHECK_COMPATIBILITY, this);
     Services.prefs.removeObserver(PREF_EM_STRICT_COMPATIBILITY, this);
     Services.prefs.removeObserver(PREF_EM_CHECK_UPDATE_SECURITY, this);
     Services.prefs.removeObserver(PREF_EM_UPDATE_ENABLED, this);
     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);
     this.addonListeners.splice(0, this.addonListeners.length);
     this.typeListeners.splice(0, this.typeListeners.length);
     for (let type in this.startupChanges)
       delete this.startupChanges[type];
     gStarted = false;
@@ -1050,93 +1074,97 @@ var AddonManagerInternal = {
   /**
    * Calls all registered AddonManagerListeners with an event. Any parameters
    * after the method parameter are passed to the listener.
    *
    * @param  aMethod
    *         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);
       }
       catch (e) {
         WARN("AddonManagerListener threw exception when calling " + aMethod, e);
       }
-    });
+    }
   },
 
   /**
    * Calls all registered InstallListeners with an event. Any parameters after
    * the extraListeners parameter are passed to the listener.
    *
    * @param  aMethod
    *         The method on the listeners to call
    * @param  aExtraListeners
    *         An optional array of extra InstallListeners to also call
    * @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);
 
     if (aExtraListeners && !Array.isArray(aExtraListeners))
       throw Components.Exception("aExtraListeners must be an array or null",
                                  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)
             result = false;
         }
       }
       catch (e) {
         WARN("InstallListener threw exception when calling " + aMethod, e);
       }
-    });
+    }
     return result;
   },
 
   /**
    * Calls all registered AddonListeners with an event. Any parameters after
    * the method parameter are passed to the listener.
    *
    * @param  aMethod
    *         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);
       }
       catch (e) {
         WARN("AddonListener threw exception when calling " + aMethod, e);
       }
-    });
+    }
   },
 
   /**
    * Notifies all providers that an add-on has been enabled when that type of
    * add-on only supports a single add-on being enabled at a time. This allows
    * the providers to disable theirs if necessary.
    *
    * @param  aID
@@ -1151,30 +1179,26 @@ var AddonManagerInternal = {
     if (aID && typeof aID != "string")
       throw Components.Exception("aID must be a string or null",
                                  Cr.NS_ERROR_INVALID_ARG);
 
     if (!aType || typeof aType != "string")
       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);
   },
 
   /**
    * Notifies all providers they need to update the appDisabled property for
    * their add-ons in response to an application change such as a blocklist
    * update.
    */
   updateAddonAppDisabledStates: function AMI_updateAddonAppDisabledStates() {
-    this.providers.forEach(function(provider) {
-      callProvider(provider, "updateAddonAppDisabledStates");
-    });
+    this.callProviders("updateAddonAppDisabledStates");
   },
   
   /**
    * Notifies all providers that the repository has updated its data for
    * installed add-ons.
    *
    * @param  aCallback
    *         Function to call when operation is complete.
@@ -1248,17 +1272,18 @@ var AddonManagerInternal = {
     if (aVersion && typeof aVersion != "string")
       throw Components.Exception("aVersion must be a string or null",
                                  Cr.NS_ERROR_INVALID_ARG);
 
     if (aLoadGroup && (!(aLoadGroup instanceof Ci.nsILoadGroup)))
       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,
                      function(aInstall) {
           safeCall(aCallback, aInstall);
         });
         return;
       }
@@ -1360,17 +1385,18 @@ var AddonManagerInternal = {
    *         The mimetype to check
    * @return true if installation is enabled for the mimetype
    */
   isInstallEnabled: function AMI_isInstallEnabled(aMimetype) {
     if (!aMimetype || typeof aMimetype != "string")
       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;
     }
     return false;
   },
 
   /**
@@ -1387,17 +1413,18 @@ var AddonManagerInternal = {
     if (!aMimetype || typeof aMimetype != "string")
       throw Components.Exception("aMimetype must be a non-empty string",
                                  Cr.NS_ERROR_INVALID_ARG);
 
     if (aURI && !(aURI instanceof Ci.nsIURI))
       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;
     }
     return false;
   },
 
   /**
new file mode 100644
--- /dev/null
+++ b/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>
new file mode 100644
--- /dev/null
+++ b/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();
+  });
+}
--- a/toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini
+++ b/toolkit/mozapps/extensions/test/xpcshell/xpcshell.ini
@@ -124,16 +124,17 @@ fail-if = os == "android"
 # Bug 676992: test consistently fails on Android
 fail-if = os == "android"
 [test_bug619730.js]
 [test_bug620837.js]
 [test_bug655254.js]
 [test_bug659772.js]
 [test_bug675371.js]
 [test_bug740612.js]
+[test_bug757663.js]
 [test_cacheflush.js]
 [test_checkcompatibility.js]
 [test_ChromeManifestParser.js]
 [test_compatoverrides.js]
 [test_corrupt.js]
 [test_corrupt_strictcompat.js]
 [test_db_sanity.js]
 [test_dictionary.js]