389244 - Uninstall information of old version not cleared from registry when updating via auto update

Status: VERIFIED FIXED

(Firefox :: Installer, defect)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Inconsistent registry entries

Today I updated one of my WinXP notebooks at work from Firefox 2.0.0.4 to Firefox 2.0.0.5 via auto update. After that I noticed that the old entry for Fx 2.0.0.4 is not cleared from the registry. There is a difference in the install location which could cause this problem:

2.0.0.4: "InstallLocation"="C:\\Programme\\Mozilla Firefox"
2.0.0.5: "InstallLocation"="C:\\PROGRA~1\\Mozilla Firefox"

Perhaps the auto update thinks that it is updating Firefox into a new location? But both path are identical. So a new registry entry for uninstalling is created. From where do we get the InstallLocation? Does it return the 8.3 aka DOS path name?

Comment 1

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

The uninstaller is used to update this info after software update and performs a string comparison of the install paths. I suspect it isn't getting the full path prior to the comparison since the path should always be a long path... I'll take this.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

As I read on http://msdn2.microsoft.com/en-us/library/ms683197.aspx the path returned by GetModuleFileName could be a long or a short path. It depends on how the path of the module was specified. Due to we are using NULL for the parameter hModule we are starting the updater with a short pathname?

Comment 4

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

The updater doesn't update the registry keys... this is in the installer code (actually uninstaller) since it has to set these values anyways. Software Update launches the uninstaller (e.g. helper.exe) with a command line switch of /PostUpdate to perform the win32 specific tasks to update the registry and the uninstall.log. Bug 368587 will move this from nsPostUpdateWin.js to updater.exe to avoid the second UAC prompt on Vista. Basically what happened is that with Vista we need the process that updates the registry to be elevated and in the past this task was performed in nsPostUpdateWin.js which isn't elevated on Vista since it is part of the app. We initially went down the path of converting it all into updater.exe and found that the effort to do so was to say the least not trivial.

Anyways, this is most likely due to what I stated in comment #1 and should be fairly trivial to fix.

Comment 7

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

I haven't confirmed this but this is likely a regression from the patch in bug 370701 due to appending a "\" to the installation dir. I'll most likely fix this by adding a path comparison macro to simplify path comparisons.

Comment 8

[Krishvanth]

Dont forget Robert, that this issue occurs in Vista too..!!

Comment 9

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

The code for XP is the same as Vista. Interestingly enough, I can reproduce by manually adding the entries seen but it isn't clear how the short path was added. I have both an XP and a Vista system that have upgraded through Software Update from 2.0.0.4 to 2.0.0.5 to 2.0.0.6 that don't exhibit this bug.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Robert, it's also strange that it is not a complete short path:
"UninstallString"="C:\\PROGRA~1\\Mozilla Firefox\\uninstall\\helper.exe"

Why only the "Programme" folder has a short name and not "Mozilla Firefox"? Do we ask for the application folder and add the subfolder ourself? Can you give me a hint were I can find the code for the registry update part? Is this the right one?

http://mxr.mozilla.org/mozilla1.8/source/browser/installer/windows/ab-CD.jst#322

If yes, then I found an suspicious registry call in line 355. We are trying to retrieve 'wr.getValueString(key, "Comment")'. That's wrong. It should be called "Comments". For myself I have installed different versions of Firefox under this folder. So it runs down to the comment comparison. Due to our wrong query we don't have a identical string and don't remove the old version from the registry?

Comment 11

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

Henry, it shouldn't be a short path at all and it isn't on my systems... I'm looking for the reason as I write this.

You are looking at the old xpinstall based installer and not the NSIS installer which is in a subdir named nsis of that windows dir in cvs

Comment 12

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

NSIS's GetFullPathName is not getting the full path for "C:\PROGRA~1\Mozilla Firefox". Not sure why yet... perhaps because it is a mixed short and full path.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Seems to be the same issue like it's reported here:
http://forums.winamp.com/showthread.php?s=7979a31ae29b3e2f027bfedb959db877&threadid=218336

Comment 14

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

This particular reg key should be a long path to start out with... not sure why but I'll add a check using short paths and possibly some other checks

Comment 15

[Henrik Skupin [:whimboo][⌚️UTC+1]]

The same a saw today on my main pc at work. I'm using nightly builds from 1.8 branch. Now the older 2.0.0.5pre information still resist in the registry. It's the same issue. The older version has a short path inside the registry while the updated one hasn't.

Comment 16

[Krishvanth]

I've been noticing this in both my computers (one XP and other Vista) at least from version 2.0.0.2 or so... first it occurred in XP, didn't come in Vista and later, around 2.0.0.4 it started showing up in both the OS's.

Comment 17

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

Attachment: branch patch in progress

Comment 18

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

Attachment: minimal branch patch

I'll file followup bugs on other areas I see that should use the GetLongPath macro. The only difference for trunk will be a change to Seamokey's installer which I'll also patch in this bug.

Comment 19

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

Attachment: patch - Seamonkey (Checked in - not needed for branch)

This additional macro is needed to fix this bug in the RegCleanMain and RegCleanUninstall macros.

btw: I'm working on way to make these types of changes across all apps in toolkit but it is never as simple as one would like it to be.

Comment 20

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

Comment on attachment 276754 [details] [diff] [review]
minimal branch patch

r=sspitzer

1)

The old code looks like it should have caused us problems, can you elaborate (for reference), why it didn't affect us?

-      IntOp $R8 $R8 + 1
+      IntOp $R8 $R8 - 1 ; Decrement the counter on successful deletion

2)

Can you add a comment about why this value is 1024?

+      System::Call 'kernel32::GetLongPathNameA(t r18, t .r17, i 1024)i .r16'

3)

do you have some ideas about how we could unit test macros like your new GetLongPath()?

Comment 21

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

Comment on attachment 276755 [details] [diff] [review]
patch - Seamonkey (Checked in - not needed for branch)

r=sspitzer, otherwise we'll break the seamonkey installer without this.

Comment 22

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

(In reply to comment #20)
> (From update of attachment 276754 [details] [diff] [review])
> r=sspitzer
> 
> 1)
> 
> The old code looks like it should have caused us problems, can you elaborate
> (for reference), why it didn't affect us?
> 
> -      IntOp $R8 $R8 + 1
> +      IntOp $R8 $R8 - 1 ; Decrement the counter on successful deletion
We typically only have one other entry under uninstall during a major update so the counter being incremented wouldn't matter and over time it would catch the additional entry. The reason this was added was due to the xpinstall based installer leaving behind entries as this bug does but for entirely different reasons. This is why this incorrect counter increment was never reported as a bug.

> 2)
> 
> Can you add a comment about why this value is 1024?
> 
> +      System::Call 'kernel32::GetLongPathNameA(t r18, t .r17, i 1024)i .r16'
Will do.

> 3)
> 
> do you have some ideas about how we could unit test macros like your new
> GetLongPath()?
I've been thinking and have a couple of ideas about adding some unit tests for the installer macros. I'm not terribly motivated to add them since the areas that often break will be impossible to test without modifying the registry of the system doing the test which I've been told we won't do with the tinderbox and that we are planning on moving to MSI's after 3.0 so the lifespan of these tests would be relatively short.

Comment 23

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

Attachment: Trunk patch - as checked in

Comment 24

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

Attachment: branch patch with comment fixed

Carrying forward sspitzer's review.

Comment 25

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

Checked in to trunk

Checking in mozilla/browser/installer/windows/nsis/installer.nsi;
/cvsroot/mozilla/browser/installer/windows/nsis/installer.nsi,v  <--  installer.nsi
new revision: 1.31; previous revision: 1.30
done
Checking in mozilla/browser/installer/windows/nsis/uninstaller.nsi;
/cvsroot/mozilla/browser/installer/windows/nsis/uninstaller.nsi,v  <--  uninstaller.nsi
new revision: 1.8; previous revision: 1.7
done
Checking in mozilla/mail/installer/windows/nsis/installer.nsi;
/cvsroot/mozilla/mail/installer/windows/nsis/installer.nsi,v  <--  installer.nsi
new revision: 1.16; previous revision: 1.15
done
Checking in mozilla/mail/installer/windows/nsis/uninstaller.nsi;
/cvsroot/mozilla/mail/installer/windows/nsis/uninstaller.nsi,v  <--  uninstaller.nsi
new revision: 1.7; previous revision: 1.6
done
Checking in mozilla/calendar/installer/windows/nsis/installer.nsi;
/cvsroot/mozilla/calendar/installer/windows/nsis/installer.nsi,v  <--  installer.nsi
new revision: 1.9; previous revision: 1.8
done
Checking in mozilla/calendar/installer/windows/nsis/uninstaller.nsi;
/cvsroot/mozilla/calendar/installer/windows/nsis/uninstaller.nsi,v  <--  uninstaller.nsi
new revision: 1.3; previous revision: 1.2
done
Checking in mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh;
/cvsroot/mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh,v  <--  common.nsh
new revision: 1.19; previous revision: 1.18
done
Checking in mozilla/suite/installer/windows/nsis/installer.nsi;
/cvsroot/mozilla/suite/installer/windows/nsis/installer.nsi,v  <--  installer.nsi
new revision: 1.5; previous revision: 1.4
done
Checking in mozilla/suite/installer/windows/nsis/uninstaller.nsi;
/cvsroot/mozilla/suite/installer/windows/nsis/uninstaller.nsi,v  <--  uninstaller.nsi
Checking in mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh;
/cvsroot/mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh,v  <--  common.nsh
new revision: 1.20; previous revision: 1.19
done

Comment 26

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

Comment on attachment 277013 [details] [diff] [review]
branch patch with comment fixed

I'm going to let this bake a couple of days and provide QA with steps to verify this along with getting this verified prior to requesting approval1.8.1.7 on this patch.

Comment 27

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

Attachment: Reg file to create extra reg entries for verification

Steps for verification

1. Open reg file to import the registry entries
2. Open Windows Add / Remove Programs in control panel
3. Note the multiple entries for Minefield
4. Run a latest nightly installer
5. Select custom during the install
6. Select C:\Yabba Dabba Do\Mozilla Firefox as the installation directory
7. After the install completes verify the additional registry entries noted in step 3 are no longer present

Uninstall entry names
Minefield (3.0a3pre)
Minefield (3.0a4pre)
Minefield (3.0a5pre)
Minefield (3.0a6pre)

Comment 28

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Robert, do we really want this? Don't we have the opportunity anymore to install different versions of the same application in different folders? That's what I understand from your last comment and which is IMO nonsatisfying. We should only remove old registry entries of installations within the same folder.

Comment 29

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Ok, sorry. Had to have a look at the attachment first. I'll try to verify it. Thanks.

Comment 30

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

(In reply to comment #28)
>...
> We should only remove old registry entries of installations within the same
> folder.
That is what this does

Comment 31

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

Tomcat, could you please verify this is fixed? Steps for verifying are in comment #27. You might want to verify bug 393149 at the same time. Thanks!

Comment 32

[Carsten Book [:Tomcat]]

Verified fixed with the steps to reproduce from comment #27 with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a8pre) Gecko/2007082203 Minefield/3.0a8pre

I can confirm that the additional entries are no longer present after the install from 2007082203

Changing Bug to Verified Fixed.

Comment 33

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

Comment on attachment 277013 [details] [diff] [review]
branch patch with comment fixed

This prevents bogus uninstall entries by making the directory path comparison use long paths when searching the registry for entries to remove.

Comment 34

[Daniel Veditz [:dveditz]]

Comment on attachment 277013 [details] [diff] [review]
branch patch with comment fixed

approved for 1.8.1.7, a=dveditz for release-drivers

Comment 35

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

Before I land this I'd like to get approval for Bug 393149 so I can land them at the same time.

Comment 38

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

Checked in to MOZILLA_1_8_BRANCH for 1.8.1.7 (e.g. Firefox 2.0.0.7)

Checking in mozilla/browser/installer/windows/nsis/installer.nsi;
/cvsroot/mozilla/browser/installer/windows/nsis/installer.nsi,v  <--  installer.nsi
new revision: 1.3.2.26; previous revision: 1.3.2.25
done
Checking in mozilla/browser/installer/windows/nsis/uninstaller.nsi;
/cvsroot/mozilla/browser/installer/windows/nsis/uninstaller.nsi,v  <--  uninstaller.nsi
new revision: 1.1.2.5; previous revision: 1.1.2.4
done
Checking in mozilla/mail/installer/windows/nsis/installer.nsi;
/cvsroot/mozilla/mail/installer/windows/nsis/installer.nsi,v  <--  installer.nsi
new revision: 1.1.2.13; previous revision: 1.1.2.12
done
Checking in mozilla/mail/installer/windows/nsis/uninstaller.nsi;
/cvsroot/mozilla/mail/installer/windows/nsis/uninstaller.nsi,v  <--  uninstaller.nsi
new revision: 1.1.2.5; previous revision: 1.1.2.4
done
Checking in mozilla/calendar/installer/windows/nsis/installer.nsi;
/cvsroot/mozilla/calendar/installer/windows/nsis/installer.nsi,v  <--  installer.nsi
new revision: 1.1.2.10; previous revision: 1.1.2.9
done
Checking in mozilla/calendar/installer/windows/nsis/uninstaller.nsi;
/cvsroot/mozilla/calendar/installer/windows/nsis/uninstaller.nsi,v  <--  uninstaller.nsi
new revision: 1.1.2.4; previous revision: 1.1.2.3
done
Checking in mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh;
/cvsroot/mozilla/toolkit/mozapps/installer/windows/nsis/common.nsh,v  <--  common.nsh
new revision: 1.2.2.15; previous revision: 1.2.2.14
done

Comment 39

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Verified with latest daily updates for Bon Echo and Thunderbird 2.0.0.7pre. Thanks Robert.

Comment 40

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Sorry that I have to reopen this bug. But today I applied the 2.0.0.7 update on my notebook which showed the initial behavior and it's still not fixed. Should this be related to some strange registry entries?

What I've seen some minutes ago based on attachment 273404 [details]. The registry entries for version 2.0.0.5 were successfully updated to 2.0.0.7. But the other ones for 2.0.0.4 still remains within the registry. They weren't deleted. Robert, do you have any idea? I could try to import the attachment mentioned above at home and trying to get it reproduced there. But I don't have a clue.

Comment 41

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

2.0.0.7 only fixed the QuickTime bug ( Bug 395942 ) and this was pushed out to 2.0.0.8 so 2.0.0.7 doesn't have this fix.

Comment 42

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

This will be fixed in 2.0.0.8 since Bug 395942 was the only bug fixed in 2.0.0.7.

Putting back fixed and verified and changing verified to 1.8.1.8.

Please file a new bug if this isn't fixed.

Comment 43

[Daniel Veditz [:dveditz]]

Please don't change bugs to "ASSIGNED" if you are not the Assignee. Leave the status REOPENED.

Comment 44

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Robert, you are right. I changed the channel pref to 'nightly' and made an update. After that both Firefox folders were deleted and the new Bon Echo was created. Indeed, this bug is fixed.