Bug 474988: Mismatched adds and removes of install listeners will fail. r=robstrong

toolkit/mozapps/extensions/src/nsExtensionManager.js.in

@@ -4089,10 +4089,8 @@ ExtensionManager.prototype = {
 
     // If there are no compatibility checks running and no downloads in
     // progress then the install operations are complete.
-    if (this._compatibilityCheckCount == 0 && this._transactions.length == 0) {
+    if (this._compatibilityCheckCount == 0 && this._transactions.length == 0)
-      for (var i = 0; i < this._installListeners.length; ++i)
+      this._callInstallListeners("onInstallsCompleted");
-        this._installListeners[i].onInstallsCompleted();
-    }
   },
 
   /**
@@ -4300,8 +4298,7 @@ ExtensionManager.prototype = {
         this._xpi             = xpiFile;
         this._installManifest = installManifest;
 
-        for (var i = 0; i < em._installListeners.length; ++i)
+        em._callInstallListeners("onCompatibilityCheckStarted", item);
-          em._installListeners[i].onCompatibilityCheckStarted(item);
         em._compatibilityCheckCount++;
         em.update([item], 1, Ci.nsIExtensionManager.UPDATE_CHECK_COMPATIBILITY, this);
       },
@@ -4343,8 +4340,7 @@ ExtensionManager.prototype = {
         em.datasource.removeDownload(this._xpi.path);
         LOG("Version Check Phone Home Completed");
 
-        for (var i = 0; i < em._installListeners.length; ++i)
+        em._callInstallListeners("onCompatibilityCheckEnded", addon, status);
-          em._installListeners[i].onCompatibilityCheckEnded(addon, status);
 
         // Only compatibility updates (e.g. STATUS_VERSIONINFO) are currently
         // supported
@@ -4389,9 +4385,8 @@ ExtensionManager.prototype = {
               BundleManager.appName + " " + gApp.version + ", Toolkit " +
               gApp.platformVersion + ". Remote compatibility check did not " +
               "resolve this.");
-          
+
-          for (var i = 0; i < em._installListeners.length; ++i)
+          em._callInstallListeners("onInstallEnded", addon, INSTALLERROR_INCOMPATIBLE_VERSION);
-            em._installListeners[i].onInstallEnded(addon, INSTALLERROR_INCOMPATIBLE_VERSION);
 
           // We are responsible for cleaning up this file!
           InstallLocations.get(aInstallLocationKey).removeFile(this._xpi);
@@ -4400,10 +4395,8 @@ ExtensionManager.prototype = {
         em._compatibilityCheckCount--;
         // If there are no more compatibility checks running and no downloads in
         // progress then the install operations are complete.
-        if (em._compatibilityCheckCount == 0 && em._transactions.length == 0) {
+        if (em._compatibilityCheckCount == 0 && em._transactions.length == 0)
-          for (var i = 0; i < em._installListeners.length; ++i)
+          em._callInstallListeners("onInstallsCompleted");
-            em._installListeners[i].onInstallsCompleted();
-        }
       },
 
       QueryInterface: XPCOMUtils.generateQI([Ci.nsIAddonUpdateCheckListener])
@@ -4420,8 +4413,7 @@ ExtensionManager.prototype = {
                            aInstallLocationKey, "", "", "",
                            getURIFromFile(aXPIFile).spec,
                            "", "", "", "", 0, gApp.id);
-      for (var i = 0; i < this._installListeners.length; ++i)
+      this._callInstallListeners("onInstallStarted", addon);
-        this._installListeners[i].onInstallStarted(addon);
 
       shouldPhoneHomeIfNecessary = true;
       var installManifest = null;
@@ -4435,8 +4427,7 @@ ExtensionManager.prototype = {
       if (!installManifest) {
         LOG("The Install Manifest supplied by this item is not well-formed. " +
             "Installation will not proceed.");
-        for (var i = 0; i < this._installListeners.length; ++i)
+        this._callInstallListeners("onInstallEnded", addon, INSTALLERROR_INVALID_MANIFEST);
-          this._installListeners[i].onInstallEnded(addon, INSTALLERROR_INVALID_MANIFEST);
         return INSTALLERROR_INVALID_MANIFEST;
       }
     }
@@ -4504,8 +4495,7 @@ ExtensionManager.prototype = {
         // this function, BEFORE staging takes place... technically speaking
         // a location could become readonly during the phone home process,
         // but that's an edge case I don't care about.
-        for (var i = 0; i < this._installListeners.length; ++i)
+        this._callInstallListeners("onInstallEnded", addon, INSTALLERROR_RESTRICTED);
-          this._installListeners[i].onInstallEnded(addon, INSTALLERROR_RESTRICTED);
         return INSTALLERROR_RESTRICTED;
       }
 
@@ -4601,9 +4591,7 @@ ExtensionManager.prototype = {
     stageXPIForOtherApps(aXPIFile, installData);
 
     // The install of this item is complete, notify observers
-    for (var i = 0; i < this._installListeners.length; ++i)
+    this._callInstallListeners("onInstallEnded", addon, installData.error);
-      this._installListeners[i].onInstallEnded(addon, installData.error);
-
     return installData.error;
   },
 
@@ -5651,12 +5639,10 @@ ExtensionManager.prototype = {
     switch (state) {
     case nsIXPIProgressDialog.DOWNLOAD_START:
       ds.updateDownloadState(id, "downloading");
-      for (var i = 0; i < this._installListeners.length; ++i)
+      this._callInstallListeners("onDownloadStarted", addon);
-        this._installListeners[i].onDownloadStarted(addon);
       break;
     case nsIXPIProgressDialog.DOWNLOAD_DONE:
-      for (var i = 0; i < this._installListeners.length; ++i)
+      this._callInstallListeners("onDownloadEnded", addon);
-        this._installListeners[i].onDownloadEnded(addon);
       break;
     case nsIXPIProgressDialog.INSTALL_START:
       ds.updateDownloadState(id, "finishing");
@@ -5673,10 +5659,8 @@ ExtensionManager.prototype = {
       }
       transaction.removeDownload(addon.xpiURL);
       // A successful install will be passing notifications via installItemFromFile
-      if (value != 0) {
+      if (value != 0)
-        for (var i = 0; i < this._installListeners.length; ++i)
+        this._callInstallListeners("onInstallEnded", addon, value);
-          this._installListeners[i].onInstallEnded(addon, value);
-      }
       break;
     case nsIXPIProgressDialog.DIALOG_CLOSE:
       for (var i = 0; i < this._transactions.length; ++i) {
@@ -5689,10 +5673,8 @@ ExtensionManager.prototype = {
 
             // If there are no compatibility checks running then the install
             // operations are complete.
-            if (this._compatibilityCheckCount == 0) {
+            if (this._compatibilityCheckCount == 0)
-              for (var i = 0; i < this._installListeners.length; ++i)
+              this._callInstallListeners("onInstallsCompleted");
-                this._installListeners[i].onInstallsCompleted();
-            }
           }
           break;
         }
@@ -5704,8 +5686,7 @@ ExtensionManager.prototype = {
   },
 
   onProgress: function EM_onProgress(addon, value, maxValue) {
-    for (var i = 0; i < this._installListeners.length; ++i)
+    this._callInstallListeners("onDownloadProgress", addon, value, maxValue);
-      this._installListeners[i].onDownloadProgress(addon, value, maxValue);
 
     var id = addon.id != addon.xpiURL ? PREFIX_ITEM_URI + addon.id : addon.xpiURL;
     var progress = Math.round((value / maxValue) * 100);
@@ -5723,7 +5704,25 @@ ExtensionManager.prototype = {
   },
 
   removeInstallListenerAt: function EM_removeInstallListenerAt(index) {
-    this._installListeners.splice(index, 1);
+    if (index < 0 || index >= this._installListeners.length)
+      throw Cr.NS_ERROR_INVALID_ARGUMENT;
+
+    this._installListeners[index] = null;
+    while (this._installListeners[this._installListeners.length - 1] === null)
+      this._installListeners.splice(this._installListeners.length - 1, 1);
+  },
+
+  _callInstallListeners: function EM__callInstallListeners(method) {
+    for (var i = 0; i < this._installListeners.length; ++i) {
+      try {
+        if (this._installListeners[i])
+          this._installListeners[i][method].apply(this._installListeners[i],
+                                                  Array.slice(arguments, 1));
+      }
+      catch (e) {
+        LOG("Failure in install listener's " + method + ": " + e);
+      }
+    }
   },
 
   /**

toolkit/mozapps/extensions/test/unit/test_bug474988.js

@@ -0,0 +1,94 @@
+/* ***** BEGIN LICENSE BLOCK *****
+ * Version: MPL 1.1/GPL 2.0/LGPL 2.1
+ *
+ * The contents of this file are subject to the Mozilla Public License Version
+ * 1.1 (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ * http://www.mozilla.org/MPL/
+ *
+ * Software distributed under the License is distributed on an "AS IS" basis,
+ * WITHOUT WARRANTY OF ANY KIND, either express or implied. See the License
+ * for the specific language governing rights and limitations under the
+ * License.
+ *
+ * The Original Code is mozilla.org code.
+ *
+ * The Initial Developer of the Original Code is
+ * Dave Townsend <dtownsend@oxymoronical.com>.
+ *
+ * Portions created by the Initial Developer are Copyright (C) 2009
+ * the Initial Developer. All Rights Reserved.
+ *
+ * Contributor(s):
+ *
+ * Alternatively, the contents of this file may be used under the terms of
+ * either the GNU General Public License Version 2 or later (the "GPL"), or
+ * the GNU Lesser General Public License Version 2.1 or later (the "LGPL"),
+ * in which case the provisions of the GPL or the LGPL are applicable instead
+ * of those above. If you wish to allow use of your version of this file only
+ * under the terms of either the GPL or the LGPL, and not to allow others to
+ * use your version of this file under the terms of the MPL, indicate your
+ * decision by deleting the provisions above and replace them with the notice
+ * and other provisions required by the GPL or the LGPL. If you do not delete
+ * the provisions above, a recipient may use your version of this file under
+ * the terms of any one of the MPL, the GPL or the LGPL
+ *
+ * ***** END LICENSE BLOCK *****
+ */
+
+var installListenerA = {};
+var installListenerB = {};
+var installListenerC = {};
+var installListenerD = {};
+var installListenerE = {};
+
+function run_test() {
+  createAppInfo("xpcshell@tests.mozilla.org", "XPCShell", "2", "1.9");
+  startupEM();
+
+  // Add some listeners
+  do_check_eq(gEM.addInstallListener(installListenerA), 0);
+  do_check_eq(gEM.addInstallListener(installListenerB), 1);
+  do_check_eq(gEM.addInstallListener(installListenerC), 2);
+  // Now stack is ABC
+
+  // Should get back the same id if adding the same listener twice
+  do_check_eq(gEM.addInstallListener(installListenerA), 0);
+  do_check_eq(gEM.addInstallListener(installListenerB), 1);
+  do_check_eq(gEM.addInstallListener(installListenerC), 2);
+  // Stack is still ABC
+
+  // Removing a listener shouldn't affect the id of the others
+  gEM.removeInstallListenerAt(0);
+  do_check_eq(gEM.addInstallListener(installListenerB), 1);
+  do_check_eq(gEM.addInstallListener(installListenerC), 2);
+  // Stack is .BC
+
+  // Adding a new listener should still give a higher id than previously
+  do_check_eq(gEM.addInstallListener(installListenerD), 3);
+  // Stack is .BCD
+
+  // Removing and re-adding the highest listener should give the same result.
+  gEM.removeInstallListenerAt(3);
+  do_check_eq(gEM.addInstallListener(installListenerE), 3);
+  // Stack is .BCE
+
+  do_check_eq(gEM.addInstallListener(installListenerD), 4);
+  gEM.removeInstallListenerAt(3);
+  do_check_eq(gEM.addInstallListener(installListenerE), 5);
+  // Stack is .BC.DE
+
+  // Stack should shorten down to the last listener
+  gEM.removeInstallListenerAt(4);
+  gEM.removeInstallListenerAt(5);
+  do_check_eq(gEM.addInstallListener(installListenerD), 3);
+  // Stack is .BCD
+
+  // Stack should get down to 0
+  gEM.removeInstallListenerAt(1);
+  gEM.removeInstallListenerAt(2);
+  gEM.removeInstallListenerAt(3);
+  do_check_eq(gEM.addInstallListener(installListenerE), 0);
+  gEM.removeInstallListenerAt(0);
+  // Stack is empty
+}