Close all windows in test controller shutdown

Close all windows in test controller shutdown

Content shell doesn't always call Shell::PlatformExit.  This is a
bandage patch to ensure that the BlinkTestController cleans up
in its destructor.

This came up because GpuProcessTransportFactory had a DCHECK in it
(changed temporarily to a CHECK) that all of the ui::Compositors
were not leaked.  These are owned indirectly via Shell::platform_.

R=mkwst@chromium.org
BUG=646633
Committed: https://crrev.com/1d90e03773bf7c204fe5f0af5ccff1c20e03a90e
Cr-Commit-Position: refs/heads/master@{#421591}

[enne (OOO), 2016-09-14 21:23:12]

Does this make sense? I don't really know this code, so am just trying to do
what looks reasonable for my limited understanding.

That said, it kind of seems like PlatformExit should be called more rigorously
during shutdown.  It's called in from all sorts of places (and there are CHECKs
in some platforms to make sure it's not called repeatedly), but there's not any
consistent place that it's called.

[enne (OOO), 2016-09-19 17:34:45]

enne@chromium.org changed reviewers:
+ peter@chromium.org

[enne (OOO), 2016-09-19 17:35:10]

+peter as alternate for content/shell/

[enne (OOO), 2016-09-20 17:46:38]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-20 17:47:00]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-20 18:42:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-20 18:42:10]

Dry run: This issue passed the CQ dry run.

[enne (OOO), 2016-09-21 20:46:03]

mkwst, peter: ping

Should I just find a content/ reviewer?

[Peter Beverloo, 2016-09-22 12:21:40]

lgtm, but we'll have to keep an eye out on the Android bot.

Sorry for the delay, our team was at an offsite.

https://codereview.chromium.org/2340693003/diff/1/content/shell/browser/layou...
File content/shell/browser/layout_test/blink_test_controller.cc (right):

https://codereview.chromium.org/2340693003/diff/1/content/shell/browser/layou...
content/shell/browser/layout_test/blink_test_controller.cc:258:
Shell::CloseAllWindows();
Maybe we should consider moving this to LayoutTestBrowserMain() in
//content/shell/browser/layout_test/layout_test_browser_main.cc, since we do it
there for the --check-layout-test-sys-deps flag too.

We'll have to keep an eye out on how the Android bot reacts- that function,
together with RunTests(), shut down the main runner at subtly different times.
(Android doesn't run the message loop before shutting it down all other
platforms do.)

This change means that difference will go away, since CloseAllWindows() does run
the message loop as well. We may be able to delete that difference if it goes
well.

Unfortunately none of this is covered by the CQ, but I'll be around in the next
few days to check the bot too.

[enne (OOO), 2016-09-26 21:27:10]

Ok, I moved the call to CloseAllWindows as you suggested, as it does make things
a bit more symmetrical.

Is there something that I can test locally for the Android bots? Or should I
just land and watch?

[enne (OOO), 2016-09-26 21:27:13]

The CQ bit was checked by enne@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-09-26 21:28:02]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-26 22:53:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2016-09-26 22:53:20]

Dry run: This issue passed the CQ dry run.

[Mike West, 2016-09-27 07:56:40]

Sorry, I was OOO last week and I'm still recovering.

LGTM. I think the Android bots were just nuts yesterday. :)

[Peter Beverloo, 2016-09-27 10:06:18]

On 2016/09/26 21:27:10, enne wrote:
> Ok, I moved the call to CloseAllWindows as you suggested, as it does make
things
> a bit more symmetrical.
> 
> Is there something that I can test locally for the Android bots? Or should I
> just land and watch?

Thanks!

The bot to watch is the following:

http://build.chromium.org/p/chromium.webkit/builders/WebKit%20Android%20(Nexus4)

(It runs ~190 layout tests, as long as it continues to be able to run these we
don't regress.)

As I recall, it's a Nexus 4 device running Lollipop, with no ability to update
to a newer Android version yet (crbug.com/567947). Land and watch SGTM.

[enne (OOO), 2016-09-28 17:48:40]

Thanks for the help.  Landing and watching! *crosses fingers*

[enne (OOO), 2016-09-28 17:48:42]

The CQ bit was checked by enne@chromium.org

[enne (OOO), 2016-09-28 17:48:44]

The patchset sent to the CQ was uploaded after l-g-t-m from peter@chromium.org
Link to the patchset: https://codereview.chromium.org/2340693003/#ps20001
(title: "Move close all windows call")

[commit-bot: I haz the power, 2016-09-28 17:49:03]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-09-28 18:51:45]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-09-28 18:53:59]

Description was changed from

==========
Close all windows in test controller shutdown

Content shell doesn't always call Shell::PlatformExit.  This is a
bandage patch to ensure that the BlinkTestController cleans up
in its destructor.

This came up because GpuProcessTransportFactory had a DCHECK in it
(changed temporarily to a CHECK) that all of the ui::Compositors
were not leaked.  These are owned indirectly via Shell::platform_.

R=mkwst@chromium.org
BUG=646633
==========

to

==========
Close all windows in test controller shutdown

Content shell doesn't always call Shell::PlatformExit.  This is a
bandage patch to ensure that the BlinkTestController cleans up
in its destructor.

This came up because GpuProcessTransportFactory had a DCHECK in it
(changed temporarily to a CHECK) that all of the ui::Compositors
were not leaked.  These are owned indirectly via Shell::platform_.

R=mkwst@chromium.org
BUG=646633

Committed: https://crrev.com/1d90e03773bf7c204fe5f0af5ccff1c20e03a90e
Cr-Commit-Position: refs/heads/master@{#421591}
==========

[commit-bot: I haz the power, 2016-09-28 18:54:01]

Patchset 2 (id:??) landed as
https://crrev.com/1d90e03773bf7c204fe5f0af5ccff1c20e03a90e
Cr-Commit-Position: refs/heads/master@{#421591}

[enne (OOO), 2016-09-28 20:37:48]

Everything looks green, thanks!