Closed Bug 1014187 Opened 7 years ago Closed 6 years ago

Update hotfix: method/API to launch the installer and get results

Categories

(Firefox :: General, enhancement)

x86
Windows XP
enhancement
Not set
normal
Points:
8

Tracking

()

RESOLVED FIXED
Iteration:
33.2

People

(Reporter: benjamin, Assigned: gfritzsche)

References

Details

Attachments

(2 files, 5 obsolete files)

The addon hotfix will need to launch the installer and eventually distinguish the following cases:

* installation completed successfully
* user declined the UAC prompt
* installer launched but failed

In the failure case it would be nice to get some information about what failed and save the log somewhere.

This needs to work in Firefox 10+, and I'm not sure whether nsIProcess does all the things we need or whether ctypes is stable back that far.

rstrong can you take this?
Flags: needinfo?(robert.strong.bugs)
Blocks: 1014194
The launching process will need to determine this. How about I add that to the add-on hotfix that launches the installer after it is initially implemented?
Flags: needinfo?(robert.strong.bugs) → needinfo?(benjamin)
Yes, I know that the calling process needs to do the bits. I'm thinking of a JS module that provides this API and hides all the "dirt" under the covers:

function launchInstaller(installerPath, cb); // cb called with a resolution object

No promises here, we're too far back in history for that or for OS.File.

I don't really care about the ordering of these tasks; I'm hoping that we can do them in parallel.
Flags: needinfo?(benjamin)
Ok then, I will try to put something together that does essentially what needs to be done, some of what you want will need to be done by calling process (e.g. detecting elevation cancel will need to be checked when launching the process), and you get someone to polish it up and integrate it with the hotfix?
oh... I guess you are asking me to implement the majority of the add-on hotfix here except for the ui. This is a lot with everything else I am trying to get done. Meh and I'll see what I can do.
Well, I was scoping this just to "launch the installer and get back the result". The UI and data collection and fallback/retry code is separate. If that's still too big, I'll need you to walk somebody else through the details about how to detect the various cases, since my naive experiments at using the process exit code didn't capture the right details.
You would need to request elevation from nsIProcess. Then the exit code should be correct unless nsIProcess is busted. The remainder of the details are fairly well documented in bug 994882 comment 20. I'm fine with helping walk someone through this.
I don't see an API on nsIProcess for requesting elevation. Even with bug 994882 comment 20 I'm worried about the gory details and what exit codes/file checks get us the answers we need here.
Sorry, I suspect that you are correct that there would be no way to get nsIProcess to perform elevation directly. Since it is unable to then ctypes would be the way to go. Basically what would need to be done is
http://mxr.mozilla.org/mozilla-central/source/toolkit/mozapps/update/updater/updater.cpp#2811
In regards to the gory details yes, they are gory and I will do what I can to help out with that part. The installer wasn't written with this type of consumer in mind and it wouldn't make sense to me to add that complexity to it for a couple of consumers when the vast majority of consumers don't need it and there are ways (albeit painful) for those few consumers that need it to get the information they need.
I expect, since ctypes and workers are not available in all our targets, that what we should do here is create a little stub executable to do the actual launching and return a useful result code. Then we can do the actual elevation and checking in C++ without worrying about async I/O.

Then we can use async nsIProcess, which is available all the way back.
I thought Firefox 10 had ctypes and didn't think you would need workers but your call on how you'd prefer to do this.
Assignee: nobody → georg.fritzsche
Flags: firefox-backlog+
Whiteboard: p=8
Hi Georg, can you recommend if this bug should be marked as [qa-] or [qa+] for QA verification.
Status: NEW → ASSIGNED
Flags: needinfo?(georg.fritzsche)
Whiteboard: p=8 → p=8 s=it-32c-31a-30b.3 [qa?]
From my understanding here, dependent bug(s) like bug 928173 will be QA-verifyable, but not this one.
Flags: needinfo?(georg.fritzsche)
Whiteboard: p=8 s=it-32c-31a-30b.3 [qa?] → p=8 s=it-32c-31a-30b.3 [qa-]
Robert, does that look reasonable to you?
https://bitbucket.org/gfritzsche/firefox-hotfixes/commits/6863904c75ded39818e9166e678dd3cae0b1fc3b?at=default

... this will still get a cleanup tomorrow, but should cover what we need.
Flags: needinfo?(robert.strong.bugs)
It does though the file size is 327 KB seems very large for what this accomplishes. I highly suspect that doing this with NSIS would create a binary less than 70 KB in size.
Flags: needinfo?(robert.strong.bugs)
The static CRT linkage accounts for the size - without dynamic CRT linkage it goes down to 52 KB.
Static CRT linkage + optimizing for size still gets us 309 KB.
(In reply to Georg Fritzsche [:gfritzsche] from comment #17)
> without dynamic CRT linkage it goes down to 52 KB.
... that should be "with dynamic CRT linkage".
Attached patch Implement installer launcher (obsolete) — Splinter Review
No issues were brought up since the last merge into gps' repo, so putting this up for review.

The JS side of this ended up integrated in update.jsm around [1] etc., so it's not going up for review here.

[1] https://bitbucket.org/indygreg/firefox-hotfixes/src/35d01284974c4f70bedaf1b25c3893f12468eef1/v20140527.01/resource/update.jsm?at=default#cl-1424
Attachment #8437010 - Flags: review?(robert.strong.bugs)
Whiteboard: p=8 s=it-32c-31a-30b.3 [qa-] → p=8 s=33.1 [qa-]
Attached patch Implement installer launcher, v2 (obsolete) — Splinter Review
Fixed an encoding issue (and some trailing whitespace as a bonus).
Attachment #8437010 - Attachment is obsolete: true
Attachment #8437010 - Flags: review?(robert.strong.bugs)
Attachment #8437976 - Flags: review?(robert.strong.bugs)
Attached patch Implement installer launcher, v3 (obsolete) — Splinter Review
Addressed bug 1014194, comment 45.
Attachment #8437976 - Attachment is obsolete: true
Attachment #8437976 - Flags: review?(robert.strong.bugs)
Attachment #8438672 - Flags: review?(robert.strong.bugs)
Comment on attachment 8438672 [details] [diff] [review]
Implement installer launcher, v3

From bug 994882 comment #20
"For WinXP we don't try to elevate since it doesn't have the same elevation method as Windows Vista and above so the user should run the installer themselves if they don't have write access to the install directory or you can specify an alternate install directory that the user does have write access to. If you do specify an alternative directory you should create shortcuts.

For Win Vista and above if the user doesn't have UAC turned on we don't try to elevate so the above for WinXP is also true.

For the above cases you could launch the installer using runas as noted in comment #19. Then if you get elevation cancelled you will know the install failed though it won't handle the case where the username and password provided don't have write access.

On WinVista and above we try to elevate even when the user is not a member of the admin group. If they don't supply an username and password that has write access we won't have write access to the install path and the install will fail."

--------------

From bug 1014194 comment #45
"Elevation is initiated using the runas verb on Vista and above as long as UAC is enabled. Using the runas verb without UAC enabled initiates runas which will allow the user to enter other credentials.

In all instances if we already have write access to the install dir and to HKLM in the registry the installer should not be launched with the runas verb.

If the user has write access to the install dir and not to HKLM in the registry the installer can still install the files and as I see it installing the files without updating the registry would be better than leaving them on the old version. If a future update or install runs with write access it will clean up the registry at that time."

--------------

I still need to go over the patch in bug 1014194 but it seems that with the ini file generation in JavaScript now and the check for elevation code being in this binary that the end result will likely be a hybrid of the above. Could you provide details as to what happens for the various conditions touched upon above? Clearing review for now.

>diff --git a/v20140527.01/InstallerLauncher/InstallerLauncher/InstallerLauncher.exe.manifest b/v20140527.01/InstallerLauncher/InstallerLauncher/InstallerLauncher.exe.manifest
>new file mode 100644
>--- /dev/null
>+++ b/v20140527.01/InstallerLauncher/InstallerLauncher/InstallerLauncher.exe.manifest
>@@ -0,0 +1,11 @@
>+<?xml version="1.0" encoding="UTF-8" standalone="yes"?> 
>+  <assembly xmlns="urn:schemas-microsoft-com:asm.v1" manifestVersion="1.0"> 
>+	<compatibility xmlns="urn:schemas-microsoft-com:compatibility.v1">
>+	  <application>
>+		<!--The ID below indicates application support for Windows Vista -->
>+		  <supportedOS Id="{e2011457-1546-43c5-a5fe-008deee3d3f0}"/>
>+		<!--The ID below indicates application support for Windows 7 -->
>+		  <supportedOS Id="{35138b9a-5d96-4fbd-8e2d-a2440225f93a}"/>
Could you include Windows 8 and 8.1?
<supportedOS Id="{1f676c76-80e1-4239-95bb-83d0f6d0da78}"/>
<supportedOS Id="{4a2f28e3-53b9-4441-ba9c-d69d4a4a6e38}"/>
Attachment #8438672 - Flags: review?(robert.strong.bugs)
(In reply to Robert Strong [:rstrong] (use needinfo to contact me) from comment #22)
> I still need to go over the patch in bug 1014194 but it seems that with the
> ini file generation in JavaScript now and the check for elevation code being
> in this binary that the end result will likely be a hybrid of the above.
> Could you provide details as to what happens for the various conditions
> touched upon above? Clearing review for now.

Expected behavior (spotchecked, but not enough coverage to be sure about everything):
* On Windows XP the launcher doesn't use runas and just runs the installer directly
* On Vista+:
  * if UAC is enabled, we "runas" the installer for elevation
  * if UAC is disabled, we run the installer directly

Comparing to the above (again), i guess the "no elevation & no write access to install dir" path is not covered properly?
Not sure what the best option is here - maybe we can signal "no write access to install dir" to JS in that case and let it take care of running the installer visibly?
Flags: needinfo?(robert.strong.bugs)
I'm still getting the "runas" behavior on XP SP3:

https://people.mozilla.org/~gszorc/hotfix-xpsp3.webm
(In reply to Georg Fritzsche [:gfritzsche] from comment #23)
> (In reply to Robert Strong [:rstrong] (use needinfo to contact me) from
> comment #22)
> > I still need to go over the patch in bug 1014194 but it seems that with the
> > ini file generation in JavaScript now and the check for elevation code being
> > in this binary that the end result will likely be a hybrid of the above.
> > Could you provide details as to what happens for the various conditions
> > touched upon above? Clearing review for now.
> 
> Expected behavior (spotchecked, but not enough coverage to be sure about
> everything):
> * On Windows XP the launcher doesn't use runas and just runs the installer
> directly
> * On Vista+:
>   * if UAC is enabled, we "runas" the installer for elevation
>   * if UAC is disabled, we run the installer directly
> 
> Comparing to the above (again), i guess the "no elevation & no write access
> to install dir" path is not covered properly?
That, the don't try to elevate when we have write access case, and we also always try to elevate on Vista and above when UAC is turned on.

> Not sure what the best option is here - maybe we can signal "no write access
> to install dir" to JS in that case and let it take care of running the
> installer visibly?
That would be better though I am tempted to recommend just using the runas verb since installing to this location will require elevating and on Vista and above with UAC turned off and WinXP they will at least have the chance of installing into the existing location.
Flags: needinfo?(robert.strong.bugs)
Attached patch Implement installer launcher, v4 (obsolete) — Splinter Review
* Added Win8/Win8.1 compatibility
* Changed behavior per yesterdays IRC discussion
  * if install dir not writable:
    * on WinXP exit with special exit code (due to default settings on the runas dialog)
    * on Vista+ elevate/runas
  * if install dir writable:
    * on WinXP install
    * on Vista+ install & elevate if possible
Attachment #8438672 - Attachment is obsolete: true
Attachment #8442217 - Flags: review?(robert.strong.bugs)
runas default settings on WinXP SP3, which are not helpful...
I updated the hotfix to use the new exit codes.

Running this on XP SP3 with Firefox in C:\Program Files... and the launcher exits with "can't write to install dir." The hotfix treats this as a non-transient failure and uninstalls itself.

I /think/ my install is pretty vanilla. This failure doesn't seem right.
The above issue was a sharing violation on trying to open for the write-check.

Per IRC chat with gps, we also want to do the check for "install dir writable" on XP in JS, so we avoid potential double process invocations.

So we end up with:
  * if install dir not writable:
    * on WinXP the JS code did that check and passed an ini file without
      InstallDirectoryPath (i.e. not triggering silent mode)
    * on Vista+ elevate/runas
  * if install dir writable:
    * on WinXP install
    * on Vista+ install & elevate if possible
Attached patch Implement installer launcher, v5 (obsolete) — Splinter Review
* Change "install dir writable" check to try to create a temp file for writing instead of the fx binary
* Assume the JS code already checked if the install dir is writable on XP
Attachment #8442217 - Attachment is obsolete: true
Attachment #8442217 - Flags: review?(robert.strong.bugs)
Attachment #8442474 - Flags: review?(robert.strong.bugs)
The new binary coupled with JS permissions sniffing is working great for me on XP and 7!

I haven't tried with "no permissions to write" yet. But the default case (which I imagine most users will be using) does the silent install with no UAC prompt.
Comment on attachment 8442474 [details] [diff] [review]
Implement installer launcher, v5

This looks good to me. It wouldn't hurt to have someone else take a look as well since I am a tad fever-adled atm.
Attachment #8442474 - Flags: review?(robert.strong.bugs) → review+
Comment on attachment 8442474 [details] [diff] [review]
Implement installer launcher, v5

Benjamin, maybe you can take a look for sanity as well or suggest someone who could?
Attachment #8442474 - Flags: review?(benjamin)
I don't think I'm the right reviewer for this. Maybe jimm or tabraldes can as Windows experts; otherwise we should just go with the review we have.
Attachment #8442474 - Flags: review?(benjamin)
Flags: needinfo?(tabraldes)
Flags: needinfo?(jmathies)
Comment on attachment 8442474 [details] [diff] [review]
Implement installer launcher, v5

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

Overall win32 coding looked good. I think this project could stand some cleanup before it gets checked into mc though.

::: v20140527.01/InstallerLauncher/InstallerLauncher.sln
@@ +1,2 @@
> +
> +Microsoft Visual Studio Solution File, Format Version 12.00

Do we need a solution file for this? Does our make system handle the build here?

::: v20140527.01/InstallerLauncher/InstallerLauncher/InstallerLauncher.cpp
@@ +1,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +// InstallerLauncher.cpp : Defines the entry point for the application.

nit - please get rid of boiler plate code/comments visual studio creates with projects.

@@ +21,5 @@
> +// Typedefs
> +typedef std::basic_string<TCHAR> tstring;
> +typedef std::basic_ostringstream<TCHAR> tostringstream;
> +
> +// Enum

nit - better comment explaingin  what this enum is.

@@ +44,5 @@
> +TCHAR szWindowClass[kMaxLoadString];      // the main window class name
> +
> +// Forward declarations of functions included in this code module:
> +ATOM        MyRegisterClass(HINSTANCE hInstance);
> +BOOL        InitInstance(HINSTANCE, int);

^^ with a little reformatting I think you can get rid of most of the cruft.

@@ +96,5 @@
> +
> +  str = str.substr(first, last - first + 1);
> +}
> +
> +bool GetDWORDValue(HKEY key, LPCWSTR valueName, DWORD &retValue)

nit - you don't appear to call this. Plus you're using TCHAR throughout the file so the build supports both unicode and mb, however this routine is wide character specific. Kinda odd.

@@ +203,5 @@
> +  LoadString(hInstance, IDC_INSTALLERLAUNCHER, szWindowClass, kMaxLoadString);
> +  MyRegisterClass(hInstance);
> +
> +  // Perform application initialization:
> +  if (!InitInstance (hInstance, nCmdShow))

Doesn't look like you are relying on the window creation stuff here, I think you can get rid of a lot of this cruft, skip registering window classes, creating windows, etc..

@@ +209,5 @@
> +    return Exit_InitializationFailed;
> +  }
> +
> +  int argc;
> +  LPWSTR* argv = ::CommandLineToArgvW(lpCmdLine, &argc);

nit - a direct wide character call. Which is fine although you haven't used this throughout. If you want to support unicode only, just update the rest of the apis in the project. Otherwise try to stick with the same convention of not specifying, and let the headers handle it.

Also, for fun, you could manage this memeory in a util class like you did with AutoHandle. We actually have a lot of these handlers already written down in xpcom, but getting them to compile and stand alone apps like this is a pain.

@@ +419,5 @@
> +    elevated = canElevate || !canWriteToInstallDir;
> +
> +    tostringstream oss;
> +    oss << "CanWriteToInstallDir=" << std::boolalpha << canWriteToInstallDir << "\n";
> +    OutputDebugString(oss.str().c_str());

For OutputDebugString, this dumps to external viewers regardless. Should we maybe wrap these logging calls and disable the output in public releases?

The CEH has a logging routine you can use - 
http://mxr.mozilla.org/mozilla-central/source/browser/metro/shell/commandexecutehandler/

::: v20140527.01/InstallerLauncher/InstallerLauncher/ReadMe.txt
@@ +1,2 @@
> +========================================================================
> +    WIN32 APPLICATION : InstallerLauncher Project Overview

nit - delete me!

::: v20140527.01/InstallerLauncher/InstallerLauncher/Resource.h
@@ +1,3 @@
> +//{{NO_DEPENDENCIES}}
> +// Microsoft Visual C++ generated include file.
> +// Used by InstallerLauncher.rc

I think we can get rid of most of this if not all of it once we clean up all that windowing stuff in InstallerLauncher.

::: v20140527.01/InstallerLauncher/InstallerLauncher/stdafx.h
@@ +19,5 @@
> +#include <memory.h>
> +#include <tchar.h>
> +
> +
> +// TODO: reference additional headers your program requires here

nit - clip todo comment, tidy up file.
Flags: needinfo?(jmathies)
This code isn't being checked into mozilla-central. We don't have a build system for it.

See https://hg.mozilla.org/releases/firefox-hotfixes/

(Personally, I think we should merge firefox-hotfixes into mozilla-central. I like unified repositories. But it's not my call.)
(In reply to Jim Mathies [:jimm] from comment #35)
> Overall win32 coding looked good. I think this project could stand some
> cleanup before it gets checked into mc though.

It's not going into m-c as gps mentioned... but you're right, that shouldn't mean i skip on cleanup.

> @@ +209,5 @@
> > +    return Exit_InitializationFailed;
> > +  }
> > +
> > +  int argc;
> > +  LPWSTR* argv = ::CommandLineToArgvW(lpCmdLine, &argc);
> 
> nit - a direct wide character call. Which is fine although you haven't used
> this throughout. If you want to support unicode only, just update the rest
> of the apis in the project. Otherwise try to stick with the same convention
> of not specifying, and let the headers handle it.

I'm not aware of a |CommandLineToArgvA|, so i will skip on that one without any actual need.

> We actually have a lot of these handlers already written
> down in xpcom, but getting them to compile and stand alone apps like this is
> a pain.

As this is not even in m-c this doesn't seem worth it.

> @@ +419,5 @@
> > +    elevated = canElevate || !canWriteToInstallDir;
> > +
> > +    tostringstream oss;
> > +    oss << "CanWriteToInstallDir=" << std::boolalpha << canWriteToInstallDir << "\n";
> > +    OutputDebugString(oss.str().c_str());
> 
> For OutputDebugString, this dumps to external viewers regardless. Should we
> maybe wrap these logging calls and disable the output in public releases?

I don't think this is a problem - none of this is performance-critical and if you happen to have viewers running that's fine - it might even be helpful for testing.
Flags: needinfo?(tabraldes)
Implemented above feedback, except the one address in above comment.
Bonus: cleanup around the AutoHandle usage.

Jim, want to take another quick look if you approve now?
Attachment #8442474 - Attachment is obsolete: true
Attachment #8443505 - Flags: review?(jmathies)
Attachment #8443505 - Flags: review?(jmathies) → review+
Comment on attachment 8443505 [details] [diff] [review]
Implement installer launcher, v6

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

This looks really well-written. I don't have any comments beyond the minor nits below. Feel free to land without addressing them.

::: v20140527.01/InstallerLauncher/InstallerLauncher/InstallerLauncher.cpp
@@ +51,5 @@
> +  HANDLE mHandle;
> +  AutoHandle(const AutoHandle&);
> +public:
> +  AutoHandle(HANDLE h = NULL) : mHandle(h) {}
> +  ~AutoHandle() { if (mHandle) ::CloseHandle(mHandle); }

nit/pedantry: `CloseHandle` will indicate failure if it is called on 0/NULL or `INVALID_HANDLE_VALUE`, or on a handle that has already been invalidated. We don't check the return value of `CloseHandle` so we don't need to check `mHandle` before the call to `CloseHandle`.

@@ +55,5 @@
> +  ~AutoHandle() { if (mHandle) ::CloseHandle(mHandle); }
> +  HANDLE get() const { return mHandle; }
> +  HANDLE* address() { return &mHandle; }
> +  AutoHandle& operator=(HANDLE h)  {
> +    mHandle = h;

nit: Depending on how we want to use this class, it might make sense to invalidate `mHandle` before updating it here, or make it an error to call `AutoHandle::operator=` if `mHandle` is already set.

@@ +105,5 @@
> +  TOKEN_ELEVATION_TYPE elevationType;
> +  DWORD len;
> +  bool canElevate = ::GetTokenInformation(token.get(), TokenElevationType,
> +                                        &elevationType,
> +                                        sizeof(elevationType), &len) &&

nit: spacing

::: v20140527.01/InstallerLauncher/InstallerLauncher/InstallerLauncher.exe.manifest
@@ +1,1 @@
> +<?xml version="1.0" encoding="UTF-8" standalone="yes"?>

Usually we explicitly specify the requested execution level:
 <ms_asmv3:trustInfo xmlns:ms_asmv3="urn:schemas-microsoft-com:asm.v3">
   <ms_asmv3:security>
     <ms_asmv3:requestedPrivileges>
       <ms_asmv3:requestedExecutionLevel level="asInvoker" uiAccess="false" />
     </ms_asmv3:requestedPrivileges>
   </ms_asmv3:security>
 </ms_asmv3:trustInfo>

Did we leave this out on purpose?
Attachment #8443505 - Flags: review+
Iteration: --- → 33.2
Points: --- → 8
QA Whiteboard: [qa-]
Whiteboard: p=8 s=33.1 [qa-]
This is done, right?
Flags: needinfo?(georg.fritzsche)
Yes, this is done.

@tabraldes: Thanks for the remarks, i'm skipping on them as they don't affect any important behavior here.
Flags: needinfo?(georg.fritzsche)
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.