321333 - Unable to uninstall or upgrade extensions due to existing extension directories having the read only attribute set

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

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

Split off from bug 316808

nsIFile remove respects read only attributes for files that it manages. Since under some circumstances on Win32 this can occur (see bug 316808 comment #21 - restoring from a cd backup comes to mind) and leave the extensions in an unusable state we should reset this when we have the permissions to do so.

To reproduce you have to use attrib +R on an extension's directories and then the nsIFile.remove will not remove the files even though you can do so with explorer. Setting the read only attribute on a directory using the ui (e.g. slecting properties of the directory and checking read only) will not cause this problem because the ui will only set the read only attribute on the files within the directory.

This did not affect 1.0 as noticeabley since it would successfully remove the extension's metadata (e.g. the extension was removed from the EM's ui) but it wouldn't remove the extension's directory when uninstalling. Since we now have the ability to install an extension in 1.5 by placing an extension's directory in the extensions directory this issue is compounded by these partially uninstalled extensions are being re-installed and showing up in the EM ui.

Also of note is bug 223771 which can be reproduced by setting the read only attribute using attrib +R.

Comment 1

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

bah - "nsIFile remove respects read only attributes for files that it manages."
should read
nsIFile remove respects read only attributes for files and the extension manager respects that for files that it manages.

Comment 2

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

Attachment: fallback to recursion / setting permissions

Comment 3

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

Attachment: fallback to recursive setting perms / delete and prevent duplicate pending ops

This should be enough for this bug but since Benjamin is on vacation until the 3rd I'm going to try to find some time to spend with this code before requesting review. Comments welcome

Comment 4

[Thomas Keller]

For those people which like to fix this bug temporarily on their PC, the correct cmd to remove the readonly attribute from the profile folder and all subfolders is

c:\Documents And Settings\...\Profiles\XXXXXXX.default\> attrib -R /S /D

This includes all files and folders in the current directory recursively as well as the folders themselves.

Comment 5

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

Attachment: patch

Adds a helper function named removeDirRecursive that will attempt to recursively set the permissions on each directory entry and then delete it if remove(true) fails. This patch is focused entirely on this one issue except for one minor change from using Components.interfaces.nsIFile.DIRECTORY_TYPE to nsILocalFile.DIRECTORY_TYPE for consistency with the rest of the file. The remaining issues I'll address in the other bugs with the same symptoms but different causes.

Comment 6

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

Note: besides bug 223771 which can be reproduced by setting the read only attribute on bookmarks.html I just checked that trying to delete a profile that has a read only attribute on its directory for the most part succeeds though it leaves the directory behind. Using explorer to delete the directory succeeds and does not warn about it being read only on XP. On a Win95 system using a copy of a profile restored from a CD none of the files in the profile were deleted but when deleting with explorer a warning is displayed though it also succeeds after confirming.

I am quite sure that XP no longer has the issue where directories / files copied from a CD have the read only attribute set but it wouldn't surprise me if there were still backup restore programs around that set this attribute while Win2k and below still have this issue. I haven't checked the behavior of a restore point yet.

Perhaps we should work out an overall solution since this affects multiple areas?

Comment 7

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

Fixed on trunk.

Note: there are a couple of more bugs that have the same general symptom of unable to install / upgrade extensions with the general sign in the ui of the extension having the must restart to install message. There are actually different root causes for these errors and this only addresses one of them. After I have addressed the others and this has baked on the trunk for a bit I'll request approval for 2.0 for this patch.

Comment 8

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

Self approving for 1.8.1 per
http://developer.mozilla.org/devnews/index.php/2006/01/30/tree-rules-for-the-18-branch/

Fixed on MOZILLA_1_8_BRANCH

Comment 9

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

*** Bug 325149 has been marked as a duplicate of this bug. ***

Comment 10

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

*** Bug 305620 has been marked as a duplicate of this bug. ***

Comment 11

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

Comment on attachment 207453 [details] [diff] [review]
patch

Requesting 1.8.0.2 - this adds a fallback method for removing files and directories where we set the appropriate permissions before we try to remove the item. I have exchanged emails with at least 3 people that have experienced this bug and have seen a couple of reports in the forums as well.

Comment 12

[Dave Liebreich [:davel]]

Comment on attachment 207453 [details] [diff] [review]
patch

as per discussion in triage meeting 23-feb-2006

Comment 13

[Daniel Veditz [:dveditz]]

Comment on attachment 207453 [details] [diff] [review]
patch

approved for 1.8.0 branch, a=dveditz for drivers

Comment 14

[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.7; previous revision: 1.144.2.16.2.6