When changing Ready Mode state from within an IE process, use chrome_launcher...

When changing Ready Mode state from within an IE process, use chrome_launcher to invoke the ProcessLauncher, so as to not cause a UAC prompt (chrome_launcher is permitted via elevation policy).

BUG=None
TEST=Install GCF in Ready Mode on Vista, interact with it.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=72361

[amit, 2011-01-24 05:54:50]

Thanks for wrapping this up in time. Just a few things I noticed but I'll defer
proper review to others.

http://codereview.chromium.org/6355011/diff/1/chrome_frame/ready_mode/interna...
File chrome_frame/ready_mode/internal/registry_ready_mode_state.cc (right):

http://codereview.chromium.org/6355011/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:107:
base::IntegrityLevel integrity_level;
= 0 or some invalid value?

http://codereview.chromium.org/6355011/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:114: return
integrity_level == base::HIGH_INTEGRITY;
Shouldn't this check be for MEDIUM_INTEGRITY (in order for launching from low to
medium integrity)?

http://codereview.chromium.org/6355011/diff/1/chrome_frame/update_launcher.cc
File chrome_frame/update_launcher.cc (right):

http://codereview.chromium.org/6355011/diff/1/chrome_frame/update_launcher.cc...
chrome_frame/update_launcher.cc:87: ::CoUninitialize();
We should call CoUninitialize only if CoInitialize returned S_OK.

[erikwright (departed), 2011-01-24 06:20:59]

http://codereview.chromium.org/6355011/diff/1/chrome_frame/ready_mode/interna...
File chrome_frame/ready_mode/internal/registry_ready_mode_state.cc (right):

http://codereview.chromium.org/6355011/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:107:
base::IntegrityLevel integrity_level;
On 2011/01/24 05:54:50, amit wrote:
> = 0 or some invalid value?

Done.

http://codereview.chromium.org/6355011/diff/1/chrome_frame/ready_mode/interna...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:114: return
integrity_level == base::HIGH_INTEGRITY;
On 2011/01/24 05:54:50, amit wrote:
> Shouldn't this check be for MEDIUM_INTEGRITY (in order for launching from low
to
> medium integrity)?

Done.

http://codereview.chromium.org/6355011/diff/1/chrome_frame/update_launcher.cc
File chrome_frame/update_launcher.cc (right):

http://codereview.chromium.org/6355011/diff/1/chrome_frame/update_launcher.cc...
chrome_frame/update_launcher.cc:87: ::CoUninitialize();
On 2011/01/24 05:54:50, amit wrote:
> We should call CoUninitialize only if CoInitialize returned S_OK. 

According to the docs, "successful", including S_FALSE. I assume SUCCEEDED and
"successful" mean the same thing. But previously I had definitely misread it to
say "all calls must be matched".

[robertshield, 2011-01-24 15:08:42]

Looks pretty good, some comments below.

Also, please could you upload another patch, I see some of Amit's comments
replied to as "done" without updated code.
Thanks!

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/chrome_launcher...
File chrome_frame/chrome_launcher_main.cc (right):

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/chrome_launcher...
chrome_frame/chrome_launcher_main.cc:20:
update_launcher::GetUpdateCommandFromArguments(cmd_line);
Nit: prefer constructor syntax:

std::wstring update_command(
    update_launcher::GetUpdateCommandFromArguments(cmd_line));

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/chrome_launcher...
File chrome_frame/chrome_launcher_utils.cc (right):

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/chrome_launcher...
chrome_frame/chrome_launcher_utils.cc:49: const wchar_t kLauncherExeBaseName[] =
L"chrome_launcher.exe";
This is only used in the anonymous namespace above. Move it up.

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/ready_mode/inte...
File chrome_frame/ready_mode/internal/registry_ready_mode_state.cc (right):

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/ready_mode/inte...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:133: if
(handle.IsValid() && CheckProcessExitCode(handle))
This appears to wait for 5 seconds whereas chrome_launcher.exe which is called
does an INFINITE wait on the elevated command. 
This means that when failing here, we don't know whether or not the elevated
command completed.

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/update_launcher.cc
File chrome_frame/update_launcher.cc (right):

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/update_launcher...
chrome_frame/update_launcher.cc:43: args = ::CommandLineToArgvW(command_line,
&num_args);
The caller is supposed to free args when done (there's another case in
chrome_launcher.cc where this isn't done).
http://msdn.microsoft.com/en-us/library/bb776391(v=vs.85).aspx

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/update_launcher...
chrome_frame/update_launcher.cc:48: if (_wcsicmp(args[1], kUpdateCommandFlag) !=
0)
You should also check that args isn't NULL, to be safe.

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/update_launcher...
chrome_frame/update_launcher.cc:78: exit_code = 
WaitForProcessExitCode(process);
nit: one too many spaces.

[erikwright (departed), 2011-01-24 17:17:14]

Proceeding to a bit of manual testing, but ideally committing around 13h30.

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/chrome_launcher...
File chrome_frame/chrome_launcher_main.cc (right):

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/chrome_launcher...
chrome_frame/chrome_launcher_main.cc:20:
update_launcher::GetUpdateCommandFromArguments(cmd_line);
On 2011/01/24 15:08:42, robertshield wrote:
> Nit: prefer constructor syntax:
> 
> std::wstring update_command(
>     update_launcher::GetUpdateCommandFromArguments(cmd_line));

Done.

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/chrome_launcher...
File chrome_frame/chrome_launcher_utils.cc (right):

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/chrome_launcher...
chrome_frame/chrome_launcher_utils.cc:49: const wchar_t kLauncherExeBaseName[] =
L"chrome_launcher.exe";
On 2011/01/24 15:08:42, robertshield wrote:
> This is only used in the anonymous namespace above. Move it up.

Done.

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/ready_mode/inte...
File chrome_frame/ready_mode/internal/registry_ready_mode_state.cc (right):

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/ready_mode/inte...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:120: }
On 2011/01/24 06:21:00, erikwright wrote:
> On 2011/01/24 05:54:50, amit wrote:
> > Shouldn't this check be for MEDIUM_INTEGRITY (in order for launching from
low
> to
> > medium integrity)?
> 
> Done.

By "Done", I meant, I added an implementation comment to the function definition
that hopefully makes it clear why this is HIGH_INTEGRITY.

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/ready_mode/inte...
chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:133: if
(handle.IsValid() && CheckProcessExitCode(handle))
On 2011/01/24 15:08:42, robertshield wrote:
> This appears to wait for 5 seconds whereas chrome_launcher.exe which is called
> does an INFINITE wait on the elevated command. 
> This means that when failing here, we don't know whether or not the elevated
> command completed.

Yes. chrome_launcher will live as long as the elevated command. But here, we
only wait 5 seconds. The reason is that, essentially, we are blocking the UI.
The near term enhancement would be to make it asynchronous.

It is certainly possible that, after timing out, the installer will have
completed. In that case, the impact is that this browser's state will not
change, but future processes will reflect the change.

Again, this preserves the existing behaviour, which I think is acceptable for
now. This can probably be fixed in branch.

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/update_launcher.cc
File chrome_frame/update_launcher.cc (right):

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/update_launcher...
chrome_frame/update_launcher.cc:43: args = ::CommandLineToArgvW(command_line,
&num_args);
On 2011/01/24 15:08:42, robertshield wrote:
> The caller is supposed to free args when done (there's another case in
> chrome_launcher.cc where this isn't done).
> http://msdn.microsoft.com/en-us/library/bb776391%28v=vs.85%29.aspx

Done.

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/update_launcher...
chrome_frame/update_launcher.cc:48: if (_wcsicmp(args[1], kUpdateCommandFlag) !=
0)
On 2011/01/24 15:08:42, robertshield wrote:
> You should also check that args isn't NULL, to be safe.

Done.

http://codereview.chromium.org/6355011/diff/8001/chrome_frame/update_launcher...
chrome_frame/update_launcher.cc:78: exit_code = 
WaitForProcessExitCode(process);
On 2011/01/24 15:08:42, robertshield wrote:
> nit: one too many spaces.

Done.

[robertshield, 2011-01-24 19:13:03]

LGTM

On 2011/01/24 17:17:14, erikwright wrote:
> Proceeding to a bit of manual testing, but ideally committing around 13h30.
> 
>
http://codereview.chromium.org/6355011/diff/8001/chrome_frame/chrome_launcher...
> File chrome_frame/chrome_launcher_main.cc (right):
> 
>
http://codereview.chromium.org/6355011/diff/8001/chrome_frame/chrome_launcher...
> chrome_frame/chrome_launcher_main.cc:20:
> update_launcher::GetUpdateCommandFromArguments(cmd_line);
> On 2011/01/24 15:08:42, robertshield wrote:
> > Nit: prefer constructor syntax:
> > 
> > std::wstring update_command(
> >     update_launcher::GetUpdateCommandFromArguments(cmd_line));
> 
> Done.
> 
>
http://codereview.chromium.org/6355011/diff/8001/chrome_frame/chrome_launcher...
> File chrome_frame/chrome_launcher_utils.cc (right):
> 
>
http://codereview.chromium.org/6355011/diff/8001/chrome_frame/chrome_launcher...
> chrome_frame/chrome_launcher_utils.cc:49: const wchar_t kLauncherExeBaseName[]
=
> L"chrome_launcher.exe";
> On 2011/01/24 15:08:42, robertshield wrote:
> > This is only used in the anonymous namespace above. Move it up.
> 
> Done.
> 
>
http://codereview.chromium.org/6355011/diff/8001/chrome_frame/ready_mode/inte...
> File chrome_frame/ready_mode/internal/registry_ready_mode_state.cc (right):
> 
>
http://codereview.chromium.org/6355011/diff/8001/chrome_frame/ready_mode/inte...
> chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:120: }
> On 2011/01/24 06:21:00, erikwright wrote:
> > On 2011/01/24 05:54:50, amit wrote:
> > > Shouldn't this check be for MEDIUM_INTEGRITY (in order for launching from
> low
> > to
> > > medium integrity)?
> > 
> > Done.
> 
> By "Done", I meant, I added an implementation comment to the function
definition
> that hopefully makes it clear why this is HIGH_INTEGRITY.
> 
>
http://codereview.chromium.org/6355011/diff/8001/chrome_frame/ready_mode/inte...
> chrome_frame/ready_mode/internal/registry_ready_mode_state.cc:133: if
> (handle.IsValid() && CheckProcessExitCode(handle))
> On 2011/01/24 15:08:42, robertshield wrote:
> > This appears to wait for 5 seconds whereas chrome_launcher.exe which is
called
> > does an INFINITE wait on the elevated command. 
> > This means that when failing here, we don't know whether or not the elevated
> > command completed.
> 
> Yes. chrome_launcher will live as long as the elevated command. But here, we
> only wait 5 seconds. The reason is that, essentially, we are blocking the UI.
> The near term enhancement would be to make it asynchronous.
> 
> It is certainly possible that, after timing out, the installer will have
> completed. In that case, the impact is that this browser's state will not
> change, but future processes will reflect the change.
> 
> Again, this preserves the existing behaviour, which I think is acceptable for
> now. This can probably be fixed in branch.
> 
>
http://codereview.chromium.org/6355011/diff/8001/chrome_frame/update_launcher.cc
> File chrome_frame/update_launcher.cc (right):
> 
>
http://codereview.chromium.org/6355011/diff/8001/chrome_frame/update_launcher...
> chrome_frame/update_launcher.cc:43: args = ::CommandLineToArgvW(command_line,
> &num_args);
> On 2011/01/24 15:08:42, robertshield wrote:
> > The caller is supposed to free args when done (there's another case in
> > chrome_launcher.cc where this isn't done).
> > http://msdn.microsoft.com/en-us/library/bb776391%2528v=vs.85%2529.aspx
> 
> Done.
> 
>
http://codereview.chromium.org/6355011/diff/8001/chrome_frame/update_launcher...
> chrome_frame/update_launcher.cc:48: if (_wcsicmp(args[1], kUpdateCommandFlag)
!=
> 0)
> On 2011/01/24 15:08:42, robertshield wrote:
> > You should also check that args isn't NULL, to be safe.
> 
> Done.
> 
>
http://codereview.chromium.org/6355011/diff/8001/chrome_frame/update_launcher...
> chrome_frame/update_launcher.cc:78: exit_code = 
> WaitForProcessExitCode(process);
> On 2011/01/24 15:08:42, robertshield wrote:
> > nit: one too many spaces.
> 
> Done.