Only do aggressive metro viewer termination in tests

Only do aggressive metro viewer termination in tests

The previous patch https://codereview.chromium.org/584513004 ran on all
metro viewer shutdowns, which would lead to Alt-F4 not being processed
which means that the proper metro process shutdown wouldn't happen,
potentially leaving the "ghost" icon.

Instead, only do the aggressive termination from the process_host side
and only when running test binaries (where we're concerned about having
the viewer be able to run again immediately.)

R=ananta@chromium.org
BUG=411147, 416356
Committed: https://crrev.com/3c19343e8829c302e8ff9421deea89b50dfb0d27
Cr-Commit-Position: refs/heads/master@{#295995}

[ananta, 2014-09-19 19:36:57]

https://codereview.chromium.org/584213002/diff/20001/win8/viewer/metro_viewer...
File win8/viewer/metro_viewer_process_host.cc (right):

https://codereview.chromium.org/584213002/diff/20001/win8/viewer/metro_viewer...
win8/viewer/metro_viewer_process_host.cc:82:
::WaitForSingleObject(viewer_process, 100);
Just TerminateProcess here?. The viewer has no state. In any case the Wait is
flaky as we have been seeing.

[scottmg, 2014-09-19 19:41:19]

https://codereview.chromium.org/584213002/diff/20001/win8/viewer/metro_viewer...
File win8/viewer/metro_viewer_process_host.cc (right):

https://codereview.chromium.org/584213002/diff/20001/win8/viewer/metro_viewer...
win8/viewer/metro_viewer_process_host.cc:82:
::WaitForSingleObject(viewer_process, 100);
On 2014/09/19 19:36:57, ananta wrote:
> Just TerminateProcess here?. The viewer has no state. In any case the Wait is
> flaky as we have been seeing.

Sure, Done.

[Shrikant Kelkar, 2014-09-19 19:42:27]

shrikant@chromium.org changed reviewers:
+ shrikant@chromium.org

[Shrikant Kelkar, 2014-09-19 19:42:28]

https://codereview.chromium.org/584213002/diff/20001/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/584213002/diff/20001/ash/test/ash_test_base.c...
ash/test/ash_test_base.cc:187: kViewerProcessArgument);
We already have some cleanup logic here, wondering why it wouldn't work? On the
other hand do we need to put this logic before test as well?

[scottmg, 2014-09-19 19:44:15]

https://codereview.chromium.org/584213002/diff/20001/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/584213002/diff/20001/ash/test/ash_test_base.c...
ash/test/ash_test_base.cc:187: kViewerProcessArgument);
On 2014/09/19 19:42:28, Shrikant Kelkar wrote:
> We already have some cleanup logic here, wondering why it wouldn't work? On
the
> other hand do we need to put this logic before test as well?

Hmm, good point. Yes, we probably could just move this higher (because it's the
host.reset() that blocks).

Do we need to do the KillAll here, or is terminating the viewer that we know is
ours sufficient?

[Shrikant Kelkar, 2014-09-19 19:49:08]

https://codereview.chromium.org/584213002/diff/20001/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/584213002/diff/20001/ash/test/ash_test_base.c...
ash/test/ash_test_base.cc:187: kViewerProcessArgument);
On 2014/09/19 19:44:15, scottmg wrote:
> On 2014/09/19 19:42:28, Shrikant Kelkar wrote:
> > We already have some cleanup logic here, wondering why it wouldn't work? On
> the
> > other hand do we need to put this logic before test as well?
> 
> Hmm, good point. Yes, we probably could just move this higher (because it's
the
> host.reset() that blocks).
> 
> Do we need to do the KillAll here, or is terminating the viewer that we know
is
> ours sufficient?

KillAll with DefaultBrowserServer should terminate only viewer, I mean all
viewers.
So is it possible that somewhere Teardown path is not getting executed if some
middle test is waiting or is it always the first test that ends up waiting?

[ananta, 2014-09-19 19:55:16]

https://codereview.chromium.org/584213002/diff/20001/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/584213002/diff/20001/ash/test/ash_test_base.c...
ash/test/ash_test_base.cc:185: const wchar_t kViewerProcessArgument[] =
L"DefaultBrowserServer";
I think we can get rid of the code below which kills viewers. The destructor is
already doing that with this patch.

[Shrikant Kelkar, 2014-09-19 20:03:12]

https://codereview.chromium.org/584213002/diff/20001/ash/test/ash_test_base.cc
File ash/test/ash_test_base.cc (right):

https://codereview.chromium.org/584213002/diff/20001/ash/test/ash_test_base.c...
ash/test/ash_test_base.cc:185: const wchar_t kViewerProcessArgument[] =
L"DefaultBrowserServer";
On 2014/09/19 19:55:16, ananta wrote:
> I think we can get rid of the code below which kills viewers. The destructor
is
> already doing that with this patch.

I feel it's better to call kill/terminate from test rather than conditionally
from MetroViewerHost.. Your call.

[scottmg, 2014-09-19 21:23:24]

PTAL, moved to TestMetroViewerProcessHost so that it's clear it's test-only, and
removed the kill-all.

https://codereview.chromium.org/584213002/diff/180001/base/test/test_process_...
File base/test/test_process_killer_win.h (left):

https://codereview.chromium.org/584213002/diff/180001/base/test/test_process_...
base/test/test_process_killer_win.h:1: // Copyright (c) 2013 The Chromium
Authors. All rights reserved.
This is now unused.

[scottmg, 2014-09-19 23:22:48]

This approach would appear to have the drawback of "not working".

(I think only on Win7 per try runs, I'm not sure what ash_unittests means on
Win7 these days?)

[scottmg, 2014-09-20 00:26:54]

Oops, was just that there's no viewer on win7 (of course).

Seem OK?

[ananta, 2014-09-20 00:32:54]

LGTM % nit

https://codereview.chromium.org/584213002/diff/120001/win8/viewer/metro_viewe...
File win8/viewer/metro_viewer_process_host.cc (right):

https://codereview.chromium.org/584213002/diff/120001/win8/viewer/metro_viewe...
win8/viewer/metro_viewer_process_host.cc:82: &viewer_process);
You need to add PROCESS_TERMINATE  to this mask

https://codereview.chromium.org/584213002/diff/220001/ash/test/test_metro_vie...
File ash/test/test_metro_viewer_process_host.cc (right):

https://codereview.chromium.org/584213002/diff/220001/ash/test/test_metro_vie...
ash/test/test_metro_viewer_process_host.cc:35: ::CloseHandle(viewer_process);
Please add a WaitForSingleObject here to ensure that the process has terminated.
As per msdn the TerminateProcess is async.

[scottmg, 2014-09-20 02:12:50]

https://codereview.chromium.org/584213002/diff/220001/ash/test/test_metro_vie...
File ash/test/test_metro_viewer_process_host.cc (right):

https://codereview.chromium.org/584213002/diff/220001/ash/test/test_metro_vie...
ash/test/test_metro_viewer_process_host.cc:35: ::CloseHandle(viewer_process);
On 2014/09/20 00:32:54, ananta wrote:
> Please add a WaitForSingleObject here to ensure that the process has
terminated.
> As per msdn the TerminateProcess is async.

Thanks, done.

[scottmg, 2014-09-20 02:14:02]

scottmg@chromium.org changed reviewers:
+ brettw@chromium.org, sky@chromium.org

[scottmg, 2014-09-20 02:14:03]

OWNERS:
+sky for ash/test/*
+brettw for base/* (all deletions)

[brettw, 2014-09-20 05:38:16]

base lgtm

[sky, 2014-09-22 16:10:58]

LGTM

[scottmg, 2014-09-22 16:25:21]

The CQ bit was checked by scottmg@chromium.org

[commit-bot: I haz the power, 2014-09-22 16:27:28]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/584213002/240001

[commit-bot: I haz the power, 2014-09-22 16:51:09]

Committed patchset #13 (id:240001) as 9a584d759473169b2ecac655ec8fbaf205dda9ed

[commit-bot: I haz the power, 2014-09-22 16:51:48]

Patchset 13 (id:??) landed as
https://crrev.com/3c19343e8829c302e8ff9421deea89b50dfb0d27
Cr-Commit-Position: refs/heads/master@{#295995}