Note: There are a few cases of duplicates in user autocompletion which are being worked on.

If an update fails, installed extensions get uninstalled

VERIFIED FIXED in mozilla2.0b7

Status

()

Toolkit
Add-ons Manager
--
critical
VERIFIED FIXED
7 years ago
7 years ago

People

(Reporter: sdwilsh, Assigned: mossop)

Tracking

({dataloss})

Trunk
mozilla2.0b7
dataloss
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +
in-litmus -

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Reporter)

Description

7 years ago
Created attachment 465737 [details]
extensions.log

Sync managed to get uninstalled because an update failed.  I've attached my extensions.log file, which shows the issue.
(Reporter)

Comment 1

7 years ago
This should probably block, at least on investigation since I've talked to at least one other person who's seen this (cc'd on bug).
blocking2.0: --- → ?
And it isn't something specific to Sync, I've had Firebug and one or two other extensions disappear after a failed update. Same extensions.log errors, with the other extension's UUID.
(Assignee)

Comment 3

7 years ago
The process for installing an update is to first delete the files from the old version, then move the new files into place. The log shows that the first step failed which means we throw away the update.

I'm guessing though that it actually succeeded to a certain extent, probably removed all the files from inside the directory and just left the directory in place due to a file lock. That'd then leave an empty directory so the extension would appear to be gone (and that directory would get removed on a subsequent startup).

The old code used to move files aside rather than removing them, we probably need to re-introduce that for safety here.
Assignee: nobody → dtownsend
blocking2.0: ? → betaN+
Depends on: 553015
(Assignee)

Updated

7 years ago
Duplicate of this bug: 588372
(Assignee)

Updated

7 years ago
Duplicate of this bug: 576227
Severity: normal → critical
Keywords: dataloss
OS: Windows 7 → All
Hardware: x86 → All
Summary: Sync magically uninstalled due to a failed update → If an update fails, installed extensions get uninstalled

Comment 6

7 years ago
This just happened with the latest dev version of noscript.
Dave, shouldn't we block beta6 on this? It's a dataloss bug which will be seen by more and more people.
(Assignee)

Comment 8

7 years ago
(In reply to comment #7)
> Dave, shouldn't we block beta6 on this? It's a dataloss bug which will be seen
> by more and more people.

I will be trying to fix it for beta 6 but I don't see a reason why it would block beta 6. I wouldn't really consider this to be dataloss either, it doesn't meet the criteria of "critical data loss", there is no data being lost just an extension and that can easily be recovered

Comment 9

7 years ago
In my case, there was no notification that an extension was removed; I didn't know NoScript was gone until I went to click on it the next day, and I had no idea what happened to it until I looked in my extensions.log file, which most end users probably won't do.

I clicked "Update Add-ons Now". Add-ons Manager said it was updating and then prompted for restart. There was no indication of what updated, before or after the restart. I tried looking in "View Recent Updates", and I checked the Last Updated field for each add-on. No clue. I thought it was a bug with update notifications. When NoScript came up missing the next day, I had forgotten about the update. The log file reminded me.


You probably have plenty of data, but as long as I'm posting, here are the last three extensions.log entries. The first error has recurred for over a year; I'm not concerned with it, but it includes NoScript's hex id. The second I included for completeness, in case it turns out to be relevant. The last looks like the uninstall, both the timestamp and description.

2010-09-14 11:26:38 - safeInstallOperation: failed to back up file: [profile path here]\extensions\{73a6fe31-595d-460b-a920-fcc0f8843232}\chrome\noscript.jar to: [profile path here]\extensions\{73a6fe31-595d-460b-a920-fcc0f8843232}-trash\chrome ... rolling back file moves and aborting installation.

2010-09-14 11:26:38 - ExtensionManager:_finishOperations - failure, catching exception - lineno: 1596 - file: undefined - [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIFile.moveTo]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: file:///C:/Program%20Files%20(x86)/ff36/components/nsExtensionManager.js :: moveFile :: line 1596" data: no]

2010-09-21 17:02:48 ERROR addons.xpi: Failed to install staged add-on {73a6fe31-595d-460b-a920-fcc0f8843232} in app-profile: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIFile.remove]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: resource://gre/modules/XPIProvider.jsm :: DirInstallLocation_installAddon :: line 5868" data: no]
(Assignee)

Comment 10

7 years ago
Created attachment 478591 [details] [diff] [review]
patch rev 1

This adds a class for safely performing a move of files from one directory to another allowing rollback at any point. For uninstalls we move old versions aside and rollback if anything fails. For installs we move old versions aside and new versions into place and roll everything back if any part fails. It includes tests on windows only where we lock either the add-on XPI or its install.rdf then try to upgrade or uninstall it verifying that it rolls back to the old version correctly.

This includes basically a backout of bug 562930. In that bug the problem was that extensions could load prior to the extension manager so if we knew an upgrade or uninstall was coming we tried to remove them from extensions.ini before restarting so they wouldn't load. Since then the platform has changed such that we never load extensions before the extension manager so this is no longer necessary.

Removing that is necessary here because if the uninstall/update of an add-on fails during startup then it's important that extensions.ini still contains the add-on otherwise the old version won't load correctly.

Currently any upgrade/uninstall that fails is never tried again (we delete the staging directory that contains that info on every run). That would be fairly easily changeable to retry on every startup depending on what we think is the best choice.
Attachment #478591 - Flags: review?(robert.bugzilla)
(Assignee)

Updated

7 years ago
Status: NEW → ASSIGNED
Whiteboard: [has patch][needs review rs]
(Assignee)

Updated

7 years ago
Duplicate of this bug: 599674
Comment on attachment 478591 [details] [diff] [review]
patch rev 1

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -155,16 +155,135 @@ var gIDTest = /^(\{[0-9a-f]{8}-[0-9a-f]{
>     Components.utils.import("resource://gre/modules/AddonLogging.jsm");
> 
>     LogManager.getLogger("addons.xpi", this);
>     return this[aName];
>   })
> }, this);
> 
> /**
>+ * A safe way to move a file or entire directory to a new directory. The move is
A safe way to move a file or the contents of a directory to a new directory.

might be worth noting that all directories are created along the way.

>+ * performed recursively and if anything fails an attempt is made to rollback
>+ * the entire operation. The operation may also be rolled back after complete by
>+ * calling the rollback method.
rolled back to its original state after it has completed by calling the rollback method.
or
rolled back to its original state after it has finished by calling the rollback method.

>+ *
>+ * Moves can be chained. Calling move multiple times will remember the whole set
>+ * and if one fails all that occured before will be rolled back.
if one fails all of the move operations will be rolled back.

>+ */
>+function SafeMoveOperation() {
>+  this._movedFiles = [];
>+  this._createdDirs = [];
>+}
>+
>+SafeMoveOperation.prototype = {
>+  _movedFiles: null,
>+  _createdDirs: null,
>+
>+  _moveFile: function(aFile, aTargetDirectory) {
>+    let oldFile = aFile.clone();
>+    let newFile = aFile.clone();
>+    try {
>+      newFile.moveTo(aTargetDirectory, null);
>+    }
>+    catch (e) {
>+      throw new Error("Failed to move file " + aFile.path + " to " +
>+                      aTargetDirectory.path + ": " + e);
>+    }
>+    this._movedFiles.push({ oldFile: oldFile, newFile: newFile });
>+  },
>+
>+  _moveDirectory: function(aDirectory, aTargetDirectory) {
>+    let newDir = aTargetDirectory.clone();
>+    newDir.append(aDirectory.leafName);
>+    try {
>+      newDir.create(Ci.nsILocalFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
>+    }
>+    catch (e) {
>+      throw new Error("Failed to create directory " + newDir.path + ": " + e);
>+    }
>+    this._createdDirs.push(newDir);
>+
>+    let entries = aDirectory.directoryEntries
>+                            .QueryInterface(Ci.nsIDirectoryEnumerator);
>+    try {
>+      let entry;
>+      while (entry = entries.nextFile)
>+        this._moveDirEntry(entry, newDir);
>+    }
>+    finally {
>+      entries.close();
>+    }
>+
>+    // The directory should be empty by this point. If it isn't this will throw
If it isn't this will throw and all of the operations will be rolled back.

>+    try {
>+      aDirectory.remove(false);
>+    }
>+    catch (e) {
>+      throw new Error("Failed to remove directory " + aDirectory.path + ": " + e);
>+    }
>+
>+    // Note we put the directory move in after all the file moves so the
>+    // directory is recreated before all the files are moved back
>+    this._movedFiles.push({ oldFile: aDirectory, newFile: newDir });
>+  },
>+
>+  _moveDirEntry: function(aDirEntry, aTargetDirectory) {
>+    if (aDirEntry.isDirectory())
>+      this._moveDirectory(aDirEntry, aTargetDirectory);
>+    else
>+      this._moveFile(aDirEntry, aTargetDirectory);
>+  },
>+
>+  /**
>+   * Moves a file or directory into a new directory. If an error occurs then all
>+   * files moved will be moved back to where they came from.
If an error occurs then all files that have been moved will be moved back to their original location.

>+   *
>+   * @param  aFile
>+   *         The file or directory to be moved.
>+   * @param  aTargetDirectory
>+   *         The directory to move into, this is expected to be an empty
>+   *         directory.
>+   */
>+  move: function(aFile, aTargetDirectory) {
>+    try {
>+      this._moveDirEntry(aFile, aTargetDirectory);
>+    }
>+    catch (e) {
>+      ERROR("Failure moving " + aFile.path + " to " + aTargetDirectory.path + ": " + e);
>+      this.rollback();
>+      throw e;
>+    }
>+  },
>+
>+  /**
>+   * Rolls back all the moves that this operation performed. If an exception
>+   * occurs here them both old and new directories are left in an indeterminate
>+   * state
occurs here s/them/then/

>+   */
>+  rollback: function() {
>+    while (this._movedFiles.length > 0) {
>+      let move = this._movedFiles.pop();
>+      if (move.newFile.isDirectory()) {
>+        let oldDir = move.oldFile.parent.clone();
>+        oldDir.append(move.oldFile.leafName);
>+        oldDir.create(Ci.nsILocalFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
>+      }
>+      else {
>+        move.newFile.moveTo(move.oldFile.parent, null);
>+      }
>+    }
>+
>+    while (this._createdDirs.length > 0) {
>+      let dir = this._createdDirs.pop();
>+      dir.remove(false);
>+    }
Though this shouldn't fail I'm a tad concerned that it might especially with passing false to dir.remove. Perhaps something like I suggested in bug 568251 comment #5.

>+  }
>+};

>@@ -649,29 +768,46 @@ function buildJarURI(aJarfile, aPath) {
>   let uri = Services.io.newFileURI(aJarfile);
>   uri = "jar:" + uri.spec + "!/" + aPath;
>   return NetUtil.newURI(uri);
> }
> 
> /**
>  * Creates and returns a new unique temporary file. The caller should delete
>  * the file when it is no longer needed.
>+ *
>  * @return an nsIFile that points to a randomly named, initially empty file in
>  *         the OS temporary files directory
>  */
> function getTemporaryFile() {
>   let file = FileUtils.getDir(KEY_TEMPDIR, []);
>   let random = Math.random().toString(36).replace(/0./, '').substr(-3);
>   file.append("tmp-" + random + ".xpi");
>   file.createUnique(Ci.nsIFile.NORMAL_FILE_TYPE, FileUtils.PERMS_FILE);
> 
>   return file;
> }
> 
> /**
>+ * Creates and returns a new unique temporary directory. The caller should
>+ * delete the directory when it is no longer needed.
>+ *
>+ * @return an nsIFile that points to a randomly named, initially empty directory
>+ *         in the OS temporary files directory
technically this is the user's temporary directory and not the OS's (e.g. C:\Users\rstrong\AppData\Local\Temp vs. C:\Windows\Temp). Same for getTemporaryFile.

>+ */
>+function getTemporaryDirectory() {
>+  let file = FileUtils.getDir(KEY_TEMPDIR, []);
>+  let random = Math.random().toString(36).replace(/0./, '').substr(-3);
>+  file.append("tmp-" + random);
>+  file.createUnique(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
>+
>+  return file;
>+}
The 1.9.2 safeInstallOperation used a trash folder inside the install location and this uses the temp dir which can be on a different volume.
Comment on attachment 478591 [details] [diff] [review]
patch rev 1

>@@ -2912,20 +3047,19 @@ var XPIDatabase = {
>     getVisibleAddons: "SELECT " + FIELDS_ADDON + " FROM addon WHERE visible=1",
>     getVisibleAddonsWithPendingOperations: "SELECT " + FIELDS_ADDON + " FROM " +
>                                            "addon WHERE visible=1 " +
>                                            "AND (pendingUninstall=1 OR " +
>                                            "MAX(userDisabled,appDisabled)=active)",
> 
>     makeAddonVisible: "UPDATE addon SET visible=1 WHERE internal_id=:internal_id",
>     removeAddonMetadata: "DELETE FROM addon WHERE internal_id=:internal_id",
>-    // Equates to active = visible && !userDisabled && !appDisabled &&
>-    // !pendingUninstall
>+    // Equates to active = visible && !userDisabled && !appDisabled
>     setActiveAddons: "UPDATE addon SET active=MIN(visible, 1 - userDisabled, " +
>-                     "1 - appDisabled, 1 - pendingUninstall)",
>+                     "1 - appDisabled)",
Wouldn't an add-on that is not restartless still be active when it is pendingUninstall before the app is restarted?

>...
>@@ -6041,37 +6162,56 @@ DirectoryInstallLocation.prototype = {
>    *
>    * @param  aId
>    *         The ID of the add-on to install
>    * @param  aSource
>    *         The source nsIFile to install from
>    * @return an nsIFile indicating where the add-on was installed to
>    */
>   installAddon: function DirInstallLocation_installAddon(aId, aSource) {
>-    let file = this._directory.clone().QueryInterface(Ci.nsILocalFile);
>-    file.append(aId);
>-    if (file.exists())
>-      file.remove(true);
>-
>-    file = this._directory.clone().QueryInterface(Ci.nsILocalFile);
>-    file.append(aId + ".xpi");
>-    if (file.exists()) {
>-      Services.obs.notifyObservers(file, "flush-cache-entry", null);
>-      file.remove(true);
>-    }
>-
>-    aSource = aSource.clone().QueryInterface(Ci.nsILocalFile);
>-    if (aSource.isFile())
>-      Services.obs.notifyObservers(aSource, "flush-cache-entry", null);
>-    aSource.moveTo(this._directory, aSource.leafName);
>-    aSource.lastModifiedTime = Date.now();
>-    this._FileToIDMap[aSource.path] = aId;
>-    this._IDToFileMap[aId] = aSource;
>-
>-    return aSource;
>+    let transaction = new SafeMoveOperation();
>+    let tempDir = getTemporaryDirectory();
>+
>+    // Ensure we try to clean up the temporary directory
this comment doesn't really make much sense to me here

>+    try {
>+      let file = this._directory.clone().QueryInterface(Ci.nsILocalFile);
>+      file.append(aId);
>+      if (file.exists())
>+        transaction.move(file, tempDir);
>+  
>+      file = this._directory.clone().QueryInterface(Ci.nsILocalFile);
>+      file.append(aId + ".xpi");
>+      if (file.exists()) {
>+        Services.obs.notifyObservers(file, "flush-cache-entry", null);
>+        transaction.move(file, tempDir);
>+      }
>+  
>+      if (aSource.isFile())
>+        Services.obs.notifyObservers(aSource, "flush-cache-entry", null);
>+  
>+      transaction.move(aSource, this._directory);
>+    }
>+    finally {
>+      // It isn't ideal if this cleanup fails but it isn't worth rolling back
>+      // the install because of it.
>+      try {
>+        tempDir.remove(true);
>+      }
>+      catch (e) {
>+        WARN("Failed to remove temporary directory when installing " + aId);
>+      }
>+    }
>+
>+    let newFile = this._directory.clone().QueryInterface(Ci.nsILocalFile);
>+    newFile.append(aSource.leafName);
>+    newFile.lastModifiedTime = Date.now();
>+    this._FileToIDMap[newFile.path] = aId;
>+    this._IDToFileMap[aId] = newFile;
>+
>+    return newFile;
>   },

The tests should check that the temporary dir was removed as well.

I'd like to take a look at this after the changes have been made or reasons given for not making the changes.
Attachment #478591 - Flags: review?(robert.bugzilla) → review-
Whiteboard: [has patch][needs review rs] → [needs new patch]
(Assignee)

Comment 14

7 years ago
(In reply to comment #13)
> Comment on attachment 478591 [details] [diff] [review]
> patch rev 1
> 
> >@@ -2912,20 +3047,19 @@ var XPIDatabase = {
> >     getVisibleAddons: "SELECT " + FIELDS_ADDON + " FROM addon WHERE visible=1",
> >     getVisibleAddonsWithPendingOperations: "SELECT " + FIELDS_ADDON + " FROM " +
> >                                            "addon WHERE visible=1 " +
> >                                            "AND (pendingUninstall=1 OR " +
> >                                            "MAX(userDisabled,appDisabled)=active)",
> > 
> >     makeAddonVisible: "UPDATE addon SET visible=1 WHERE internal_id=:internal_id",
> >     removeAddonMetadata: "DELETE FROM addon WHERE internal_id=:internal_id",
> >-    // Equates to active = visible && !userDisabled && !appDisabled &&
> >-    // !pendingUninstall
> >+    // Equates to active = visible && !userDisabled && !appDisabled
> >     setActiveAddons: "UPDATE addon SET active=MIN(visible, 1 - userDisabled, " +
> >-                     "1 - appDisabled, 1 - pendingUninstall)",
> >+                     "1 - appDisabled)",
> Wouldn't an add-on that is not restartless still be active when it is
> pendingUninstall before the app is restarted?

Yes, this change makes it so those add-ons are still marked as active in the database since if the uninstall fails they should remain active and if it completes they will be removed from the db and so will not be active anymore.

> >-    return aSource;
> >+    let transaction = new SafeMoveOperation();
> >+    let tempDir = getTemporaryDirectory();
> >+
> >+    // Ensure we try to clean up the temporary directory
> this comment doesn't really make much sense to me here

Cleaned that up

> The tests should check that the temporary dir was removed as well.

head.js already tests that the directory we register as TmpD is empty at the end of the test run.

Comment 15

7 years ago
im still getting update failures only when a certain combination of extensions is enabled.

i've just resorted to disabling ABP before updating.
(Assignee)

Comment 16

7 years ago
(In reply to comment #12)
> >+ */
> >+function getTemporaryDirectory() {
> >+  let file = FileUtils.getDir(KEY_TEMPDIR, []);
> >+  let random = Math.random().toString(36).replace(/0./, '').substr(-3);
> >+  file.append("tmp-" + random);
> >+  file.createUnique(Ci.nsIFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
> >+
> >+  return file;
> >+}
> The 1.9.2 safeInstallOperation used a trash folder inside the install location
> and this uses the temp dir which can be on a different volume.

Changed to use a trash directory in the install location, also started verifying that the main directories within the profile install location are missing at the end of all tests. Turns out they weren't all the time because in some cases we don't clean up the staging directory completely (like when an install gets staged then cancelled), I've added a helper function to do that automatically in those cases.
(Assignee)

Comment 17

7 years ago
Created attachment 483307 [details] [diff] [review]
patch rev 2
Attachment #478591 - Attachment is obsolete: true
Attachment #483307 - Flags: review?(robert.bugzilla)
Comment on attachment 483307 [details] [diff] [review]
patch rev 2

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>...
>@@ -156,16 +157,134 @@ var gIDTest = /^(\{[0-9a-f]{8}-[0-9a-f]{
>...
>+  _moveDirectory: function(aDirectory, aTargetDirectory) {
>+    let newDir = aTargetDirectory.clone();
>+    newDir.append(aDirectory.leafName);
>+    try {
>+      newDir.create(Ci.nsILocalFile.DIRECTORY_TYPE, FileUtils.PERMS_DIRECTORY);
>+    }
>+    catch (e) {
>+      throw new Error("Failed to create directory " + newDir.path + ": " + e);
>+    }
>+    this._createdDirs.push(newDir);
>+
>+    let entries = aDirectory.directoryEntries
>+                            .QueryInterface(Ci.nsIDirectoryEnumerator);
>+    try {
>+      let entry;
>+      while (entry = entries.nextFile)
>+        this._moveDirEntry(entry, newDir);
>+    }
>+    finally {
>+      entries.close();
>+    }
>+
>+    // The directory should be empty by this point. If it isn't this will throw
>+    // and all of the operations will be rolled back
>+    try {
>+      aDirectory.remove(false);
Might be a good thing to try to set the permissions on this before trying to remove... leave that up to you.

>@@ -885,16 +1005,45 @@ function resultRows(aStatement) {
>       yield aStatement.row;
>   }
>   finally {
>     aStatement.reset();
>   }
> }
> 
> /**
>+ * Removes a number of entries in a staging directory and then if the staging
>+ * directory is empty attempts to remove it.
nit: not sure if this is better
Removes the specified files / directories in a staging directory and then if the staging directory is empty attempts to remove it.

>+ *
>+ * @param  aDir
>+ *         The staging directory to clean up
nsIFile for the staging directory to clean up

>+ * @param  aEntries
>+ *         An array of filenames to remove from the directory, the array may be
>+ *         empty
An array of file / directory names

I think this parameter would be better named as aLeafNames

>+ */
>+function cleanStagingDir(aDir, aEntries) {
>+  aEntries.forEach(function(aEntry) {
>+    let file = aDir.clone();
>+    file.append(aEntry);
>+    if (file.exists())
>+      recursiveRemove(file);
>+  });
>+
>+  if (aDir.directoryEntries.hasMoreElements())
>+    return;
>+
>+  try {
>+    aDir.remove(false);
Might be a good thing to try to set the permissions on this before trying to remove... leave that up to you.
Attachment #483307 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 19

7 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/58994b6ef83b
Status: ASSIGNED → RESOLVED
Last Resolved: 7 years ago
Flags: in-testsuite+
Flags: in-litmus-
Resolution: --- → FIXED
Whiteboard: [needs new patch]
Target Milestone: --- → mozilla2.0b8
(Assignee)

Comment 20

7 years ago
Backed out as this was breaking the browser tests
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Updated

7 years ago
Depends on: 562922
(Assignee)

Comment 21

7 years ago
As well as the browser chrome failures that bug 562922 should fix this also breaks xpcshell tests on windows debug builds and from IRC it looks like it breaks the installation of the crashme extension on Fennec on Android.
(Assignee)

Updated

7 years ago
Duplicate of this bug: 553015
(Assignee)

Comment 23

7 years ago
Created attachment 484729 [details] [diff] [review]
bustage fix rev 1

This is an additional patch that fixes the bustage seen on windows debug xpcshell, it also seems to work ok on android now too.

It appears that not closing the directory enumerator is leaving something in a bad state.
Attachment #484729 - Flags: review?(robert.bugzilla)
Comment on attachment 484729 [details] [diff] [review]
bustage fix rev 1

>diff --git a/toolkit/mozapps/extensions/XPIProvider.jsm b/toolkit/mozapps/extensions/XPIProvider.jsm
>--- a/toolkit/mozapps/extensions/XPIProvider.jsm
>+++ b/toolkit/mozapps/extensions/XPIProvider.jsm
>@@ -1023,18 +1023,24 @@ function resultRows(aStatement) {
> function cleanStagingDir(aDir, aLeafNames) {
>   aLeafNames.forEach(function(aName) {
>     let file = aDir.clone();
>     file.append(aName);
>     if (file.exists())
>       recursiveRemove(file);
>   });
> 
>-  if (aDir.directoryEntries.hasMoreElements())
>-    return;
>+  let dirEntries = aDir.directoryEntries.QueryInterface(Ci.nsIDirectoryEnumerator);
>+  try {
>+    if (dirEntries.nextFile)
>+      return true;
Why return true?

r=me with that changed to return;
Attachment #484729 - Flags: review?(robert.bugzilla) → review+
(Assignee)

Comment 25

7 years ago
Landed: http://hg.mozilla.org/mozilla-central/rev/ac4ad11c9a7a
Status: REOPENED → RESOLVED
Last Resolved: 7 years ago7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Target Milestone: mozilla2.0b8 → mozilla2.0b7
Blocks: 461973
No longer depends on: 553015

Comment 26

7 years ago
Any idea if this bug is more likely to happen when upgrading from beta 6 to 7? Or perhaps add-ons were being updated to work with beta 7 and triggered an update?
(Assignee)

Comment 27

7 years ago
(In reply to comment #26)
> Any idea if this bug is more likely to happen when upgrading from beta 6 to 7?
> Or perhaps add-ons were being updated to work with beta 7 and triggered an
> update?

I know of no reason why it would be more likely to happen
Verified fixed with Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b12pre) Gecko/20110209 Firefox/4.0b12pre ID:20110209030359 by revoking write access to the formerly version of the extension.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.