349551 - Updated builds won't uninstall via Windows Control Panel

Status: VERIFIED FIXED

(Firefox :: General, defect)

Description

[Carsten Book [:Tomcat]]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.9a1) Gecko/20060816 Mnenhy/0.7.4.10002 SeaMonkey/1.5a
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1b2) Gecko/20060821 BonEcho/2.0b2

If you update a build like Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1b1) Gecko/20060817 BonEcho/2.0b1 via the Check-Update Function and deinstall this build with the Windows Software Control Panel, get an success message, but the directory and the files are still there. Also is Bon Echo Full runable.

If you update this via an Clean Install (from the ftp Server) there are also 
3 empty directorys:

Default
Extension
Res

Reproducible: Always

Comment 1

[Mike Beltzner [:beltzner, not reading bugmail]]

Rob, Seth: seen this before?

Comment 2

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

The failure on uninstall when just using software update is due to a failure with the xpinstall based installer. This will be fixed after we get the NSIS uninstall into software update but it is not due to us switching to NSIS.

The leaving behind of the 3 empty directories is Bug 346351.

Comment 3

[Tim Riley [:timr]]

Marking blocking for FF2.  I think this should be blocking for Beta2 and I want to get it on the radar or a quick decision.  I don't mean to be over zealous, but not being able to update and un-install seems very bad.

Looking at Rob's comment #2, maybe we have not choice but to wait for the NSIS uninstall fix.

Comment 4

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

There have been numerous uninstall bugs reported with the xpinstall based installer and switching to NSIS would fix this... the only problem here is time to change the build process so that it will pick up the NSIS uninstaller which I don't have bandwidth to take on atm.

Comment 5

[Tracy Walker [:tracy]]

To clarify:  the bug here is that "updated" builds won't uninstall from the Control Panel.  Updated builds are still BE branded 2.0b2.  We're not updated to the Firefox branded 2.0b2 on the nightly channel. 

Running "uninstaller" from the application does work.

Also, fresh installs of the Firefox branded 2.0b2 builds can be uninstalled from the control Panel.

Comment 6

[Mike Beltzner [:beltzner, not reading bugmail]]

Rob, could you outline the use cases in which we know the uninstaller will fail from the Control Panel?

Comment 7

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

Bug 349462 - perhaps the same as this bug except it is exclusively with 1.5.0.x
Bug 339870 - unable to uninstall via the control panel and launching the exe directly.
Bug 338752 - unable to uninstall on Windows ME. Works after installing via the NSIS installer.
Bug 339344 - installing on top of an existing install with a different language. Only affects 1.5.0.x
Bug 329237 - VC8 C Runtime.

The cause of the xpinstall based installer's uninstall is not known for this bug. I'm looking at what it will take to get the NSIS uninstaller into the mar files.

Comment 8

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

Attachment: patch in progress

Benjamin, I'd appreciate it if you could take a look at this before it is ready to get your ok with the direction I am going with this. Thanks.

Comment 9

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

Attachment: Sample log file that needs to be migrated

There can be a ton of these log files in the uninstall directory for a nightly user. On one of my installs I had somewhere around 100 of them. The vast majority of the information necessary for the NSIS uninstaller is duplicated throughout the log files.

Comment 10

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

Attachment: patch

I tested this with over 70 log files totaling over 10 MB and it took less than 10 seconds to process them on my system. I could put a cap on the number of log files it processes but I decided not to since it is a one time shot and only affect nightly users one time plus going forward software update writes to the uninstall.log used by NSIS and thereby will avoid this.

Comment 11

[Benjamin Smedberg]

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

I'm confused. For the installer case, "setup.exe" is making uninst.exe, correct?

In the zip/MAR build case, I had thought that "makensis" was going to be making uninst.exe... is that not the case? It looks to me like uninst.exe is being made in makensis.mk, which is run *after* we package the ZIP build (unless the sequence of things has changed in tinderbox, but I'd hate to rely on that). If we're going to put something in dist/bin, it should happen during the main "make" process, not when we're building the installer.

Comment 12

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

*sigh* there is no option to build an uninstaller with makensis... I'll try to come up with something.

Comment 13

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

Hey Benjamin, since you are ok with building the uninstaller with makensis during the build process can we just create the installer during the build process and then have it generate the uninstaller?

Comment 14

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

I'm going to make it so that we can build the installer in a manner where on launch it will write the uninstaller and then launch the uninstaller it has just written.

Comment 15

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

Attachment: patch for nsPostUpdateWin.js

Darin, this just contains the changes to nsPostUpdateWin.js

Comment 16

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

Attachment: same as previous patch without -w

Comment 17

[Darin Fisher]

Comment on attachment 235124 [details] [diff] [review]
patch for nsPostUpdateWin.js

looks good to me, r=darin

Comment 18

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

Attachment: Patch - Firefox installer only

Benjamin, I separated out the installer changes from the build changes in the hope that will be easier to review. This moves all of the code for the uninstaller into uninstaller.nsi and cleans up the naming used.

Comment 19

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

Attachment: patch build changes

This builds both the installer and uninstaller when building with MOZ_INSTALLER and both makensis and iconv are present.

Comment 20

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

Attachment: patch - everything

This includes everything including the changes to nsPostUpdateWin.js that Darin has already reviewed.

Comment 21

[Benjamin Smedberg]

Comment on attachment 235381 [details] [diff] [review]
patch build changes

>Index: configure.in

>+        # Disable installer for Windows builds that use the new toolkit if NSIS
>+        # isn't in the path.
>+        AC_PATH_PROGS(MAKENSIS, makensis)
>+        if test -z "$MAKENSIS" || test "$MAKENSIS" = ":"; then
>+            MOZ_INSTALLER=
>+        fi

Instead of having hidden side-effects, put an explicit AC_MSG_ERROR here and abort.

>+        if test -z "$HOST_ICONV"; then

and here

>+    elif test -z "$MOZ_XPINSTALL"; then
>+        # Automatically disable installer if xpinstall isn't built
>+        MOZ_INSTALLER=
>+    fi

You can set this as a *default* (before the --enable/disable-installer check), but overrides are bad.

>Index: browser/installer/windows/Makefile.in

>+libs::
>+	$(MAKE) $(CONFIG_DIR)/setup.exe

This creates uninstall as a side-effect? It looks like it, but please add a comment.

Comment 22

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

Attachment: patch - for build config

Benjamin, this only changes requirements when $MOZ_INSTALLER and $MOZ_XUL_APP are non zero and $OS_ARCH = "WINNT". I really don't like changing the requirements to build since the majority of people building so they can write / submit patches don't care about building the installer. I have left everything else alone since I would prefer at the very least to limit the impact on people building on other OS's, etc. I suspect people building on Win32 will be rather annoyed by this requirement for NSIS especially when building debug, etc. where the installer can't even be packaged due to the requirement for it to be static, etc.

Comment 23

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

I think the better fix here is to generate the mar files from the installer but that is non-trivial as we discussed. Generating the mar files from the installer would allow us to build the installer and uninstaller after the build process which would remove the issue I stated in comment #22 and it should prevent inconsistencies between the files used to generate the mar files and the installer which has been a problem at times.

Comment 24

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

Attachment: patch for Thunderbird

Comment 25

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

Attachment: patch - Thunderbird

Comment 26

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

Comment on attachment 235699 [details] [diff] [review]
patch - Thunderbird

Scott, I also cleaned up a comment in mail/Makefile.in that no longer applies

Comment 27

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

Attachment: patch - Sunbird

Comment 28

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

Attachment: patch - build config

After thinking on this for a bit I came around to being done during the build.

Comment 29

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

Comment on attachment 235719 [details] [diff] [review]
patch - build config

Looks like removed-files.in will need to be processed earlier or I could just generate the uninstaller during the build and the installer afterwards as was done before.

Comment 30

[Matthew (lilmatt) Willis]

Comment on attachment 235712 [details] [diff] [review]
patch - Sunbird

> Index: calendar/Makefile.in
> +ifeq ($(OS_ARCH),WINNT)
> +ifdef MOZ_INSTALLER
> +DIRS += installer/windows
> +endif
> +endif
Why are we adding this here? We only need to create an installer for Sunbird,
and similar lines already exist in /mozilla/calendar/sunbird/Makefile.in


> Index: calendar/installer/windows/packages-static
> [@AB_CD@]
> +bin\uninstall\uninst.exe
Why is this in the AB_CD section? Is it localized?

> Index: calendar/installer/windows/nsis/installer.nsi
> -;  ${StrFilter} "${FileMainEXE}" "+" "" "" $R9
> -;  StrCpy $0 "Software\Clients\Calendar\$R9"
> -;  ${WriteRegStr2} $TmpVal "$0" "" "${BrandFullName}" 0
> -
> -;  StrCpy $0 "Software\Clients\Calendar\$R9"
> -;  ${WriteRegStr2} $TmpVal "$0" "" "${BrandFullName}" 0
> -
> -;  StrCpy $0 "Software\Clients\Calendar\$R9\Protocols\.ics"
> -;  ${WriteRegStr2} $TmpVal "$0" "" "icsfile" 0
> -;  ${WriteRegStr2} $TmpVal "$0" "Content Type" "text/calendar" 0
> -
> -;  StrCpy $0 "Software\Clients\Calendar\$R9\Protocols\.vcs"
> -;  ${WriteRegStr2} $TmpVal "$0" "" "vcsfile" 0
> -
> -  ; XXXrstrong - iCalendar File needs localization?
> -;  StrCpy $0 "Software\Clients\Calendar\$R9\Protocols\icsfile"
> -;  ${WriteRegStr2} $TmpVal "$0" "" "iCalendar File" 0
> -;  ${WriteRegDWORD2} $TmpVal "$0" "EditFlags" 0 0
> -
> -  ; XXXrstrong - no default icon for icsfile?
> -;  StrCpy $0 "Software\Clients\Calendar\$R9\Protocols\icsfile\DefaultIcon"
> -;  ${WriteRegStr2} $TmpVal "$0" "" "path to default icon,0" 0
> -
> -  ; XXXrstrong - does Sunbird have a command line handler for opening files?
> -;  StrCpy $0 "Software\Clients\Calendar\$R9\Protocols\icsfile\shell\open\command"
> -;  ${WriteRegStr2} $TmpVal "$0" "" "$INSTDIR\${FileMainEXE}" 0
> -
> -  ; XXXrstrong - vCalendar File needs localization?
> -;  StrCpy $0 "Software\Clients\Calendar\$R9\Protocols\vcsfile"
> -;  ${WriteRegStr2} $TmpVal "$0" "" "vCalendar File" 0
> -;  ${WriteRegDWORD2} $TmpVal "$0" "EditFlags" 0 0
> -
> -  ; XXXrstrong - no default icon for vcsfile?
> -;  StrCpy $0 "Software\Clients\Calendar\$R9\Protocols\vcsfile\DefaultIcon"
> -;  ${WriteRegStr2} $TmpVal "$0" "" "path to default icon,0" 0
> -
> -  ; XXXrstrong - does Sunbird have a command line handler for opening files?
> -;  StrCpy $0 "Software\Clients\Calendar\$R9\Protocols\vcsfile\shell\open\command"
> -;  ${WriteRegStr2} $TmpVal "$0" "" "$INSTDIR\${FileMainEXE}" 0
> -
> -;  StrCpy $0 "Software\Clients\Calendar\$R9\shell\open\command"
> -;  ${WriteRegStr2} $TmpVal "$0" "" "$INSTDIR\${FileMainEXE}" 0
> +  ; XXXrstrong - Add registry values for HKLM\Software\Clients\Calendar here.
> +  ; See Thunderbird's installer.nsi for an example.
Why are we removing these? I thought we wanted to enable this stuff soon?

> +; The previous installer removed directories even when they aren't empty so this
> +; funtion does as well.
Typo - function

> Index: calendar/installer/windows/nsis/uninstaller.nsi
> +  ; Refresh destop icons otherwise the start menu internet item won't be removed
Typo - desktop

Comment 31

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

(In reply to comment #30)
> (From update of attachment 235712 [details] [diff] [review] [edit])
> > Index: calendar/Makefile.in
> > +ifeq ($(OS_ARCH),WINNT)
> > +ifdef MOZ_INSTALLER
> > +DIRS += installer/windows
> > +endif
> > +endif
> Why are we adding this here? We only need to create an installer for Sunbird,
> and similar lines already exist in /mozilla/calendar/sunbird/Makefile.in
The difference with other toolkit apps caught me here.

> > Index: calendar/installer/windows/packages-static
> > [@AB_CD@]
> > +bin\uninstall\uninst.exe
> Why is this in the AB_CD section? Is it localized?
It is

> > +  ; XXXrstrong - Add registry values for HKLM\Software\Clients\Calendar here.
> > +  ; See Thunderbird's installer.nsi for an example.
> Why are we removing these? I thought we wanted to enable this stuff soon?
Because the one from Thunderbird is more recent and should be used as a guideline for adding this once the app is ready for it.

> > +; The previous installer removed directories even when they aren't empty so this
> > +; funtion does as well.
> Typo - function
fixed

> > Index: calendar/installer/windows/nsis/uninstaller.nsi
> > +  ; Refresh destop icons otherwise the start menu internet item won't be removed
> Typo - desktop
fixed

Comment 32

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

Attachment: patch - build config

Benjamin, I've tested this pretty thoroughly including locale repackaging. I am going to add the setup.ico to toolkit/mozapps/installer/windows/nsis so the NSIS installer doesn't rely on a file in a directory that will be going away soon which is the reason for the setup.ico change in makensis.mk.

Comment 33

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

Attachment: patch - Thunderbird

Comment 34

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

Attachment: patch - Sunbird

Comment 35

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

Attachment: patch - Thunderbird

Forgot the new strings in the last patch

Comment 36

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

Attachment: patch - build config (only remove uninstall dir for WINNT)

Benjamin, I've tested this pretty thoroughly including locale repackaging. I am going to add the setup.ico to toolkit/mozapps/installer/windows/nsis so the NSIS installer doesn't rely on a file in a directory that will be going away soon which is the reason for the setup.ico change in makensis.mk.

Comment 37

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

Attachment: patch - everything

Comment 38

[Scott MacGregor]

Comment on attachment 235881 [details] [diff] [review]
patch - Thunderbird

thanks a lot for making the thunderbird changes Rob.

Comment 39

[Dan Mosedale (:dmosedale, :dmose)]

Comment on attachment 235880 [details] [diff] [review]
patch - Sunbird

Since Matt was unavailable today, and landing this was urgent, I completed the review with Rob at his request.  r=dmose

Comment 40

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

Checked in to trunk. I'm going to wait a cycle before resolving this fixed.

Comment 41

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

Resolving Fixed. I'm going to let this bake a day before requesting approval for the 1.8.1 branch

Comment 42

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

Attachment: patch for branch

Comment 43

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

Attachment: patch - branch

Comment 44

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

Comment on attachment 236155 [details] [diff] [review]
patch - branch

I've tested a nightly update from a 3 month old Firefox and Thunderbird build and all appears to be working as it should.

Comment 45

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 236155 [details] [diff] [review]
patch - branch

a=beltzner on behalf of 181drivers

Comment 46

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

Checked in to MOZILLA_1_8_BRANCH

Comment 47

[Carsten Book [:Tomcat]]

Verified Fixed

Tested with Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1b1) Gecko/20060817 BonEcho/2.0b1 
and updated to 
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8.1b2) Gecko/20060901 BonEcho/2.0b2

Uninstalled via Windows Control Panel -> Removed completly.