735518 - Implement NSIS uninstaller for natively-installed web apps

Status: VERIFIED FIXED

(Firefox Graveyard :: Web Apps, defect)

Description

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

After an app has been installed on Windows (bug 731541), and optionally run once or more times (bug 725408), the user must be able to uninstall it.

Comment 1

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v1 - Initial implementation of Web App uninstaller

Hey Robert, we've talked about this offline a bit but seeing the patch should help cement our discussions.

The TODOs I have so far are:
  - Don't build uninstaller at build time: Use WriteUninstaller and ExecWait from the install sections
  - Revert change to common.nsh; instead of using common init function, copy the init functionality needed into the webapp uninstaller
  - Create localization strings for webapp uninstaller and modify script to use them (don't use browser strings)

Let me know what other items come up as you read through this.  Thanks!

Comment 2

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

Comment on attachment 606386 [details] [diff] [review]
Patch v1 - Initial implementation of Web App uninstaller

>+Function un.leaveConfirm
>+  ${MUI_INSTALLOPTIONS_READ} $0 "unconfirm.ini" "Settings" "State"
>+  StrCmp $0 "3" +1 continue
>+  ${MUI_INSTALLOPTIONS_READ} $0 "unconfirm.ini" "Field 3" "State"
>+  ${MUI_INSTALLOPTIONS_READ} $1 "unconfirm.ini" "Field 4" "HWND"
>+  StrCmp $0 1 +1 +3
>+  ShowWindow $1 ${SW_SHOW}
>+  Abort
>+
>+  ShowWindow $1 ${SW_HIDE}
>+  Abort
>+
>+  continue:
>+
>+  ; Try to delete the app executable and if we can't delete it try to find the
>+  ; app's message window and prompt the user to close the app. This allows
>+  ; running an instance that is located in another directory. If for whatever
>+  ; reason there is no message window we will just rename the app's files and
>+  ; then remove them on restart if they are in use.
>+  ClearErrors
>+  ${DeleteFile} "$INSTDIR\${FileMainEXE}"
>+  ${If} ${Errors}
>+    ${un.ManualCloseAppPrompt} "${WindowClass}" "$(WARN_MANUALLY_CLOSE_APP_UNINSTALL)"
You can't rely on the Firefox strings for this.

It isn't clear to me that this shouldn't just rename the stub out of the way and just put the new one in place. Need to think on this some more but at present I think that would be best for the stub in which case a lot of the complexity required for checking if the stub is in use goes away. I would just go with the confirmation page and the progress page in that case. What do you think?

>+  ${EndIf}
>+FunctionEnd
>+
>+################################################################################
>+# Initialization Functions
>+Function .onInit
>+  WriteUninstaller "$EXEDIR\${UninstallerFilename}.exe"
>+FunctionEnd
>+
>+Function un.onInit
>+  StrCpy $LANGUAGE 0
>+  ${un.UninstallUnOnInitCommon}
>+  !insertmacro InitInstallOptionsFile "unconfirm.ini"
>+
>+  ReadINIStr $AppFilename "$INSTDIR\application.ini" "WebAppRT" "Filename"
>+  ReadINIStr $ProfileDir "$INSTDIR\application.ini" "App" "Profile"
>+
>+  ${un.SetBrandNameVars} "$INSTDIR\application.ini"
Please use your own code for this since it is used specifically for distributions and will likely change in the future

>+FunctionEnd
>+
>+Function un.onGUIEnd
>+  ${un.OnEndCommon}
Please use your own code here since this requires removal of code in common.nsh that is needed by Firefox and other apps.

>diff --git a/toolkit/mozapps/installer/windows/nsis/common.nsh b/toolkit/mozapps/installer/windows/nsis/common.nsh
>index 8db20ac..dfb5037 100755
>--- a/toolkit/mozapps/installer/windows/nsis/common.nsh
>+++ b/toolkit/mozapps/installer/windows/nsis/common.nsh
>@@ -5003,9 +5003,6 @@
>     Function un.UninstallUnOnInitCommon
>       ${un.GetParent} "$INSTDIR" $INSTDIR
>       ${un.GetLongPath} "$INSTDIR" $INSTDIR
>-      ${Unless} ${FileExists} "$INSTDIR\${FileMainEXE}"
>-        Abort
>-      ${EndUnless}

btw: you can't use the strings supplied by NSIS since we have locales that NSIS doesn't so you will need to localize all strings used similar to how Firefox and other apps do though you only have a subset of the strings required by Firefox and other apps. Because of this, you shouldn't rely on the same common code used by Firefox and other apps for localization and instead should have your own that only implements the subset.

Comment 3

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v2 - Less reliant on common.nsh. Added localization.

This patch implements a bunch of the feedback I've received online and offline:
  Write uninstaller at runtime rather than build time
  Rely less on common.nsh functions; implement the pieces that are necessary in webapp-uninstaller.nsi
  Revert any modifications to common.nsh
  Remove logic for deleting profile dirs (see also bug 710790)
  If EXE is in use, rename it out of the way (see also bug 732466)
  Create localization strings for webapp uninstaller and modify script to use them (don't use browser strings)

A couple of questions came up while I was working on this:
  1) When the webapp-uninstaller executable is launched, it writes the actual uninstaller and launches it.  To have a meaningful return code from webapp-uninstaller, it has to wait for the real uninstaller to exit, then return its exit code.  One of the things that the uninstaller is supposed to do is remove webapp-uninstaller, which can't be done while webapp-uninstaller is running.  Additionally, webapp-uninstaller opens itself and allows only readers, so we can't use the standard trick of moving webapp-uninstaller to a temp directory instead of deleting it.  I don't yet see a way to have a) a webapp-uninstaller that generates our real uninstaller at runtime, b) removal of webapp-uninstaller during uninstallation, and c) a meaningful return code from (and ability to wait on the pid of) webapp-uninstaller.  It seems like we can only have 2 of the 3.
  2) preprocess-locales.py takes our 3 properties files and generates baseLocale.nlf, baseLocal.nsh, customLocale.nsh, and overrideLocale.nsh.  From what I understand, the arguments to preprocess-locales.py can be changed to allow other locales to be built.  However, the baseLocal.nsh that it generates inserts the macro MOZ_MUI_LANGUAGEFILE_END as its last line.  That macro expects a bunch of string macros to be defined, hence all of the "(ignored)" entries in mui.properties.  Is there a convenient workaround for this issue that allows us to use our existing localization framework but avoids the extra "(ignored)" entries?

Thanks for all your guidance, Robert!

Comment 4

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Tim Abraldes from comment #3)
> Created attachment 608947 [details] [diff] [review]
> Patch v2 - Less reliant on common.nsh.  Added localization.
>   2) preprocess-locales.py takes our 3 properties files and generates
> baseLocale.nlf, baseLocal.nsh, customLocale.nsh, and overrideLocale.nsh. 
> From what I understand, the arguments to preprocess-locales.py can be
> changed to allow other locales to be built.  However, the baseLocal.nsh that
> it generates inserts the macro MOZ_MUI_LANGUAGEFILE_END as its last line. 
> That macro expects a bunch of string macros to be defined, hence all of the
> "(ignored)" entries in mui.properties.  Is there a convenient workaround for
> this issue that allows us to use our existing localization framework but
> avoids the extra "(ignored)" entries?

Now that I know more about the issue, I think this may have been what you were talking about when you said "you shouldn't rely on the same common code used by Firefox and other apps for localization and instead should have your own that only implements the subset." :)

So instead of having the ".properties" files, should I just put the ".nsh" and ".nlf" files in the "locales\en-US\webapp-uninstaller" dir and copy them to "instgen" during the build?

Comment 5

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

If you choose to use MUI then will you need to implement a similar method as used in common.nsh with MOZ_MUI_LANGUAGEFILE_END, etc. but only include the strings you actually need. If you go with a simple messagebox then you can implement a different method to get the strings into the installer. In both cases you will need to use a properties file to keep l10n happy.

Comment 6

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v3 - Non-MUI

Switched to a non-MUI implementation.  Localization is accomplished entirely through the customLocale and overrideLocale files.

Comment 7

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

Comment on attachment 609527 [details] [diff] [review]
Patch v3 - Non-MUI

Went over the patch with Tim over the phone... almost there. yay!

Comment 8

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v4 - No UI. moz_webapprt temp dir. Localization.

- Reduce BrandFullName/BrandFullNameDA/BrandShortName to just one variable: AppName.
- Put EXE in a "moz_webapprt" dir in temp if it can't be deleted.
- Use Delete+Rename instead of MoveFileEx.
- Use no NSIS UI; just a MessageBox.
- Modify preprocess-locales.py to work for webapp-uninstaller and for existing apps. NOTE: override.properties/overrideLocale.nsh and custom.properties/customLocale.nsh still make sense for webapp-uninstaller (one contains the string for the title of the message box and the other contains the string for the contents of the message box).  Thus, I modified preprocess-locales.py to silently ignore when any of the 3 properties files that it looks for does not exist.  This allows us to have no mui.nsh file in webapp-uninstaller.  Robert: I think this makes sense, but are there any worrysome unintended consequences you can think of?
- Use FileFunc implementation of icon refreshing.
- Delete "moz_webapprt" dir at beginning of uninstall.  NOTE: I used "RMDIR /R" for this; I verified that even if it encounters a file it cannot delete, it removes the remaining files from the directory.

Outstanding issues:
  1) Exit codes are still not meaningful and other processes cannot reliably determine when the uninstaller has completed (we can file this as a follow-up).
  2) It seems like, for RTL languages, we should specify `MB_RTLREADING` in the MessageBox command.  I'm not yet sure how best to accomplish this.

Comment 9

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

(In reply to Tim Abraldes from comment #8)
> Created attachment 611069 [details] [diff] [review]
> Patch v4 - No UI.  moz_webapprt temp dir.  Localization.
> 
> - Reduce BrandFullName/BrandFullNameDA/BrandShortName to just one variable:
> AppName.
> - Put EXE in a "moz_webapprt" dir in temp if it can't be deleted.
> - Use Delete+Rename instead of MoveFileEx.
> - Use no NSIS UI; just a MessageBox.
> - Modify preprocess-locales.py to work for webapp-uninstaller and for
> existing apps. NOTE: override.properties/overrideLocale.nsh and
> custom.properties/customLocale.nsh still make sense for webapp-uninstaller
> (one contains the string for the title of the message box and the other
> contains the string for the contents of the message box).  Thus, I modified
> preprocess-locales.py to silently ignore when any of the 3 properties files
> that it looks for does not exist.  This allows us to have no mui.nsh file in
> webapp-uninstaller.  Robert: I think this makes sense, but are there any
> worrysome unintended consequences you can think of?
I don't see any value in having a separate file for each string whatsoever. :/

I'd just go with what we discussed where you can specify a file to generically preprocess and leave the existing code alone.

> - Use FileFunc implementation of icon refreshing.
> - Delete "moz_webapprt" dir at beginning of uninstall.  NOTE: I used "RMDIR
> /R" for this; I verified that even if it encounters a file it cannot delete,
> it removes the remaining files from the directory.
> 
> Outstanding issues:
>   1) Exit codes are still not meaningful and other processes cannot reliably
> determine when the uninstaller has completed (we can file this as a
> follow-up).
>   2) It seems like, for RTL languages, we should specify `MB_RTLREADING` in
> the MessageBox command.  I'm not yet sure how best to accomplish this.

Comment 10

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v5 - Correctly moving EXE to temp dir

Just noticed some incorrect logic.  Fixed and tested; we now correctly move the EXE to the %TEMP%\moz_webapprt directory if it is in use.  Apologies for the spam.

Comment 11

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v6 - Single properties file. --preprocess-single-file arg for preprocess-locale.py.

There is now just a single properties file for webapp-uninstaller localization.
Added a --preprocess-single-file command-line arg to preprocess-locale.py (I think the diff makes the changes look more complicated than they actually are).

Comment 12

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v7 - Cleanup. Remove defines.nsi.in - preprocess webapp-uninstaller.nsi.in directly.

Uninstaller changes from latest review.

webapp-uninstaller.nsi:
  Remove SetRegView for 64-bit registry stuff
  Remove lang_id (use 0)
  Remove unnecessary '${RemoveDir} "$INSTDIR\chrome"'
  Remove unnecessary '${RemoveDir} "$INSTDIR\uninstall"'
  Don't need to push and pop registers in .onInit
  Add "Quit" to end of .onInit
  Add a check for existence of FileMainEXE in un.onInit

webapp-uninstaller.properties:
  Swap the order of the lang strings
  Add localization note explaining AppName

webapp uninstaller makefile:
  Remove -DAB_CD
  Remove -DAPP_VERSION
    EDIT: Can't since this is used by webapp-uninstaller.nsi via defines.nsi.in

Additionally:
  Got rid of defines.nsi.in
  The define in defines.nsi.in (AppVersion) is now in webapp-uninstaller.nsi.in
  We now preprocess webapp-uninstaller.nsi.in to create webapp-uninstaller.nsi

Comment 13

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

Comment on attachment 612963 [details] [diff] [review]
Patch v7 - Cleanup.  Remove defines.nsi.in - preprocess webapp-uninstaller.nsi.in directly.

>diff --git a/webapprt/win/Makefile.in b/webapprt/win/Makefile.in
>new file mode 100644
>index 0000000..83cb9d8
>--- /dev/null
>+++ b/webapprt/win/Makefile.in
>@@ -0,0 +1,124 @@
>...
>+# Installer stuff
>+include $(topsrcdir)/toolkit/mozapps/installer/package-name.mk
>+
>+CONFIG_DIR = instgen
>+SFX_MODULE = $(topsrcdir)/other-licenses/7zstub/firefox/7zSD.sfx
>+APP_VERSION := $(shell cat $(topsrcdir)/browser/config/version.txt)
>+DEFINES += -DAPP_VERSION=$(APP_VERSION)
>+PRE_RELEASE_SUFFIX := ""
>+DEFINES += -DPRE_RELEASE_SUFFIX="$(PRE_RELEASE_SUFFIX)"
Is SFX_MODULE and PRE_RELEASE_SUFFIX actually used by web apps?

The Firefox installer will compress the NSIS binary better if it isn't in a 7-zip self extracting archive. Plus, the NSIS binary should be extremely small as in smaller than the 7-zip self extracting archive.

>diff --git a/webapprt/win/webapp-uninstaller.nsi.in b/webapprt/win/webapp-uninstaller.nsi.in
>new file mode 100644
>index 0000000..f66e082
>--- /dev/null
>+++ b/webapprt/win/webapp-uninstaller.nsi.in
>@@ -0,0 +1,161 @@

>+; Set verbosity to 3 (e.g. no script) to lessen the noise in the build logs
>+!verbose 3
>+
>+; 7-Zip provides better compression than the lzma from NSIS so we add the files
>+; uncompressed and 
s/use 7-Zip to create a SFX archive of it/let the application installer compress it.

>...
>+VIAddVersionKey "ProductName"     "${UninstallerName}"
>+VIAddVersionKey "CompanyName"     "${CompanyName}"
>+VIAddVersionKey "LegalCopyright"  "${CompanyName}"
>+VIAddVersionKey "FileVersion"     "${AppVersion}"
>+VIAddVersionKey "ProductVersion"  "${AppVersion}"
>+VIAddVersionKey "FileDescription" "${UninstallerName}"
>+VIAddVersionKey "OriginalFilename" "${UninstallerFilename}"
nit: might as well align them all or not at all.

>...
>+  ; Remove our shortcuts from start menu, desktop, and taskbar
>+  ${un.DeleteShortcuts}
>+
>+  ; Parse the uninstall log to unregister dll's and remove all installed
s/unregister dll's and //

>...
>+Function un.onInit
>+  StrCpy $LANGUAGE 0
>+
>+  ${un.GetParent} "$INSTDIR" $INSTDIR
>+  ${un.GetLongPath} "$INSTDIR" $INSTDIR
>+
>+  ReadINIStr $AppFilename "$INSTDIR\webapp.ini" "WebappRT" "Executable"
Why not just use a hardcoded value? Does this really need to be configurable via the ini file and is so, why?

I'm going to go over the changes to preprocess-locale.py tomorrow. Thanks!

Comment 14

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v8 - Cleanup

(In reply to Robert Strong [:rstrong] (do not email) from comment #13)
> [...]
> Is SFX_MODULE and PRE_RELEASE_SUFFIX actually used by web apps?

SFX_MODULE must be defined before including makensis.mk (it errors out otherwise).
PRE_RELEASE_SUFFIX seems not to be used so I've removed it.

> The Firefox installer will compress the NSIS binary better if it isn't in a
> 7-zip self extracting archive. Plus, the NSIS binary should be extremely
> small as in smaller than the 7-zip self extracting archive.
>
> [...]
> 
> s/use 7-Zip to create a SFX archive of it/let the application installer
> compress it.

Done.

> >...
> >+VIAddVersionKey "ProductName"     "${UninstallerName}"
> >+VIAddVersionKey "CompanyName"     "${CompanyName}"
> >+VIAddVersionKey "LegalCopyright"  "${CompanyName}"
> >+VIAddVersionKey "FileVersion"     "${AppVersion}"
> >+VIAddVersionKey "ProductVersion"  "${AppVersion}"
> >+VIAddVersionKey "FileDescription" "${UninstallerName}"
> >+VIAddVersionKey "OriginalFilename" "${UninstallerFilename}"
> nit: might as well align them all or not at all.

Done.  I wish I could claim that "it looked aligned in my text editor" but I guess I just missed this one :)

> >...
> >+  ; Remove our shortcuts from start menu, desktop, and taskbar
> >+  ${un.DeleteShortcuts}
> >+
> >+  ; Parse the uninstall log to unregister dll's and remove all installed
> s/unregister dll's and //
> 

Done.

> >...
> >+Function un.onInit
> >+  StrCpy $LANGUAGE 0
> >+
> >+  ${un.GetParent} "$INSTDIR" $INSTDIR
> >+  ${un.GetLongPath} "$INSTDIR" $INSTDIR
> >+
> >+  ReadINIStr $AppFilename "$INSTDIR\webapp.ini" "WebappRT" "Executable"
> Why not just use a hardcoded value? Does this really need to be configurable
> via the ini file and is so, why?

Each app's executable is named after the app.  This way the user sees, for example, "LineRage.exe," "Daves Galaxy.exe," and "Z-Type.exe" running rather than 3 copies of "webapprt.exe."  In the uninstaller we have to find out what the executable name is so we can delete it.

> I'm going to go over the changes to preprocess-locale.py tomorrow. Thanks!

Great, thanks Robert! In the meantime I'll post this patch implementing the review comments you've given me so far.

Comment 15

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

Comment on attachment 614120 [details] [diff] [review]
Patch v8 - Cleanup

>diff --git a/toolkit/mozapps/installer/windows/nsis/preprocess-locale.py b/toolkit/mozapps/installer/windows/nsis/preprocess-locale.py
>index 06e016e..53ae7aa 100644
>--- a/toolkit/mozapps/installer/windows/nsis/preprocess-locale.py
>+++ b/toolkit/mozapps/installer/windows/nsis/preprocess-locale.py
>@@ -67,26 +67,65 @@ def lookup(path, l10ndirs):
>...
>+        # Create the output files
>+        create_nlf_file(moz_dir, ab_cd, config_dir)
this is ok but it would be better if there was an option to either just create the nlf or a param to denote the nlf should be created when preprocessing a single file.

>diff --git a/webapprt/win/webapp-uninstaller.nsi.in b/webapprt/win/webapp-uninstaller.nsi.in
>new file mode 100644
>index 0000000..c8ba5c9
>--- /dev/null
>+++ b/webapprt/win/webapp-uninstaller.nsi.in
>@@ -0,0 +1,163 @@
>...
>+# Initialization Functions
>+Function .onInit
>+  GetTempFileName $0
>+  Delete $0
>+  CreateDirectory $0
>+  SetOutPath $0
When using paths in registers quote it.

r=me as long as this has gone through try and you've checked the resulting installer.

Comment 16

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

Attachment: Patch v9 - Preprocess-locale.py parameter for nlf creation. Cleanup.

(In reply to Robert Strong [:rstrong] (do not email) from comment #15)
> [...] it would be better if there was an option to either just
> create the nlf or a param to denote the nlf should be created when
> preprocessing a single file.

Done.  preprocess-locale.py now has a "--create-nlf-file" param which causes a basic nlf file to be generated.  The webapprt makefile calls preprocess-locale.py once with that param, and once with the --process-single-file param.

> >diff --git a/webapprt/win/webapp-uninstaller.nsi.in b/webapprt/win/webapp-uninstaller.nsi.in
> >new file mode 100644
> >index 0000000..c8ba5c9
> >--- /dev/null
> >+++ b/webapprt/win/webapp-uninstaller.nsi.in
> >@@ -0,0 +1,163 @@
> >...
> >+# Initialization Functions
> >+Function .onInit
> >+  GetTempFileName $0
> >+  Delete $0
> >+  CreateDirectory $0
> >+  SetOutPath $0
> When using paths in registers quote it.

Done.

> r=me as long as this has gone through try and you've checked the resulting
> installer.

I've tested the uninstaller locally and it will be run through tryserver as part of testing in bug 725408.  Thanks!

Comment 17

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

This patch was submitted as part of the patch submission in bug 725408:
  https://hg.mozilla.org/integration/mozilla-inbound/rev/ef55c163a23a

Comment 18

[Gabriela [:gaby2300]]

I verify the bug is fixed: I can see an unistaller in the apps folder.

Windows 7, nightly build 20120504

Comment 19

[Jason Smith [:jsmith]]

Marking as verified, as Gaby and I can confirm that this feature has landed. Additional bugs in regards to uninstaller shall be tracked in separate bugs.

Comment 20

[Axel Hecht [:Pike]]

I'm suprised by the ^ in

^UninstallCaption=$AppName Uninstall

in webapp-uninstaller.properties.

How did we end up with that?

Comment 21

[Tim Abraldes [:TimAbraldes] [:tabraldes]]

(In reply to Axel Hecht [:Pike] from comment #20)
> I'm suprised by the ^ in
> 
> ^UninstallCaption=$AppName Uninstall
> 
> in webapp-uninstaller.properties.
> 
> How did we end up with that?

When the browser's installer is being created, we invoke preprocess-locale.py with the `--preprocess-locale` command-line arg:
  https://mxr.mozilla.org/mozilla-central/source/browser/installer/windows/Makefile.in?rev=f4157e8c4107#96

When the webapp uninstaller is being created, we invoke preprocess-locale.py with the `--preprocess-single-file` command-line arg:
  https://mxr.mozilla.org/mozilla-central/source/webapprt/win/Makefile.in?rev=01844720b147#89

`--preprocess-locale` causes the script to look for three separate properties files and prepend '^' to the appropriate items.  Check out the file 'obj/browser/installer/windows/instgen/overrideLocale.nsh' in a local build to verify that '^'s are included in the `LangString`s.

'--preprocess-single-file' causes the script to process a single file, and the '^'s must already be present in the properties file. Check out the file 'obj/webapprt/win/instgen/webapp-uninstaller-locale.nsh' in a local build to see the file that results from processing 'webapp-uninstaller.properties'

Let me know if this makes sense! I had to do some digging to remember exactly why 'webapp-uninstaller.properties' has some '^'s in it.