326895 - Extension isn't uninstalled cleanly when its folder is in use ( safeInstallOperation failure )

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

[Adam Kowalczyk]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060211 Firefox/1.5
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8) Gecko/20060211 Firefox/1.5

When the extension folder is in use, the files get deleted but the folder does not and a phantom extension is left in the EM. Trying to bring up Options of the phantom extension freezes Firefox. The extension disappears after the folder is no longer in use.

Same happens with themes.

Reproducible: Always

Steps to Reproduce:
1. Make the extension folder in use e.g. open it in Total Commander (Windows Explorer won't do).
2. Uninstall the extension and restart Firefox.
3. The files are deleted but the folder remains. There's a phantom extension in the EM.
4. Try to bring up Options of the extension - this freezes Firefox.
5. After the folder is no longer in use, the extension gets cleaned up on the next restart.



Maybe we shouldn't uninstall anything until the folder is deletable. Leaving user with a permanent message that the extension will be uninstalled on the next restart which doesn't happen isn't perfect but it's definitely better. Normal users don't mess with folders in profile anyway, so the exposure is very low unless there are other variations of this bug.

Comment 1

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

This is caused by the code for putting back the files on removal failure not working properly in all situations.

Comment 2

[Dave Townsend [:mossop]]

Attachment: Fix rollback

This patch makes three main changes:

Firstly rollbackMove is changed to remove files from the movedFiles array as each is rolled back. This is not really necessary for the rest of this patch or for any other situation I can think of, but I think it makes the system a bit cleaner.

Secondly moveDirectory previously absorbed any exceptions thrown from moveFile. That meant that if moveFile failed, the uninstallation would continue and try to remove the remaining files from the item location.

Thirdly, moveDirectory was basically broken because it did not QI the directoryEntries to a nsIDirectoryEnumerator, then attempted to use it as one, so no files were ever moved away and when any extension removal/upgrade failed, rollback didnt have anything to rollback.

Comment 3

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

Comment on attachment 214096 [details] [diff] [review]
Fix rollback

Dave, thanks for taking care of this.

>   function rollbackMove()
>   {
>-    for (var i = 0; i < movedFiles.length; ++i) {
>-      var oldFile = movedFiles[i].oldFile;
>-      var newFile = movedFiles[i].newFile;
>+    while (movedFiles.length > 0) {
>+      var movedFile = movedFiles.pop();
>+      var oldFile = movedFile.oldFile;
>+      var newFile = movedFile.newFile;
>       try {
>         newFile.moveTo(oldFile.parent, newFile.leafName);
>       }
>       catch (e) {
>+        movedFiles.push(movedFile);
Since this throws at this point what is the value of adding back the file it failed to move? I'm not entirely sure there isn't an edgecase where the last file in the array being rolled back won't fail and with the old way going from first to last it would at least put back all the files before failing entirely - that is if it had been working in the first place. :/

Since this change isn't needed I feel safer leaving it the way it was.

>+    while (true) {
>+      var entry = entries.nextFile;
>+      if (!entry)
>+        break;
>+      if (entry.isDirectory())
>+        moveDirectory(sourceDir, targetDir, entry);
>+      else {
>+        if (entry instanceof nsILocalFile) {
nit: this should be changed to an else if

With those two items fixed r=me and thanks again!

Comment 4

[Dave Townsend [:mossop]]

(In reply to comment #3)
> Since this change isn't needed I feel safer leaving it the way it was.
That's fair enough.

> With those two items fixed r=me and thanks again!
Cheers :)

Comment 5

[Dave Townsend [:mossop]]

Attachment: Final patch

Addressed comments, carried over review.

Comment 6

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

Checked in on trunk

Comment 7

[Ben Goodger (use ben at mozilla dot org for email)]

Comment on attachment 214290 [details] [diff] [review]
Final patch

a=[e-mail] for the 1.8.1 branch

Comment 8

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

Fixed on MOZILLA_1_8_BRANCH

Comment 9

[Jay Patel [:jay]]

Comment on attachment 214290 [details] [diff] [review]
Final patch

Please check in promptly on the 1.8.0 branch.  Thanks!

Comment 10

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

Checked in to MOZILLA_1_8_0_BRANCH

Checking in mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in;
/cvsroot/mozilla/toolkit/mozapps/extensions/src/nsExtensionManager.js.in,v  <--  nsExtensionManager.js.in
new revision: 1.144.2.16.2.6; previous revision: 1.144.2.16.2.5