Fix user-level Chrome self-destruction issues on Windows 8.

Remove self-destruct message box (causing hang in Metro).

Launch self-destruct setup out of job (so that it doesn't die when killing the chrome.exe which created it...).

And be more drastic about killing Chrome processes on force-uninstall (pure kill, no wait).


BUG=158632, 159995
TEST=Install system-level over user-level (with user-level set to run in Metro mode); next run should self-destruct user-level Chrome (silently); subsequent launches should launch system-level Chrome.
This should also fix users currently in this bad state (upon updating the user-level Chrome which will then be able to self-destruct properly).

This might also solve bug 159995, will need to test with official build (it definitely solves the part where when both Chromes are present, user-level is activated, but hangs on the self-destruct message (which can't be displayed in Metro) -- the stronger kill process in this CL might also help with the user-level binaries left behind issue).


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

[gab, 2012-11-16 19:42:44]

Sirs, this is going to stable so I'd like a double-review (although this is
fairly clever after all)...

Thanks,
Gab

[robertshield, 2012-11-16 20:23:33]

https://codereview.chromium.org/11417036/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main_win.cc (left):

https://codereview.chromium.org/11417036/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main_win.cc:368: ui::MessageBox(NULL, text,
caption, flags);
Let's only remove this on Win8/metro for now. We can take it out when we get the
auto-launching of system level in.

[grt (UTC plus 2), 2012-11-16 21:11:24]

https://codereview.chromium.org/11417036/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main_win.cc (left):

https://codereview.chromium.org/11417036/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main_win.cc:368: ui::MessageBox(NULL, text,
caption, flags);
On 2012/11/16 20:23:33, robertshield wrote:
> Let's only remove this on Win8/metro for now. We can take it out when we get
the
> auto-launching of system level in.

what he said.

https://codereview.chromium.org/11417036/diff/1/chrome/installer/setup/uninst...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/11417036/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:190:
base::CleanupProcesses(installer::kChromeExe, base::TimeDelta(),
can this be made to kill only the chrome being uninstalled so that it doesn't
kill canaries or kill regular chrome when a sxs install of chrome frame is being
uninstalled?

https://codereview.chromium.org/11417036/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:194: }
how about killing delegate_execute.exe, too?

[gab, 2012-11-20 00:46:36]

PTAL.

CloseChromeFrameHelperProcess() already takes care of the Chrome Frame related
processes; however it only does so if uninstalling Chrome Frame (which I guess
is fine as if Chrome Frame is present and we are not uninstalling it; then the
binaries not be deleted anyways, right?).

As for delegate_execute.exe, I added it, but see comment below... not sure this
is the right thing to do...

https://codereview.chromium.org/11417036/diff/1/chrome/browser/chrome_browser...
File chrome/browser/chrome_browser_main_win.cc (left):

https://codereview.chromium.org/11417036/diff/1/chrome/browser/chrome_browser...
chrome/browser/chrome_browser_main_win.cc:368: ui::MessageBox(NULL, text,
caption, flags);
On 2012/11/16 21:11:24, grt wrote:
> On 2012/11/16 20:23:33, robertshield wrote:
> > Let's only remove this on Win8/metro for now. We can take it out when we get
> the
> > auto-launching of system level in.
> 
> what he said.

Done.

https://codereview.chromium.org/11417036/diff/1/chrome/installer/setup/uninst...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/11417036/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:190:
base::CleanupProcesses(installer::kChromeExe, base::TimeDelta(),
On 2012/11/16 21:11:24, grt wrote:
> can this be made to kill only the chrome being uninstalled so that it doesn't
> kill canaries or kill regular chrome when a sxs install of chrome frame is
being
> uninstalled?

As per F2F discussion, we would like to fix this, but won't merge such a change
in stable now.

https://codereview.chromium.org/11417036/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:194: }
On 2012/11/16 21:11:24, grt wrote:
> how about killing delegate_execute.exe, too?

Done. However I just thought that "delegate_execute.exe" is quite a generic name
that other programs should use, do we really want to kill this before we have a
way to kill only our processes? It doesn't stay alive after activating I believe
so it shouldn't be a big source of flakiness on uninstall anyways...

[robertshield, 2012-11-20 02:09:24]

lgtm

[grt (UTC plus 2), 2012-11-20 02:35:38]

https://codereview.chromium.org/11417036/diff/1/chrome/installer/setup/uninst...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/11417036/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:190:
base::CleanupProcesses(installer::kChromeExe, base::TimeDelta(),
On 2012/11/20 00:46:36, gab wrote:
> On 2012/11/16 21:11:24, grt wrote:
> > can this be made to kill only the chrome being uninstalled so that it
doesn't
> > kill canaries or kill regular chrome when a sxs install of chrome frame is
> being
> > uninstalled?
> 
> As per F2F discussion, we would like to fix this, but won't merge such a
change
> in stable now.

oh yeah. thanks for the reminder.

https://codereview.chromium.org/11417036/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:194: }
On 2012/11/20 00:46:36, gab wrote:
> On 2012/11/16 21:11:24, grt wrote:
> > how about killing delegate_execute.exe, too?
> 
> Done. However I just thought that "delegate_execute.exe" is quite a generic
name
> that other programs should use, do we really want to kill this before we have
a
> way to kill only our processes? It doesn't stay alive after activating I
believe
> so it shouldn't be a big source of flakiness on uninstall anyways...

hmm.  that's a good point.  i think we should see the benefits from this CL
without the delegate_execute killing.

https://codereview.chromium.org/11417036/diff/8001/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/11417036/diff/8001/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_win.cc:383:
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?

[gab, 2012-11-20 15:38:06]

PTAL

https://codereview.chromium.org/11417036/diff/1/chrome/installer/setup/uninst...
File chrome/installer/setup/uninstall.cc (right):

https://codereview.chromium.org/11417036/diff/1/chrome/installer/setup/uninst...
chrome/installer/setup/uninstall.cc:194: }
On 2012/11/20 02:35:38, grt wrote:
> On 2012/11/20 00:46:36, gab wrote:
> > On 2012/11/16 21:11:24, grt wrote:
> > > how about killing delegate_execute.exe, too?
> > 
> > Done. However I just thought that "delegate_execute.exe" is quite a generic
> name
> > that other programs should use, do we really want to kill this before we
have

s/should/could

> a
> > way to kill only our processes? It doesn't stay alive after activating I
> believe
> > so it shouldn't be a big source of flakiness on uninstall anyways...
> 
> hmm.  that's a good point.  i think we should see the benefits from this CL
> without the delegate_execute killing.

Removed killing of delegate_execute.

https://codereview.chromium.org/11417036/diff/8001/chrome/browser/chrome_brow...
File chrome/browser/chrome_browser_main_win.cc (right):

https://codereview.chromium.org/11417036/diff/8001/chrome/browser/chrome_brow...
chrome/browser/chrome_browser_main_win.cc:383:
launch_options.force_breakaway_from_job_ = true;
On 2012/11/20 02:35:38, 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?

Done.

[grt (UTC plus 2), 2012-11-20 15:57:21]

lgtm

[commit-bot: I haz the power, 2012-11-20 20:57:10]

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

[commit-bot: I haz the power, 2012-11-20 20:57:13]

Presubmit check for 11417036-1004 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

[gab, 2012-11-21 17:07:49]

@Nico for chrome/browser/chrome_browser_main_win.cc

Thanks,
Gab

[Nico, 2012-11-21 17:13:53]

lgtm stamp

[commit-bot: I haz the power, 2012-11-21 19:55:36]

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

[commit-bot: I haz the power, 2012-11-21 21:07:51]

Change committed as 169130