Closed Bug 774144 Opened 12 years ago Closed 10 years ago

Webapp uninstallation on Windows through mozapps uninstall function

Categories

(Firefox Graveyard :: Web Apps, defect, P2)

All
Windows 7
defect

Tracking

(firefox16 wontfix)

RESOLVED FIXED
Firefox 31
Tracking Status
firefox16 --- wontfix

People

(Reporter: jsmith, Assigned: marco)

References

Details

Attachments

(1 file, 2 obsolete files)

Similar to bug 774142, except for Windows. Support uninstalling an app natively through the mozapps uninstall() function on Windows.
Anant - What are your thoughts on this?
Keywords: productwanted
Priority: -- → P2
Question: do we want to just execute the uninstaller or to directly remove the application?
Blocks: 981251
Attached patch Patch (obsolete) — Splinter Review
Assignee: nobody → mar.castelluccio
Status: NEW → ASSIGNED
Attachment #8401796 - Flags: review?(tabraldes)
Comment on attachment 8401796 [details] [diff] [review]
Patch

Review of attachment 8401796 [details] [diff] [review]:
-----------------------------------------------------------------

This patch:
 - Modifies the NSIS uninstaller to make it work in silent mode
 - Implements the uninstall function executing the uninstaller in silent mode
 - Makes the uninstall function return a promise that is resolved if the app is uninstalled correctly, rejected if it isn't

::: webapprt/win/webapp-uninstaller.nsi.in
@@ -13,2 @@
>  SetDatablockOptimize on
> -SetCompress off

This way the uninstaller size is reduced by half (80 KB instead of 160 KB).

@@ +99,5 @@
>    ; files / directories this install is responsible for.
>    ${un.ParseUninstallLog}
>  
>    ; Remove the uninstall directory that we control
> +  RmDir /r /REBOOTOK "$INSTDIR\uninstall"

This is an unrelated change, but needed to remove the "uninstall" directory that may be in use while we uninstall.

@@ +144,5 @@
>    WriteUninstaller "$1"
>  
>    ${GetParameters} $2
> +  StrCpy $2 "$2 _?=$EXEDIR"
> +  ExecWait '"$1" $2'

This is needed to make the promise in WebappOSUtils::uninstall resolve when the uninstallation is finished.
(In reply to Marco Castelluccio [:marco] from comment #4)
> @@ +144,5 @@
> >    WriteUninstaller "$1"
> >  
> >    ${GetParameters} $2
> > +  StrCpy $2 "$2 _?=$EXEDIR"
> > +  ExecWait '"$1" $2'
> 
> This is needed to make the promise in WebappOSUtils::uninstall resolve when
> the uninstallation is finished.

(The first changed line is needed because "_?= sets $INSTDIR. ... It must be the last parameter used in the command line and must not contain any quotes, even if the path contains spaces.", from http://nsis.sourceforge.net/Docs/Chapter3.html)

I hope my comments help the review.
Comment on attachment 8401796 [details] [diff] [review]
Patch

Review of attachment 8401796 [details] [diff] [review]:
-----------------------------------------------------------------

Please do the following:
  1. Correct the comment at [1]. It currently says that the installer will run at build time, but it looks like the "installer" is actually what becomes webapp-uninstaller.exe. When the "installer" runs, it writes the real uninstaller (through a call to `WriteUninstaller`) to a temp directory and then launches that.
  2. File a follow-up bug and add a code comment explaining why every app uninstall leaves behind a copy of webapp-uninstaller.exe until the user reboots
  3. Modify `getAppRegKey` so that we're not returning an array of return values (please implement one of the suggested alternatives or something else)

[1] https://mxr.mozilla.org/mozilla-central/source/webapprt/win/webapp-uninstaller.nsi.in?rev=fb30db0fa0aa#71

::: toolkit/webapps/WebappOSUtils.jsm
@@ +89,2 @@
>  #ifdef XP_WIN
> +    let [ appRegKey, isOldNamingScheme ] = this.getAppRegKey(aApp);

I'm not a fan of returning an array of values. I'd rather see something like this:
  let isOldNamingScheme = false;
  let appRegKey = this.getAppRegKeyVersion2(aApp);
  if (!appRegKey) {
    appRegKey = this.getAppRegKeyVersion1(aApp);
    isOldNamingScheme = true;
  }
  if (!appRegKey) {
    // Error
  }
  // ...

Or something like this:
  let appRegKey = this.getAppRegKey(aApp);
  if (!appRegKey) {
    // Error
    // ...
  }
  let isOldNamingScheme = (appRegKey.namingSchemeVersion == 1);
  // reg value is stored in appRegKey.value
  // ...

@@ +329,5 @@
> +
> +    return deferred.promise;
> +#elifdef XP_MACOSX
> +    return Promise.reject("Uninstallation not implemented");
> +#elifdef XP_UNIX

These Linux and Mac changes are probably meant for a different patch/bug, right?

::: webapprt/win/webapp-uninstaller.nsi.in
@@ +144,5 @@
>    WriteUninstaller "$1"
>  
>    ${GetParameters} $2
> +  StrCpy $2 "$2 _?=$EXEDIR"
> +  ExecWait '"$1" $2'

It's important to note what's going on here: The webapp-uninstaller.exe that lives in the webapp's directory (which I'll call uninstallerA) actually writes a real uninstaller (which I'll call uninstallerB) to a temporary directory and runs that. We used to exit immediately upon launching uninstallerB, but now we wait for it to finish executing.

Before, when uninstallerB would try to delete uninstallerA, the deletion would succeed because uninstallerA wasn't running. Now, when uninstallerB tries to delete uninstallerA, uninstallerA is still running and so it can't be deleted without a reboot.

Every app uninstallation is going to require a reboot if we keep this change. That's not ideal. "Require a reboot" in this case means that we'll litter the user's temp directory with copies of our uninstaller until the user reboots.

I remember trying to tackle this problem and finding that there isn't really a way for JS code to determine that the real uninstaller has finished running: The JS code has no idea where the real uninstaller gets written to (it's a randomly-generated temp file path), it doesn't know the PID of the real uninstaller, and even if it knew the PID it would be possible for the uninstaller to exit and for the PID to be reused before our JS code tries to check that the process is still running.

I'm not sure that there's a better approach than the one presented in this patch, but we should at least file a follow-up bug explaining why every uninstallation leaves around a copy of webapp-uninstaller.exe until the user reboots. Also a comment in the code would  be helpful
Attachment #8401796 - Flags: review?(tabraldes)
Attached patch Patch v2 (obsolete) — Splinter Review
(In reply to Tim Abraldes [:TimAbraldes] [:tabraldes] from comment #6)
>   2. File a follow-up bug and add a code comment explaining why every app
> uninstall leaves behind a copy of webapp-uninstaller.exe until the user
> reboots

Filed bug 994965.

> @@ +329,5 @@
> > +
> > +    return deferred.promise;
> > +#elifdef XP_MACOSX
> > +    return Promise.reject("Uninstallation not implemented");
> > +#elifdef XP_UNIX
> 
> These Linux and Mac changes are probably meant for a different patch/bug,
> right?

I've modified the uninstall function to return a promise (it used to return a boolean on Linux, nothing on Windows and Mac), so I've had to modify the other platforms' code too.
Attachment #8401796 - Attachment is obsolete: true
Attachment #8405004 - Flags: review?(tabraldes)
Comment on attachment 8405004 [details] [diff] [review]
Patch v2

Review of attachment 8405004 [details] [diff] [review]:
-----------------------------------------------------------------

::: webapprt/win/webapp-uninstaller.nsi.in
@@ +67,5 @@
>  
>  ################################################################################
>  # Install Sections
> +# The "installer" that is generated by this file is a stub that generates the
> +# uninstaller at runtime in a temp directory and launches it. 

nit: trailing whitespace

@@ +103,5 @@
> +  ; The installer is in the uninstall directory, it generates
> +  ; the uninstaller in a temp directory and waits for its
> +  ; execution. Thus, we can't remove the uninstall directory
> +  ; now and need to wait for a restart.
> +  ; See bug 994965.

Thanks for adding this comment!
Attachment #8405004 - Flags: review?(tabraldes) → review+
Attached patch Patch v2Splinter Review
Carrying r+.
Attachment #8405004 - Attachment is obsolete: true
Attachment #8405276 - Flags: review+
Keywords: checkin-needed
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #11)
> Backed out for frequent Android mochitest-4 failures.
> https://hg.mozilla.org/integration/mozilla-inbound/rev/0a1315c30090
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=37730923&tree=Mozilla-
> Inbound

Isn't that a known intermittent failure? This patch touches desktop specific code (mostly Windows), nothing that could affect Android.
20+ retriggers on the push prior without hitting it. Consistently failing afterwards.
Very strange, the WebappOSUtils::uninstall function is only called by Firefox desktop and the desktop webapp runtime: http://mxr.mozilla.org/mozilla-central/search?string=WebappOSUtils.uninstall
The import of Promise.jsm would be cross-platform, right?
(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #17)
> The import of Promise.jsm would be cross-platform, right?

Yes, but Promise.jsm is already being imported by all the modules that use WebappOSUtils.jsm, and it is imported right at startup on Android too.
If the test failing rate increased just because we import Promise.jsm in WebappOSUtils.jsm then we should most probably disable the test on Android.
No occurrences post-backout, FWIW.
With bug 996109 fixed, we never load WebappOSUtils during dom/apps tests (we're loading it lazily, but since we never use it during those tests, we never load it).
Keywords: checkin-needed
FWIW, bug 858772 started appearing on inbound again when this landed. I haven't seen it occurring otherwise in a long time (and it went away after this was backed out previously).
https://hg.mozilla.org/mozilla-central/rev/f7645d1aaaea
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 31
Jason, does this need testing before we release Firefox 31?
Flags: needinfo?(jsmith)
Don't think so.
Flags: needinfo?(jsmith)
QA Whiteboard: [qa-]
Product: Firefox → Firefox Graveyard
You need to log in before you can comment on or make changes to this bug.