252418 - Extension Manager does not uninstall / remove / delete files when they no longer exist in newer version of extension

Status: RESOLVED FIXED

(Toolkit :: Add-ons Manager, defect)

Description

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

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040719 Firefox/0.9.1+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7) Gecko/20040719 Firefox/0.9.1+

If a file is removed from a newer version of an extension the EM does not remove
them when the extension is uninstalled. It appears that this is due to when an
extension is updated files no longer in use are not removed. Also, the Uninstall
file is being written with only the files installed during for the update so any
record of the previous files is lost as far as uninstall is concerned.

Reproducible: Always
Steps to Reproduce:
1. add a components folder with a component inside it to any extension.
2. install the extension - exit and restart.
3. install the original extension - exit and restart
4. uninstall the extension - exit and restart
5. find the extensions folder (e.g. look at the install.rdf for the guid)
6. there will be a components folder with the component inside.
Actual Results:  
The component was still located inside the components directory for the
uninstalled extension. The good news is that the extensions were not listed in
either the compreg.dat or the xpti.dat.

Expected Results:  
Removed the components directory and the root directory for the extension.

Comment 1

[Nickolay_Ponomarev]

Confirming with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7.3)
Gecko/20040917 Firefox/0.10

Could you attach a simple testcase for this bug?

Comment 2

[Peter van der Woude [:Peter6]]

a "remove all evidence" is needed.
extension preferences in prefs.js are not removed either

Comment 3

[Nickolay_Ponomarev]

"Remove all evidence" should be filed as a separate bug. I'm not sure it *is*
needed. This bug describes an actual bug in EM that needs to be fixed.

Comment 4

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

Attachment: Testcase part 1

Comment 5

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

Attachment: Testcase part 2

Comment 6

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

The testcase consists of two xpi's packaged for the new extension manager only.
They both only have one install.rdf and one .txt file that I chose to put in the
components directory. I did this to demonstrate that if you just install
Testcase part 1 (e.g. testcase1.xpi) and uninstall it the components and the
extension's (e.g. guid) directory will not be removed when uninstalling. I
believe this would be a separate bug but is important to note.

To test for this bug the following steps need to be performed:
Install testcase1.xpi and restart.
Install testcase2.xpi and restart.
Uninstall the item labeled as "Testcase 2 for Bug ID 252418" in the extension
manager.
View the contents of the directory named {5DB6B72E-5C55-47a0-AB3B-945B4023E398}
in your profile's extensions directory.
There will be a chrome and a components directory.
There will also be a file named testcase1.txt in the components directory.

The reason for the testcase1.txt still being there is that the Uninstall file
only contains the file information for the last installed xpi. Since an xpi
should be a fully functional package in regards to it not requiring files from
previous versions of installations it seems that the install process should
remove all previous files and sub-directories and then perform the installation.

Comment 7

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

I accidentally copied the description over from the install.rdf instead of the
name. The name in the Extension Manager will read Bug ID 252418 Testcase 1 or 2
depending on if testcase1.xpi or testcase2.xpi is installed at the time.

Comment 8

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

adding keyword testcase

Comment 9

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

Attachment: Updated testcase 1

Testcases updated to maxVersion 2.0... the old testcases maxVersion were 0.10

Comment 10

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

Attachment: Updated testcase 2

Comment 11

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

Attachment: patch

This patch steals existing code and with slight modifications fixes this bug by
copying the Uninstall file to Uninstall.tmp. Then when an extension is
installed / updated it checks for the existence of an Uninstall.tmp and if
found it compares the contents of the two files. What ever exists in
Uninstall.tmp that doesn't exist in Uninstall it then removes using the same
process that uninstalling an extension uses. I have also successfully tested
this with an update that removes a component. I tried to stay consistent with
the current naming scheme and used nsInstallTmpReader but I expect a better
name could be used.

To test after applying the patch follow the instructions in comment #6 and use
the updated testcases.

Not sure who to ask for sr or if this patch would need an sr so any info in
this regards would be appreciated.

Comment 12

[Benjamin Smedberg]

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

Ben G. wrote all this, and I only pretend to understand most of it. Bumping
review to him, though I can say that "Uninstall.tmp" is a bad name for this
file. Why not Uninstall.old ?

Comment 13

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

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

I'm started to take a stab at adding support for for themes as well even though
this case would be rare. I am obsoleting this patch and should have a new one
soon.

Comment 14

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

Attachment: patch

Ben - Benjamin passed the review of this patch to you so I am asking for
review. This basically takes parts of the existing code to make
nsInstallBakReader which copies the Uninstall file if it exists to
Uninstall.bak and uses that to compare against the new Uninstall file and then
removes any files that existed in the previous version of an extension that
don't exist in the new version. Thanks.

Comment 15

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

Attachment: patch

Sorry about the spam. Fixed a declaration... everything else still applies.

Comment 16

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

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

My initial testing of the patch for bug 286034 shows that it will fix this bug
and with the changes to nsExtensionManager.js.in this patch would no longer
makes sense anyway.

Comment 17

[Asa Dotzler [:asa]]

Robert, should we just resolve this bug then?

Comment 18

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

Asa, I'd prefer to wait until after the checkin for bug 286034 especially with
the number of changes and size of that patch. If for what ever reason it doesn't
fix this bug I should be able to come up with a patch within a day or two after
bug 286034 is checked in and if it does I will mark this resolved shortly after
the checkin.

Comment 19

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

The checkin for bug 286034 has fixed this.