323546 - Installing XPI of an extension developed direct from directory deletes entire directory.

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, enhancement)

Description

[Daniel Orner]

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

I was developing an extension by Firefox 1.5's new "direct from directory" method (where an XPI is not required). However, I then wanted to test it out using an XPI. When I installed the XPI, the ENTIRE CONTENTS of the directory containing the extension (including source files and everything else in it) was deleted. 

I then tried to replicate this bug in 1.6a but never even got that far: the directory contents were deleted as soon as it was installed! There's something weird when dealing with the file in the "extensions" folder that points to the directory. I finally renamed that file and tried again, and was able to replicate the bug in 1.6a using the method mentioned earlier.

Reproducible: Always

Steps to Reproduce:
1. Create a file with the GUID of an extension (in my case, "{7e1d8572-7d97-4e52-983b-f29498a9bca6}" in the _profile folder_/extensions directory; the file's contents point to a directory on disk containing the extension
2. Start Firefox, ensure extension is installed
3. Turn the extension directory into an XPI (I used roachfiend's zip.bat to do this)
4. Open the XPI in Firefox while the previous extension is still installed
5. Start Firefox.
Actual Results:  
Entire contents of extension directory are deleted.

Expected Results:  
The extension should just be upgraded.

I was using Qute, but I don't see how that would affect things. 8-)

Comment 1

[Dave Townsend [:mossop]]

I think this might be a duplicate, or at least partially answered in bug 305620

Comment 2

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

Daniel - the behavior as designed is to treat an extension installed in a location exactly the same as other extensions installed into that location. If a file pointer is used from the profile's extensions directory the same capabilities are provided and the same file system permissions are required. So, upgrades will upgrade the directory it is installed in - as long as the same permissions are applied - with the one exception being uninstall where it just removes the file pointer. It sounds like you would prefer it to remove the file pointer and install a new copy in the profile's extensions directory which I think sounds like it may be appropriate but it is not the way it is designed. If this were changed the behavior for updates should probably also be changed since it currently does the same thing and since we wouldn't be updating in place (e.g. we would be installing a new copy in the extensions directory) updates should probably be disabled for items installed with a file pointer. I personally prefer the current behavior since it behaves consistently for all items installed in a location though I would consider changing the behavior as long as the new behavior is also consistent and takes into account all of the scenarios (e.g. uninstall, upgrade, install, etc.) without it adding significant complexity to the code.

Comment 3

[Daniel Orner]

Robert: I can understand why it happens, but that doesn't excuse the fact that it happens. 8p The whole point of being able to develop an extension using a file pointer is that the source code is contained in that directory. It's pretty common for someone to then package the extension and test it out in their browser. Deleting the entire contents of the directory, which contains all the source code as well as other things (in my case it was my entire thesis!) is absolutely the wrong way to do it IMO, especially since there are no warnings of this behavior anywhere. It took me over two hours to find an undelete program that managed to find the files again.

In other words, if the question is to choose between a) usability and simplicity, and b) not automatically deleting files in a way which makes it extremely difficult to get them back, the choice shouldn't even be a choice.

Comment 4

[Dave Townsend [:mossop]]

I would tend to agree, I think the following behaviour is sensible:

On uninstall just remove the file pointer, as now.
On upgrade/reinstall remove the file pointer then install as normal.

I cant think of any other scenarios that apply to an already existing extension. This setup would also solve the problem in bug 305620 since the EM is no longer trying to delete files it didn't create.

Rob, I'm quite interested to see this happen so depending on your thoughts I wouldn't mind taking a crack at this.

Comment 5

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

My main concern is that extension authors are an extremely small subset of the people using Firefox and there are no numbers as to distributors or regular users using this method to install extensions along with the application should always do what is right for an end user. Having said that, I believe the numbers are for all practical purposes very small and that in this case the usability case should probably be for extension authors and not end users.

Dave - the new behavior sounds fine if you'd like to change it to what you outlined but it would probably be a good thing to run it by darin and ben in case they have any concerns.

Comment 6

[Darin Fisher]

I think Dave's suggestion makes sense.

Comment 7

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

This seems like a pretty bad bug. The suggestions sound good. Thanks for helping out!

Comment 8

[Dave Townsend [:mossop]]

Attachment: Remove file pointer before installing new version

This implements the behaviour described above in a suitably safe manner.

Installer doesn't need to cache the metadataFile since it is only used when retrieving the metadataDS, which is itself cached. Also with the new behaviour the metadataFile can be in a different location when the DS is first needed compared to when Installer is created.

Standard install path remains the same, but if the currently installing item is a file pointer (uses the same tests as for uninstall) then clean out any existing trash dir, create the trash dir afresh and try moving the file pointer into it.

If anything during the final install fails, rollbackMove should move the file pointer back to it's original place.

Comment 9

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

Comment on attachment 212324 [details] [diff] [review]
Remove file pointer before installing new version

Looks good and thanks for taking this on. There are still a couple of issues with the logic in safeInstallOperation but they have nothing to do with this bug and I don't believe this patch is adversely affected by it.

Comment 10

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

Checked in on trunk.

Comment 11

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

Checked in on MOZILLA_1_8_BRANCH