Launch system-level Active Setup immediately following self-destruction of a user-level Chrome.

Launch system-level Active Setup immediately following self-destruction of a user-level Chrome.

1st attempt at https://codereview.chromium.org/11359219/ was reverted (see comments post commit on that CL).

TBR=thakis
BUG=158524
TEST=Install user-level Chrome on user A, install system-level Chrome on user B (while A is still logged in), go back to user A and run Chrome, user-level Chrome self-destructs and new shortcuts are installed pointing to system-level Chrome.
Try with: user A's chrome never ran, user A's Chrome ran and is set to prefer desktop chrome, user A's Chrome ran and is set to prefer Metro mode.

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

[gab, 2012-11-20 00:16:22]

grt can you please take a look? You came up with the suggestion originally after
commit on https://codereview.chromium.org/11359219/ (which I'm reverting as we
speak).

Thanks,
Gab

[gab, 2012-11-20 00:18:48]

On 2012/11/20 00:16:22, gab wrote:
> grt can you please take a look? You came up with the suggestion originally
after
> commit on https://codereview.chromium.org/11359219/ (which I'm reverting as we
> speak).

Err... my bad it had already been reverted because the tweak I added to use
ResolveShortcut() actually surfaced a bug in ProgramCompare I'm working on now.

> 
> Thanks,
> Gab

[grt (UTC plus 2), 2012-11-20 02:04:45]

lgtm w/ two suggestions

https://codereview.chromium.org/11420045/diff/2001/chrome/installer/setup/uni...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/11420045/diff/2001/chrome/installer/setup/uni...
chrome/installer/setup/uninstall.cc:1273: // Upon uninstalling user-level
Chrome, if system-level Chrome is present on
suggestion:
  // If user-level Chrome is being uninstalled and system-level Chrome is
  // present, launch the system-level Active Setup command to do post-install
  // tasks for this user (i.e., create shortcuts).
  if (product.is_chrome() && !installer_state.system_install() &&
      original_state.GetProductState(true, browser_dist->GetType())) {
    InstallUtil::TriggerActiveSetupCommand();
  }

https://codereview.chromium.org/11420045/diff/2001/chrome/installer/util/inst...
File chrome/installer/util/install_util.h (right):

https://codereview.chromium.org/11420045/diff/2001/chrome/installer/util/inst...
chrome/installer/util/install_util.h:35: // Attempts to trigger the command that
would be triggered for Chrome on
wdyt of:
  // Attempts to trigger the command that would be run for a system-level
  // Chrome by Active Setup. For use only when system-level Chrome is installed.
the final sentence is because the function logs an error if the active setup
key/value can't be found.

[grt (UTC plus 2), 2012-11-20 02:40:57]

Hi Julian,

I remember that you fixed the issue with jobs on Citrix.  Would you take a look
at the BREAKAWAY_FROM_JOB part of this CL?  Could this potentially be a problem
if Chrome is running on Citrix?  The motivation for the breakaway is so that the
child process runs outside of the job created by Windows 8 for metro Chrome. 
Thanks.

https://codereview.chromium.org/11420045/diff/2001/chrome/installer/util/inst...
File chrome/installer/util/install_util.cc (right):

https://codereview.chromium.org/11420045/diff/2001/chrome/installer/util/inst...
chrome/installer/util/install_util.cc:149:
launch_options.force_breakaway_from_job_ = true;
i'm a bit concerned about always doing this.  as it happens, BREAKAWAY_FROM_JOB
messes with the environment set up by Citrix (see
https://code.google.com/p/chromium/issues/detail?id=79091#c45). can you make it
so that the breakaway is only done for Win8+IsMetroProcess?  looping in 
pastarmovj@ to see what he thinks.

[pastarmovj, 2012-11-20 09:41:35]

https://codereview.chromium.org/11420045/diff/2001/chrome/installer/util/inst...
File chrome/installer/util/install_util.cc (right):

https://codereview.chromium.org/11420045/diff/2001/chrome/installer/util/inst...
chrome/installer/util/install_util.cc:149:
launch_options.force_breakaway_from_job_ = true;
On 2012/11/20 02:40:57, grt wrote:
> i'm a bit concerned about always doing this.  as it happens,
BREAKAWAY_FROM_JOB
> messes with the environment set up by Citrix (see
> https://code.google.com/p/chromium/issues/detail?id=79091#c45). can you make
it
> so that the breakaway is only done for Win8+IsMetroProcess?  looping in 
> pastarmovj@ to see what he thinks.

The BREAKAWAY_FROM_JOB flag will fail if Chrome is running as a published app
inside RDP/Citrix. Therefore we introduced the --allow-no-sandbox-job flag which
is used to hint the sanbox system to not try to assign job objects to sandboxed
processes. I would suggest to look at the code here :
content/common/sandbox_policy.cc:ShouldSetJobLevel to see how we decide if the
sandbox process should run inside a job or not. You'll see that the last checks
is exactly whether our host process is allowed to break away from its job or
not. 

In general you can either make this function more visible and call it or simply
copy parts of it as you wish and what makes the reviewer happy.

[pastarmovj, 2012-11-20 09:42:37]

https://codereview.chromium.org/11420045/diff/2001/chrome/installer/util/inst...
File chrome/installer/util/install_util.cc (right):

https://codereview.chromium.org/11420045/diff/2001/chrome/installer/util/inst...
chrome/installer/util/install_util.cc:149:
launch_options.force_breakaway_from_job_ = true;
On 2012/11/20 09:41:35, pastarmovj wrote:
> On 2012/11/20 02:40:57, grt wrote:
> > i'm a bit concerned about always doing this.  as it happens,
> BREAKAWAY_FROM_JOB
> > messes with the environment set up by Citrix (see
> > https://code.google.com/p/chromium/issues/detail?id=79091#c45). can you make
> it
> > so that the breakaway is only done for Win8+IsMetroProcess?  looping in 
> > pastarmovj@ to see what he thinks.
> 
> The BREAKAWAY_FROM_JOB flag will fail if Chrome is running as a published app

And by fail I mean your LaunchProcess will not succeed at all not that it will
run without the flag.

> inside RDP/Citrix. Therefore we introduced the --allow-no-sandbox-job flag
which
> is used to hint the sanbox system to not try to assign job objects to
sandboxed
> processes. I would suggest to look at the code here :
> content/common/sandbox_policy.cc:ShouldSetJobLevel to see how we decide if the
> sandbox process should run inside a job or not. You'll see that the last
checks
> is exactly whether our host process is allowed to break away from its job or
> not. 
> 
> In general you can either make this function more visible and call it or
simply
> copy parts of it as you wish and what makes the reviewer happy.

[gab, 2012-11-20 15:33:51]

Thanks, done.

@thakis for OWNER approval on side-effects in
chrome/browser/chrome_browser_main.cc

https://codereview.chromium.org/11420045/diff/2001/chrome/installer/setup/uni...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/11420045/diff/2001/chrome/installer/setup/uni...
chrome/installer/setup/uninstall.cc:1273: // Upon uninstalling user-level
Chrome, if system-level Chrome is present on
On 2012/11/20 02:04:45, grt wrote:
> suggestion:
>   // If user-level Chrome is being uninstalled and system-level Chrome is
>   // present, launch the system-level Active Setup command to do post-install
>   // tasks for this user (i.e., create shortcuts).
>   if (product.is_chrome() && !installer_state.system_install() &&
>       original_state.GetProductState(true, browser_dist->GetType())) {
>     InstallUtil::TriggerActiveSetupCommand();
>   }

Done.

https://codereview.chromium.org/11420045/diff/2001/chrome/installer/util/inst...
File chrome/installer/util/install_util.cc (right):

https://codereview.chromium.org/11420045/diff/2001/chrome/installer/util/inst...
chrome/installer/util/install_util.cc:149:
launch_options.force_breakaway_from_job_ = true;
On 2012/11/20 02:40:57, grt wrote:
> i'm a bit concerned about always doing this.  as it happens,
BREAKAWAY_FROM_JOB
> messes with the environment set up by Citrix (see
> https://code.google.com/p/chromium/issues/detail?id=79091#c45). can you make
it
> so that the breakaway is only done for Win8+IsMetroProcess?  looping in 
> pastarmovj@ to see what he thinks.

Done.

https://codereview.chromium.org/11420045/diff/2001/chrome/installer/util/inst...
File chrome/installer/util/install_util.h (right):

https://codereview.chromium.org/11420045/diff/2001/chrome/installer/util/inst...
chrome/installer/util/install_util.h:35: // Attempts to trigger the command that
would be triggered for Chrome on
On 2012/11/20 02:04:45, grt wrote:
> wdyt of:
>   // Attempts to trigger the command that would be run for a system-level
>   // Chrome by Active Setup. For use only when system-level Chrome is
installed.
> the final sentence is because the function logs an error if the active setup
> key/value can't be found.

Done.

[grt (UTC plus 2), 2012-11-20 15:55:37]

https://codereview.chromium.org/11420045/diff/7003/chrome/installer/util/inst...
File chrome/installer/util/install_util.h (right):

https://codereview.chromium.org/11420045/diff/7003/chrome/installer/util/inst...
chrome/installer/util/install_util.h:35: // Attempts to trigger the command that
would be ran by Active Setup for a
ran -> run

[Nico, 2012-11-20 18:07:12]

https://codereview.chromium.org/11420045/diff/7003/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11420045/diff/7003/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1206: // OS specific sections below.
Sounds like a good opportunity to work on this TODO :-)

[gab, 2012-11-20 20:42:10]

@Nico: see reply below.

Thanks,
Gab

https://codereview.chromium.org/11420045/diff/7003/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main.cc (right):

https://codereview.chromium.org/11420045/diff/7003/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main.cc:1206: // OS specific sections below.
On 2012/11/20 18:07:12, Nico wrote:
> Sounds like a good opportunity to work on this TODO :-)

Actually, this needs to merged in M24 so I want to keep the change minimal.

This piece of code will need to move anyways to solve http://crbug.com/161914
and thus will likely get out of chrome_browser_main.cc very soon.

https://codereview.chromium.org/11420045/diff/7003/chrome/installer/util/inst...
File chrome/installer/util/install_util.h (right):

https://codereview.chromium.org/11420045/diff/7003/chrome/installer/util/inst...
chrome/installer/util/install_util.h:35: // Attempts to trigger the command that
would be ran by Active Setup for a
On 2012/11/20 15:55:38, grt wrote:
> ran -> run

Done.

[Nico, 2012-11-20 20:59:30]

On 2012/11/20 20:42:10, gab wrote:
> @Nico: see reply below.
> 
> Thanks,
> Gab
> 
>
https://codereview.chromium.org/11420045/diff/7003/chrome/browser/chrome_brow...
> File chrome/browser/chrome_browser_main.cc (right):
> 
>
https://codereview.chromium.org/11420045/diff/7003/chrome/browser/chrome_brow...
> chrome/browser/chrome_browser_main.cc:1206: // OS specific sections below.
> On 2012/11/20 18:07:12, Nico wrote:
> > Sounds like a good opportunity to work on this TODO :-)
> 
> Actually, this needs to merged in M24 so I want to keep the change minimal.

This part of chrome_browser_main doesn't undergo a lot of churn, and the TODO
doesn't seem to get fixed otherwise.

In general, we follow a "marathon, not sprint" mentality in chrome.

> 
> This piece of code will need to move anyways to solve http://crbug.com/161914
> and thus will likely get out of chrome_browser_main.cc very soon.
> 
>
https://codereview.chromium.org/11420045/diff/7003/chrome/installer/util/inst...
> File chrome/installer/util/install_util.h (right):
> 
>
https://codereview.chromium.org/11420045/diff/7003/chrome/installer/util/inst...
> chrome/installer/util/install_util.h:35: // Attempts to trigger the command
that
> would be ran by Active Setup for a
> On 2012/11/20 15:55:38, grt wrote:
> > ran -> run
> 
> Done.

[gab, 2012-11-21 16:08:58]

Ok, I will do the refactoring part in this CL
(https://codereview.chromium.org/11413121/), but I would still prefer to commit
this one as is for an easier merge in M24.

Thanks,
Gab

[gab, 2012-11-21 16:48:37]

Nico sent his lgtm by email, but the code review tool doesn't seem to have
picked it up, TBRing.

Thanks,
Gab

[commit-bot: I haz the power, 2012-11-21 16:48:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gab@chromium.org/11420045/15001

[Nico, 2012-11-21 17:07:25]

and another lgtm for rietveld

[commit-bot: I haz the power, 2012-11-21 18:36:58]

Change committed as 169079