439672 - ExtensionManager:_finishOperations removeDirRecursive failed because directory was in use

Status: VERIFIED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[timeless]

as defined here, this problem is probably windows only. but it is probably possible to have other platforms which would encounter it (especially fat on linux or various network file systems).

steps:
1. install firefox 3.0
Build identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9) Gecko/2008052906 Firefox/3.0
(do it today, be counted!)
2. install noscript 1.6.9.3
3. quit
4. find your profile directory
5. delete extensions.rdf
6. in cmd:
cd extensions\{73a6fe31-595d-460b-a920-fcc0f8843232}
[do not quit this command prompt! and do not change the working directory!]
7. change install.rdf to say 1.6 instead of 1.6.9.3
8. run firefox
9. open addons
10. check for updates
11. install the noscript update
12. click restart firefox
13. open addons
14. check to see if noscript is listed as updated.

2008-06-17 22:21:40 - safeInstallOperation: failed to clean up item location after its contents were properly backed up. Failed to clean up: Mozilla\Firefox\Profiles\Test\extensions\{73a6fe31-595d-460b-a920-fcc0f8843232} ... rolling back file moves and aborting installation.
2008-06-17 22:21:40 - ExtensionManager:_finishOperations - failure, catching exception - lineno: 543 - file: undefined - [Exception... "Component returned failure code: 0x80520015 (NS_ERROR_FILE_ACCESS_DENIED) [nsIFile.remove]"  nsresult: "0x80520015 (NS_ERROR_FILE_ACCESS_DENIED)"  location: "JS frame :: file:///C:/Program%20Files/Mozilla%20Firefox%203/components/nsExtensionManager.js :: removeDirRecursive :: line 543"  data: no]

Comment 1

[Robert Strong (they/them - no direct email)]

This is due to the way Windows prevents deleting files and directories under certain circumstances such as the one in your steps with cmd.exe and cd'ing into the extensions directory. I've seen other Windows programs in the past not handle deleting its files / directories when a separate program is using them and I personally think the current behavior is fine since these files and directories are application managed.

Comment 2

[timeless]

careful. the content indexer and virus scanners both will randomly open these directories. so will things like picasa.

i'd prefer a user facing message (after trying one extra time?).

Comment 3

[Robert Strong (they/them - no direct email)]

No problem... I'm for handling edgecases such as this but I'd prefer steps to reproduce that aren't a case of shooting oneself in the foot such as using cmd.exe.

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Dave, is that issue still valid after the rewrite of the EM? Would we still encounter an issue like that?

Comment 5

[Dave Townsend [:mossop]]

Still a small issue here, if an add-on is getting uninstalled and that fails because it is locked it remains running on next startup, claims to be pending uninstall but will never go away.

Comment 6

[Dave Townsend [:mossop]]

Attachment: patch rev 1

I realised that some of the behaviour here wasn't quite what a user probably wants. If an uninstall/upgrade/install fails because of a failure to copy files then just throwing away that operation probably isn't right. Instead after the restart the operation should stay pending and try again after a restart (or the user should be able to undo it). This is a precursor to actually being able to signal to the user that something went wrong here too but that is something for another bug.

This gets slightly complicated because for the upgrade/install case we need to recreate the pending AddonInstall info out of the add-on waiting to be installed. To do that I moved around some of the initialisation code for AddonInstall to make it hopefully a bit clearer for the different ways it can be initialised.

Comment 7

[Robert Strong (they/them - no direct email)]

Comment on attachment 540869 [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
>...
>@@ -5293,45 +5314,34 @@ function getHashStringForCrypto(aCrypto)
>   let binary = aCrypto.finish(false);
>   return [toHexString(binary.charCodeAt(i)) for (i in binary)].join("").toLowerCase()
> }
> 
> /**
>  * Instantiates an AddonInstall and passes the new object to a callback when
>  * it is complete.
callback is no longer applicable

>  *
>- * @param  aCallback
>- *         The callback to pass the AddonInstall to
>...
>@@ -6377,18 +6451,24 @@ AddonInstall.createInstall = function(aC
>  *         A version for the add-on
>  * @param  aLoadGroup
>  *         An nsILoadGroup to associate the download with
>  */
> AddonInstall.createDownload = function(aCallback, aUri, aHash, aName, aIconURL,
>                                        aVersion, aLoadGroup) {
>   let location = XPIProvider.installLocationsByName[KEY_APP_PROFILE];
>   let url = NetUtil.newURI(aUri);
>-  new AddonInstall(aCallback, location, url, aHash, aName, null,
>-                   aIconURL, aVersion, null, null, aLoadGroup);
>+
>+  let install = new AddonInstall(location, url, aHash, null, null, aLoadGroup);
>+  if (url instanceof Ci.nsIFileURL) {
>+    install.initLocalInstall(aCallback);
>+  }
>+  else {
>+    install.initAvailableDownload(aName, null, aIconURL, aVersion, aCallback);
>+  }
nit: brackets not needed and not your normal style

>@@ -6401,19 +6481,26 @@ AddonInstall.createUpdate = function(aCa
>   let releaseNotesURI = null;
>   try {
>     if (aUpdate.updateInfoURL)
>       releaseNotesURI = NetUtil.newURI(escapeAddonURI(aAddon, aUpdate.updateInfoURL));
>   }
>   catch (e) {
>     // If the releaseNotesURI cannot be parsed then just ignore it.
>   }
>-  new AddonInstall(aCallback, aAddon._installLocation, url, aUpdate.updateHash,
>-                   aAddon.selectedLocale.name, aAddon.type,
>-                   aAddon.iconURL, aUpdate.version, releaseNotesURI, aAddon);
>+
>+  let install = new AddonInstall(aAddon._installLocation, url,
>+                                 aUpdate.updateHash, releaseNotesURI, aAddon);
>+  if (url instanceof Ci.nsIFileURL) {
>+    install.initLocalInstall(aCallback);
>+  }
>+  else {
>+    install.initAvailableDownload(aAddon.selectedLocale.name, aAddon.type,
>+                                  aAddon.iconURL, aUpdate.version, aCallback);
>+  }
> };
> 
> /**
>  * Creates a wrapper for an AddonInstall that only exposes the public API
>  *
>  * @param  install
>  *         The AddonInstall to create a wrapper for
>  */
>@@ -7091,20 +7178,22 @@ function AddonWrapper(aAddon) {
>   this.__defineGetter__("pendingOperations", function() {
>     let pending = 0;
>     if (!(aAddon instanceof DBAddonInternal)) {
>       // Add-on is pending install if there is no associated install (shouldn't
>       // happen here) or if the install is in the process of or has successfully
>       // completed the install.
>       if (!aAddon._install || aAddon._install.state == AddonManager.STATE_INSTALLING ||
>           aAddon._install.state == AddonManager.STATE_INSTALLED)
>-        pending |= AddonManager.PENDING_INSTALL;
>+        return AddonManager.PENDING_INSTALL;
>     }
>     else if (aAddon.pendingUninstall) {
>-      pending |= AddonManager.PENDING_UNINSTALL;
>+      // If an add-on is pending uninstall then we ignore any other pending
>+      // operations
>+      return AddonManager.PENDING_UNINSTALL;
Similar comment applies to returning AddonManager.PENDING_INSTALL above

Comment 8

[Dave Townsend [:mossop]]

Landed: http://hg.mozilla.org/mozilla-central/rev/aa6e6d48e96a

Comment 9

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified fixed with Mozilla/5.0 (Windows NT 5.1; rv:7.0a2) Gecko/20110810 Firefox/7.0a2