338931 - Win32 installer should cleanup old / bogus registry entries on install and uninstall

Status: RESOLVED FIXED

(Firefox :: Installer, defect)

Description

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

Due to allowing multiple installs of the same app and use the app version in an attempt to keep the registry keys unique the installer is prone to leave old / unused registry keys behind. The installer and uninstaller should enumerate the registry keys looking for an install location that matches its install location and cleanup the entries it leaves behind.

Comment 1

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

I am going to fix bug 339272 and bug 338890 in this same patch since they both heavily touch the same parts of the files.

note: we do funky things in the registry... for example, when an install occurs we write to both HKLM and HKCU yet I am quite sure I had an app update only update my HKLM keys. When an app update occurs it can and does change key names so the keys in the uninstall log may be bogus or worse refer to a side by side install that happens later.

Instead of using an uninstall log for well known registry keys I am going to enumerate / validate / remove as appropriate since with the way we do things I can't rely on the uninstall log for these keys. Arghhh... but it is the right thing to do under these circumstances.

Comment 2

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

Attachment: almost there

Comment 3

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

Attachment: very close

Comment 4

[(not reading, please use [e-mail] instead)]

Comment on attachment 224671 [details] [diff] [review]
very close

I can't claim to understand everything in this patch, but robert has explained bits and pieces of how it works (and how NSIS works) and I'm slowly coming to speed.

r=sspitzer based on the little I do know.

Comment 5

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

Attachment: patch

Comment 6

[Benjamin Smedberg]

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

Ugh, all this stack manipulation is painful. Could you comment it a bit? Looking at CreateRegKey it's not at all clear to me what all the pushing/popping at the beginning/end actually does.

Comment 7

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

Will do. btw: there is another option re: CreateRegKey - instead we could create a regvalue which will auto create the key and then delete the value. Would you prefer that?

Comment 8

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

Attachment: common.nsh w/ additional comments

This just contains the changes to common.nsh. I added a section to the top of the file that hopefully explains in enough detail the whats and whys for Exch, Push, and Pop on the stack. I'll submit a complete patch after Bug 340864 lands that includes the changes required for this patch. Thanks!

Comment 9

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

Comment on attachment 224945 [details] [diff] [review]
common.nsh w/ additional comments

Benjamin, if you could let me know if the comments as they stand in common.nsh are sufficient I would appreciate it.

Comment 10

[Benjamin Smedberg]

Comment on attachment 224945 [details] [diff] [review]
common.nsh w/ additional comments

Better... my brain is still feeling the pinch.

Comment 11

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

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

Benjamin, are you ok with this patch if it contains the changes to common.nsh in the other patch you r+'d?

Also, do you want me to land this before the remove xpinstall installer patch?

Comment 12

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

Attachment: complete patch w/ mods for bug 340864

Benjamin, I wasn't sure if you wanted to review the full patch so here it is. Thanks!

Comment 13

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

Checked in on trunk

Comment 14

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

Checked in on MOZILLA_1_8_BRANCH