The default bug view has changed. See this FAQ.

Updated builds won't uninstall via Windows Control Panel

VERIFIED FIXED in Firefox 2

Status

()

Firefox
General
--
critical
VERIFIED FIXED
11 years ago
11 years ago

People

(Reporter: Tomcat, Assigned: rstrong)

Tracking

({relnote, verified1.8.1})

unspecified
Firefox 2
x86
Windows 2000
relnote, verified1.8.1
Points:
---
Dependency tree / graph
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments, 13 obsolete attachments)

295.96 KB, text/plain
Details
14.19 KB, patch
Darin Fisher
: review+
Details | Diff | Splinter Review
55.93 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
58.23 KB, patch
dmose
: review+
Details | Diff | Splinter Review
66.80 KB, patch
Scott MacGregor
: review+
Details | Diff | Splinter Review
11.32 KB, patch
bsmedberg
: review+
Details | Diff | Splinter Review
202.10 KB, patch
Details | Diff | Splinter Review
211.16 KB, patch
beltzner
: approval1.8.1+
Details | Diff | Splinter Review
(Reporter)

Description

11 years ago
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
Rob, Seth: seen this before?
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

11 years ago
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.
Flags: blocking-firefox2?
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

11 years ago
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.

Summary: Uninstall via Windows Control Panel don`t work → Updated builds won't uninstall via Windows Control Panel
Flags: blocking-firefox2? → blocking-firefox2+
Keywords: relnote
Target Milestone: --- → Firefox 2
Rob, could you outline the use cases in which we know the uninstaller will fail from the Control Panel?
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.
Created attachment 234911 [details] [diff] [review]
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.
Assignee: nobody → robert.bugzilla
Status: NEW → ASSIGNED
Attachment #234911 - Flags: review?(benjamin)
Attachment #234911 - Attachment is obsolete: true
Attachment #234911 - Flags: review?(benjamin)
Created attachment 235066 [details]
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.
Created attachment 235068 [details] [diff] [review]
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.
Attachment #235068 - Flags: superreview?(darin)
Attachment #235068 - Flags: review?(benjamin)
Blocks: 344236
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.
Attachment #235068 - Flags: review?(benjamin) → review-

Updated

11 years ago
Attachment #235068 - Flags: superreview?(darin)
*sigh* there is no option to build an uninstaller with makensis... I'll try to come up with something.
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?
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.
Created attachment 235124 [details] [diff] [review]
patch for nsPostUpdateWin.js

Darin, this just contains the changes to nsPostUpdateWin.js
Attachment #235124 - Flags: review?(darin)
Created attachment 235125 [details] [diff] [review]
same as previous patch without -w

Comment 17

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

looks good to me, r=darin
Attachment #235124 - Flags: review?(darin) → review+
Created attachment 235379 [details] [diff] [review]
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.
Attachment #235379 - Flags: review?(benjamin)
Created attachment 235381 [details] [diff] [review]
patch build changes

This builds both the installer and uninstaller when building with MOZ_INSTALLER and both makensis and iconv are present.
Attachment #235381 - Flags: review?(benjamin)
Created attachment 235382 [details] [diff] [review]
patch - everything

This includes everything including the changes to nsPostUpdateWin.js that Darin has already reviewed.
Attachment #235379 - Flags: review?(benjamin) → review+
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.
Attachment #235381 - Flags: review?(benjamin) → review-
Created attachment 235484 [details] [diff] [review]
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.
Attachment #235381 - Attachment is obsolete: true
Attachment #235484 - Flags: review?(benjamin)
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.
Attachment #235382 - Attachment is obsolete: true
Created attachment 235500 [details] [diff] [review]
patch for Thunderbird
Created attachment 235699 [details] [diff] [review]
patch - Thunderbird
Attachment #235500 - Attachment is obsolete: true
Attachment #235699 - Flags: review?(mscott)
Comment on attachment 235699 [details] [diff] [review]
patch - Thunderbird

Scott, I also cleaned up a comment in mail/Makefile.in that no longer applies
Attachment #235699 - Attachment description: patch → patch - Thunderbird
Created attachment 235712 [details] [diff] [review]
patch - Sunbird
Attachment #235712 - Flags: review?(mattwillis)
Blocks: 350113
Blocks: 329237, 346351
Created attachment 235719 [details] [diff] [review]
patch - build config

After thinking on this for a bit I came around to being done during the build.
Attachment #235484 - Attachment is obsolete: true
Attachment #235719 - Flags: review?(benjamin)
Attachment #235484 - Flags: review?(benjamin)
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.
Attachment #235719 - Flags: review?(benjamin)
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
(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
Created attachment 235873 [details] [diff] [review]
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.
Attachment #235699 - Attachment is obsolete: true
Attachment #235712 - Attachment is obsolete: true
Attachment #235719 - Attachment is obsolete: true
Attachment #235873 - Flags: review?(benjamin)
Attachment #235712 - Flags: review?(mattwillis)
Attachment #235699 - Flags: review?(mscott)
Created attachment 235877 [details] [diff] [review]
patch - Thunderbird
Attachment #235877 - Flags: review?(mscott)
Created attachment 235880 [details] [diff] [review]
patch - Sunbird
Attachment #235880 - Flags: review?(mattwillis)
Created attachment 235881 [details] [diff] [review]
patch - Thunderbird

Forgot the new strings in the last patch
Attachment #235877 - Attachment is obsolete: true
Attachment #235881 - Flags: review?(mscott)
Attachment #235877 - Flags: review?(mscott)
Created attachment 235882 [details] [diff] [review]
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.
Attachment #235873 - Attachment is obsolete: true
Attachment #235882 - Flags: review?(benjamin)
Attachment #235873 - Flags: review?(benjamin)
Created attachment 235884 [details] [diff] [review]
patch - everything
Attachment #235068 - Attachment is obsolete: true
Attachment #235125 - Attachment is obsolete: true
Attachment #235379 - Attachment description: Patch - installer only → Patch - Firefox installer only
Whiteboard: [has patch][needs review bsmedberg, mscott, mattwillis]
Attachment #235882 - Flags: review?(benjamin) → review+

Comment 38

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

thanks a lot for making the thunderbird changes Rob.
Attachment #235881 - Flags: review?(mscott) → review+
Whiteboard: [has patch][needs review bsmedberg, mscott, mattwillis] → [has patch][needs review mattwillis]
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
Attachment #235880 - Flags: review?(mattwillis) → review+
Checked in to trunk. I'm going to wait a cycle before resolving this fixed.
Whiteboard: [has patch][needs review mattwillis]
Resolving Fixed. I'm going to let this bake a day before requesting approval for the 1.8.1 branch
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
Created attachment 236120 [details] [diff] [review]
patch for branch
Created attachment 236155 [details] [diff] [review]
patch - branch
Attachment #236120 - Attachment is obsolete: true
Attachment #236155 - Flags: approval1.8.1?
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 on attachment 236155 [details] [diff] [review]
patch - branch

a=beltzner on behalf of 181drivers
Attachment #236155 - Flags: approval1.8.1? → approval1.8.1+
Checked in to MOZILLA_1_8_BRANCH
Keywords: fixed1.8.1
(Reporter)

Comment 47

11 years ago
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.
Status: RESOLVED → VERIFIED
(Reporter)

Updated

11 years ago
Keywords: fixed1.8.1 → verified1.8.1
You need to log in before you can comment on or make changes to this bug.