In WinAura, also kill the Metro viewer process in AttemptExit().

In WinAura, also kill the Metro viewer process in AttemptExit().

This results in an IPC channel error in the browser process which releases Ash's reference to g_browser_process, this is necessary for the browser process to terminate.

This is a precursor to Ash browser_tests (otherwise when browser_tests try to exit Chrome by closing all browser windows, Ash stays up and the test never ends...!).

BUG=235648, 232842, 179830
TEST=
A) Local hack of Ash browser_tests works!!
B) Exit Chrome from the wrench menu on the native Desktop while Ash is running and make sure that the viewer is killed (whether it was sleeping or not) which subsequently results in the browser process going away with a clean exit :).
C) Exit Chrome from the wrench menu on the native Desktop when Ash is NOT running and make sure it closes fine (as it did before this CL).

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=201476

[gab, 2013-05-10 15:30:58]

Sir, PTAL.

Thanks!
Gab

[robertshield, 2013-05-10 16:51:48]

lg, two nits

https://codereview.chromium.org/14576015/diff/3008/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl_win.cc (right):

https://codereview.chromium.org/14576015/diff/3008/chrome/browser/browser_pro...
chrome/browser/browser_process_impl_win.cc:34: NOTREACHED();
what if this gets called twice before OnMetroViewerProcessTerminated gets
called? Would this not then be expected?

https://codereview.chromium.org/14576015/diff/3008/chrome/browser/lifetime/ap...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/14576015/diff/3008/chrome/browser/lifetime/ap...
chrome/browser/lifetime/application_lifetime.cc:112: // browsers are closed. The
viewer process, which holds a reference to
technically it's not the viewer process that holds a reference, it's the host
side of the browser-viewer connection.

[gab, 2013-05-13 17:46:16]

PTAL, thanks!

https://codereview.chromium.org/14576015/diff/3008/chrome/browser/browser_pro...
File chrome/browser/browser_process_impl_win.cc (right):

https://codereview.chromium.org/14576015/diff/3008/chrome/browser/browser_pro...
chrome/browser/browser_process_impl_win.cc:34: NOTREACHED();
On 2013/05/10 16:51:48, robertshield wrote:
> what if this gets called twice before OnMetroViewerProcessTerminated gets
> called? Would this not then be expected?

OnMetroViewerProcessTerminated() is called synchronously from OnChannelError()
so I'd guess that it's not possible for the IPC to be disconnected yet for this
to be called when metro_viewer_process_host_ is non-NULL.

Plus this method is really only meant to be called once from AttemptExit() which
is probably also not meant to be called multiple times in a short timespan.

https://codereview.chromium.org/14576015/diff/3008/chrome/browser/lifetime/ap...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/14576015/diff/3008/chrome/browser/lifetime/ap...
chrome/browser/lifetime/application_lifetime.cc:112: // browsers are closed. The
viewer process, which holds a reference to
On 2013/05/10 16:51:48, robertshield wrote:
> technically it's not the viewer process that holds a reference, it's the host
> side of the browser-viewer connection. 

Good point, clarified.

[gab, 2013-05-13 22:47:50]

Sir, PTAL, removed NOTREACHED() as discussed.

Cheers!
Gab

[robertshield, 2013-05-14 16:46:57]

lgtm

[gab, 2013-05-14 17:07:48]

Scott, please take a look.

Thanks!
Gab

[sky, 2013-05-14 17:17:51]

https://codereview.chromium.org/14576015/diff/19001/chrome/browser/browser_pr...
File chrome/browser/browser_process.h (right):

https://codereview.chromium.org/14576015/diff/19001/chrome/browser/browser_pr...
chrome/browser/browser_process.h:230: virtual void TerminateMetroViewerProcess()
= 0;
Move these two into a win-aura specific BrowserProcessPlatformPart. There isn't
one yet, so you'll have to create it. Follow the pattern of the chromeos
specific one.

https://codereview.chromium.org/14576015/diff/19001/chrome/browser/metro_view...
File chrome/browser/metro_viewer/metro_viewer_process_host_win.cc (right):

https://codereview.chromium.org/14576015/diff/19001/chrome/browser/metro_view...
chrome/browser/metro_viewer/metro_viewer_process_host_win.cc:60: else
nit: no else.

[gab, 2013-05-14 17:45:25]

See replies below.

Thanks,
Gab

https://codereview.chromium.org/14576015/diff/19001/chrome/browser/browser_pr...
File chrome/browser/browser_process.h (right):

https://codereview.chromium.org/14576015/diff/19001/chrome/browser/browser_pr...
chrome/browser/browser_process.h:230: virtual void TerminateMetroViewerProcess()
= 0;
On 2013/05/14 17:17:51, sky wrote:
> Move these two into a win-aura specific BrowserProcessPlatformPart. There
isn't
> one yet, so you'll have to create it. Follow the pattern of the chromeos
> specific one.

These methods need to have access to metro_viewer_process_host_ which is a
member of BrowserProcessImpl.

The only way I can think of to move these into a win-aura specific
BrowserProcessPlatformPart would be to pass a pointer the this pointer of
BrowserProcessImpl when constructing a BrowserProcessPlatformPart and making
BrowserProcessPlatformPart a friend of BrowserProcessImpl (or publicly expose a
non-const getter to the internal pointer =S), but that feels like a worst hack
than the current one...

WDYT?

https://codereview.chromium.org/14576015/diff/19001/chrome/browser/metro_view...
File chrome/browser/metro_viewer/metro_viewer_process_host_win.cc (right):

https://codereview.chromium.org/14576015/diff/19001/chrome/browser/metro_view...
chrome/browser/metro_viewer/metro_viewer_process_host_win.cc:60: else
On 2013/05/14 17:17:51, sky wrote:
> nit: no else.

Done.

[sky, 2013-05-14 20:47:45]

https://codereview.chromium.org/14576015/diff/19001/chrome/browser/browser_pr...
File chrome/browser/browser_process.h (right):

https://codereview.chromium.org/14576015/diff/19001/chrome/browser/browser_pr...
chrome/browser/browser_process.h:230: virtual void TerminateMetroViewerProcess()
= 0;
On 2013/05/14 17:45:25, gab wrote:
> On 2013/05/14 17:17:51, sky wrote:
> > Move these two into a win-aura specific BrowserProcessPlatformPart. There
> isn't
> > one yet, so you'll have to create it. Follow the pattern of the chromeos
> > specific one.
> 
> These methods need to have access to metro_viewer_process_host_ which is a
> member of BrowserProcessImpl.
> 
> The only way I can think of to move these into a win-aura specific
> BrowserProcessPlatformPart would be to pass a pointer the this pointer of
> BrowserProcessImpl when constructing a BrowserProcessPlatformPart and making
> BrowserProcessPlatformPart a friend of BrowserProcessImpl (or publicly expose
a
> non-const getter to the internal pointer =S), but that feels like a worst hack
> than the current one...
> 
> WDYT?

Can metro_viewer_process_host_ be moved too? Ideally this file would have no
ifdefs.

[gab, 2013-05-15 17:13:23]

https://codereview.chromium.org/14576015/diff/19001/chrome/browser/browser_pr...
File chrome/browser/browser_process.h (right):

https://codereview.chromium.org/14576015/diff/19001/chrome/browser/browser_pr...
chrome/browser/browser_process.h:230: virtual void TerminateMetroViewerProcess()
= 0;
On 2013/05/14 20:47:45, sky wrote:
> On 2013/05/14 17:45:25, gab wrote:
> > On 2013/05/14 17:17:51, sky wrote:
> > > Move these two into a win-aura specific BrowserProcessPlatformPart. There
> > isn't
> > > one yet, so you'll have to create it. Follow the pattern of the chromeos
> > > specific one.
> > 
> > These methods need to have access to metro_viewer_process_host_ which is a
> > member of BrowserProcessImpl.
> > 
> > The only way I can think of to move these into a win-aura specific
> > BrowserProcessPlatformPart would be to pass a pointer the this pointer of
> > BrowserProcessImpl when constructing a BrowserProcessPlatformPart and making
> > BrowserProcessPlatformPart a friend of BrowserProcessImpl (or publicly
expose
> a
> > non-const getter to the internal pointer =S), but that feels like a worst
hack
> > than the current one...
> > 
> > WDYT?
> 
> Can metro_viewer_process_host_ be moved too? Ideally this file would have no
> ifdefs.

Of course, not sure why I thought that wouldn't work... anyways did it in a
separate CL (https://codereview.chromium.org/14698027/) and rebased this one on
top of that one.

PTAL.

Thanks!
Gab

[sky, 2013-05-15 21:13:18]

https://codereview.chromium.org/14576015/diff/43017/chrome/browser/lifetime/a...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/14576015/diff/43017/chrome/browser/lifetime/a...
chrome/browser/lifetime/application_lifetime.cc:116:
g_browser_process->platform_part()->TerminateMetroViewerProcess();
How about naming this AttemptExit so that you can put it in the base class and
remove the ifdefs here?

[gab, 2013-05-17 23:19:18]

Done, PTAL.

Thanks!
Gab

https://codereview.chromium.org/14576015/diff/43017/chrome/browser/lifetime/a...
File chrome/browser/lifetime/application_lifetime.cc (right):

https://codereview.chromium.org/14576015/diff/43017/chrome/browser/lifetime/a...
chrome/browser/lifetime/application_lifetime.cc:116:
g_browser_process->platform_part()->TerminateMetroViewerProcess();
On 2013/05/15 21:13:18, sky wrote:
> How about naming this AttemptExit so that you can put it in the base class and
> remove the ifdefs here?

Done on top of https://codereview.chromium.org/14951007.

[sky, 2013-05-18 15:18:57]

Nice cleanup, LGTM

https://codereview.chromium.org/14576015/diff/87001/chrome/browser/browser_pr...
File chrome/browser/browser_process_platform_part_base.cc (right):

https://codereview.chromium.org/14576015/diff/87001/chrome/browser/browser_pr...
chrome/browser/browser_process_platform_part_base.cc:23:
chrome::CloseAllBrowsers();
I think its a little mysterious that you have this here and sometimes invoke it
(say aurawin) and sometimes don't (say android). But I can't think of a better
way:(

[commit-bot: I haz the power, 2013-05-20 15:32:31]

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

[commit-bot: I haz the power, 2013-05-20 15:43:33]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-20 18:53:40]

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

[commit-bot: I haz the power, 2013-05-20 19:08:29]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_dbg_simulator.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_dbg_si...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-21 02:56:47]

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

[commit-bot: I haz the power, 2013-05-21 03:10:08]

Sorry for I got bad news for ya.
Compile failed with a clobber build on ios_rel_device.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=ios_rel_de...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[gab, 2013-05-21 03:50:33]

Scott, see reply below.

Thanks,
Gab

https://codereview.chromium.org/14576015/diff/87001/chrome/browser/browser_pr...
File chrome/browser/browser_process_platform_part_base.cc (right):

https://codereview.chromium.org/14576015/diff/87001/chrome/browser/browser_pr...
chrome/browser/browser_process_platform_part_base.cc:23:
chrome::CloseAllBrowsers();
On 2013/05/18 15:18:58, sky wrote:
> I think its a little mysterious that you have this here and sometimes invoke
it
> (say aurawin) and sometimes don't (say android). But I can't think of a better
> way:(

While playing some more with the application lifetime code to try to fix
android/ios compile issues with this CL I found out that application_lifetime
already has platform specific files for HandleAppExitingForPlatform() [1].

It would probably make sense to put the platform-specific parts of AttemptExit()
in there instead of in BrowserProcessPlatformPart since only Winaura actually
needs to do work that requires state from the BrowserProcessPlatformPart
(metro_viewer_process_host_); this could be brought back as a WinAura specific
BrowserProcessPlatformPart::TerminateMetroViewerProcess() call as in patch set 5
and be called from application_lifetime_win.cc::AttemptExit() for USE_AURA/Win8+
only.

WDYT?
Either:
1) Keep the current paradigm and go through a generic AttemptExit() call in
g_browser_process for all platforms.
or 2) Make a generic AttemptExitForPlatform() call in an internals:: namespace
in the existing platform specific application_lifetime_* files and introduce a
WinAura only call BrowserProcessPlatformPart::TerminateMetroViewerProcess().


[1]
https://code.google.com/p/chromium/codesearch#search/&q=HandleAppExitingForPl...

[gab, 2013-05-21 15:39:30]

https://codereview.chromium.org/14576015/diff/87001/chrome/browser/browser_pr...
File chrome/browser/browser_process_platform_part_base.cc (right):

https://codereview.chromium.org/14576015/diff/87001/chrome/browser/browser_pr...
chrome/browser/browser_process_platform_part_base.cc:23:
chrome::CloseAllBrowsers();
On 2013/05/21 03:50:33, gab wrote:
> On 2013/05/18 15:18:58, sky wrote:
> > I think its a little mysterious that you have this here and sometimes invoke
> it
> > (say aurawin) and sometimes don't (say android). But I can't think of a
better
> > way:(
> 
> While playing some more with the application lifetime code to try to fix
> android/ios compile issues with this CL I found out that application_lifetime
> already has platform specific files for HandleAppExitingForPlatform() [1].
> 
> It would probably make sense to put the platform-specific parts of
AttemptExit()
> in there instead of in BrowserProcessPlatformPart since only Winaura actually
> needs to do work that requires state from the BrowserProcessPlatformPart
> (metro_viewer_process_host_); this could be brought back as a WinAura specific
> BrowserProcessPlatformPart::TerminateMetroViewerProcess() call as in patch set
5
> and be called from application_lifetime_win.cc::AttemptExit() for
USE_AURA/Win8+
> only.
> 
> WDYT?
> Either:
> 1) Keep the current paradigm and go through a generic AttemptExit() call in
> g_browser_process for all platforms.
> or 2) Make a generic AttemptExitForPlatform() call in an internals:: namespace
> in the existing platform specific application_lifetime_* files and introduce a
> WinAura only call BrowserProcessPlatformPart::TerminateMetroViewerProcess().
> 

Actually, I still feel the current approach (1) is better, the
application_lifetime_* approach also has the issue that it uses a _stub.cc as a
base impl, but any platform not using the stub has to re-implement the default
behavior for methods it doesn't want to override...

Let me know what you think.

> 
> [1]
>
https://code.google.com/p/chromium/codesearch#search/&q=HandleAppExitingForPl...

[sky, 2013-05-21 16:28:06]

I like 1 better too.

On Tue, May 21, 2013 at 8:39 AM,  <gab@chromium.org> wrote:
>
>
https://codereview.chromium.org/14576015/diff/87001/chrome/browser/browser_pr...
> File chrome/browser/browser_process_platform_part_base.cc (right):
>
>
https://codereview.chromium.org/14576015/diff/87001/chrome/browser/browser_pr...
> chrome/browser/browser_process_platform_part_base.cc:23:
> chrome::CloseAllBrowsers();
> On 2013/05/21 03:50:33, gab wrote:
>>
>> On 2013/05/18 15:18:58, sky wrote:
>> > I think its a little mysterious that you have this here and
>
> sometimes invoke
>>
>> it
>> > (say aurawin) and sometimes don't (say android). But I can't think
>
> of a better
>>
>> > way:(
>
>
>> While playing some more with the application lifetime code to try to
>
> fix
>>
>> android/ios compile issues with this CL I found out that
>
> application_lifetime
>>
>> already has platform specific files for HandleAppExitingForPlatform()
>
> [1].
>
>> It would probably make sense to put the platform-specific parts of
>
> AttemptExit()
>>
>> in there instead of in BrowserProcessPlatformPart since only Winaura
>
> actually
>>
>> needs to do work that requires state from the
>
> BrowserProcessPlatformPart
>>
>> (metro_viewer_process_host_); this could be brought back as a WinAura
>
> specific
>>
>> BrowserProcessPlatformPart::TerminateMetroViewerProcess() call as in
>
> patch set 5
>>
>> and be called from application_lifetime_win.cc::AttemptExit() for
>
> USE_AURA/Win8+
>>
>> only.
>
>
>> WDYT?
>> Either:
>> 1) Keep the current paradigm and go through a generic AttemptExit()
>
> call in
>>
>> g_browser_process for all platforms.
>> or 2) Make a generic AttemptExitForPlatform() call in an internals::
>
> namespace
>>
>> in the existing platform specific application_lifetime_* files and
>
> introduce a
>>
>> WinAura only call
>
> BrowserProcessPlatformPart::TerminateMetroViewerProcess().
>
>
> Actually, I still feel the current approach (1) is better, the
> application_lifetime_* approach also has the issue that it uses a
> _stub.cc as a base impl, but any platform not using the stub has to
> re-implement the default behavior for methods it doesn't want to
> override...
>
> Let me know what you think.
>
>
>
>> [1]
>
>
>
https://code.google.com/p/chromium/codesearch#search/&q=HandleAppExitingForPl...
>
> https://codereview.chromium.org/14576015/

[commit-bot: I haz the power, 2013-05-21 19:49:56]

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

[gab, 2013-05-21 19:57:50]

Ok... this is ugly... OS_IOS barely compiles anything, but
browser_process_platform_part_base.cc is part of what it compiles... so to make
it work (from patch set 13) I had to do 2 very ugly things:

1) Apparently OS_IOS implies OS_MACOSX (but filename_rules.gypi clearly splits
_mac and _ios files for each OS... so there is a discrepancy there)... anyways
in our case we don't want the _mac file on OS_IOS so we have to check
defined(OS_MACOSX) && !defined(OS_IOS) in browser_process_platform_part.h
(ewwww...!).

2) OS_IOS needs to compile browser_process_platform_part_base.cc because it is
required in the basic set of dependencies for chrome_test_suite.cc which ios
does compile... but chrome::CloseAllBrowsers() doesn't actually link on OS_IOS
(since application_lifetime.cc isn't compiled in)... so the easiest (but oh so
ugly) solution is to #if !defined(OS_IOS) that call in
browser_process_platform_part_base.cc.... it seems OS_IOS doesn't use
AttemptExit anyways, but I still hate having to put this there....


Opened to other suggestions, but so far it feels to me the overall build config
for ios is terribly broken and hurting non-ios parts of the codebase :(.

Gab

[commit-bot: I haz the power, 2013-05-21 20:26:59]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-21 20:35:13]

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

[commit-bot: I haz the power, 2013-05-21 20:47:06]

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

[commit-bot: I haz the power, 2013-05-21 21:23:40]

Sorry for I got bad news for ya.
Compile failed with a clobber build on android_clang_dbg.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_cl...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[commit-bot: I haz the power, 2013-05-21 22:43:49]

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

[gab, 2013-05-21 22:44:54]

Ugh... had to make this even uglier since chrome::CloseAllBrowsers() also
doesn't link on OS_ANDROID.

Let me know if you think of something better.


On 2013/05/21 19:57:50, gab wrote:
> Ok... this is ugly... OS_IOS barely compiles anything, but
> browser_process_platform_part_base.cc is part of what it compiles... so to
make
> it work (from patch set 13) I had to do 2 very ugly things:
> 
> 1) Apparently OS_IOS implies OS_MACOSX (but filename_rules.gypi clearly splits
> _mac and _ios files for each OS... so there is a discrepancy there)... anyways
> in our case we don't want the _mac file on OS_IOS so we have to check
> defined(OS_MACOSX) && !defined(OS_IOS) in browser_process_platform_part.h
> (ewwww...!).
> 
> 2) OS_IOS needs to compile browser_process_platform_part_base.cc because it is
> required in the basic set of dependencies for chrome_test_suite.cc which ios
> does compile... but chrome::CloseAllBrowsers() doesn't actually link on OS_IOS
> (since application_lifetime.cc isn't compiled in)... so the easiest (but oh so
> ugly) solution is to #if !defined(OS_IOS) that call in
> browser_process_platform_part_base.cc.... it seems OS_IOS doesn't use
> AttemptExit anyways, but I still hate having to put this there....
> 
> 
> Opened to other suggestions, but so far it feels to me the overall build
config
> for ios is terribly broken and hurting non-ios parts of the codebase :(.
> 
> Gab

[commit-bot: I haz the power, 2013-05-21 23:20:02]

Sorry for I got bad news for ya.
Compile failed with a clobber build on linux_chromeos_clang.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...
Your code is likely broken or HEAD is junk. Please ensure your
code is not broken then alert the build sheriffs.
Look at the try server FAQ for more details.

[sky, 2013-05-21 23:47:02]

SLGTM

[commit-bot: I haz the power, 2013-05-22 02:20:52]

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

[gab, 2013-05-22 02:41:27]

The CQ seems to be stuck, dcommitting since the bots that were failing on patch
set 13 (android and ios) are now passing and the bots that flaked on patch set
21 (chrome_os and win) were passing then (and nothing has changed for them since
patch set 13).

[commit-bot: I haz the power, 2013-05-22 03:40:48]

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

[commit-bot: I haz the power, 2013-05-22 10:10:49]

Change committed as 201476