mus/chrome: Always wait for the mojo shell.

mus/chrome: Always wait for the mojo shell in the browser process.

When chrome is launched in mus, it always needs to block on Initialize
to complete, regardless of whether run as 'chrome --mash' or as
'mojo_runner mojo:mash_session'.

BUG=594852
Committed: https://crrev.com/2d26a030a05d92fbb7489d62cfb7759b8f045335
Cr-Commit-Position: refs/heads/master@{#389816}

[sadrul, 2016-04-25 22:14:24]

sadrul@chromium.org changed reviewers:
+ rockot@chromium.org, sky@chromium.org

[sadrul, 2016-04-25 22:14:25]

When 'mojo_runner mojo:mash_session' is run, chrome never launches. It does
launch if --wait-for-mojo-shell flag is used. To fix this, we could probably add
the flag in //mash/session/session.cc before doing anything, but we would also
have to add the flag elsewhere (e.g. in
https://codereview.chromium.org/1921113002/). I don't think there is any
use-case where we would launch chrome in mus where we would not need the
--wait-for-mojo-shell flag. So this patch removes the flag. (The TODO to make
this non-blocking remains in effect, of course)

[Ken Rockot(use gerrit already), 2016-04-25 23:29:35]

https://codereview.chromium.org/1915053004/diff/1/content/common/mojo/mojo_sh...
File content/common/mojo/mojo_shell_connection_impl.cc (right):

https://codereview.chromium.org/1915053004/diff/1/content/common/mojo/mojo_sh...
content/common/mojo/mojo_shell_connection_impl.cc:43: return
mojo_shell_connection_factory || IsRunningInMojoShell();
Unfortunately IsRunningInMojoShell is a misnomer, as it looks for a flag which
is used for reasons other than running in Mojo shell. For example, content child
processes get this flag set by the browser process when launched. It should
really be called HasShellClientRequestOnCommandLine or something.

We do not want content child processes waiting for the Mojo shell. We need some
other way to detect if we're running in an external shell, and that's what
--wait-for-mojo-shell was for. Can we somehow force this flag to be present in
the cases we care about?

[sadrul, 2016-04-26 03:22:04]

https://codereview.chromium.org/1915053004/diff/1/content/common/mojo/mojo_sh...
File content/common/mojo/mojo_shell_connection_impl.cc (right):

https://codereview.chromium.org/1915053004/diff/1/content/common/mojo/mojo_sh...
content/common/mojo/mojo_shell_connection_impl.cc:43: return
mojo_shell_connection_factory || IsRunningInMojoShell();
On 2016/04/25 23:29:35, Ken Rockot wrote:
> Unfortunately IsRunningInMojoShell is a misnomer, as it looks for a flag which
> is used for reasons other than running in Mojo shell. For example, content
child
> processes get this flag set by the browser process when launched. It should
> really be called HasShellClientRequestOnCommandLine or something.
> 
> We do not want content child processes waiting for the Mojo shell. We need
some
> other way to detect if we're running in an external shell, and that's what
> --wait-for-mojo-shell was for. Can we somehow force this flag to be present in
> the cases we care about?

I was thinking a naive workaround would be to check for the process-type as well
... but I have done things a bit differently in the latest patchset: from
reading https://crrev.com/381182, it sounds like we need to wait for the
initialization only because of how we initialize views
(views::WindowManagerConnection, which initializes views::ScreenMus, which does
another blocking call to mojo:mus), and so I moved the 'wait for initialization'
in the same place where we create this views::WindowManagerConnection object, in
BrowserMainLoop (just before WindowManagerConnection is creted). This means none
of the child processes ever block, and the browser process only blocks when
running inside of mus. I think this takes care of the concerns? (if not, I will
add the cmd-flag explicitly in the test and other such cases where we need to,
but I thought I would give this one more try)

[Ken Rockot(use gerrit already), 2016-04-26 03:28:56]

content changes lgtm as non-owner, i'll defer to sky on test & mash changes.
thanks!

https://codereview.chromium.org/1915053004/diff/20001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1915053004/diff/20001/content/browser/browser...
content/browser/browser_main_loop.cc:1202: WaitForMojoShellInitialize();
Do you think there may ever be a future where we can avoid blocking startup on
this connection? IIRC this is only needed because mus (or the window server, or
mus client code or... something, I don't remember) itself needs to completely
block the main thread until it receives some message, and that message can't be
received unless the shell connection has been initialized (which of course can't
happen while the main thread is blocked).

If this might be possible to avoid, please preserve my TODO from the other file
here. If not, I would like to understand why.

[sadrul, 2016-04-26 03:43:34]

sadrul@chromium.org changed reviewers:
+ jam@chromium.org

[sadrul, 2016-04-26 03:43:35]

sky@ for chrome/ owner review.

jam@ for content/ owner review.

https://codereview.chromium.org/1915053004/diff/20001/content/browser/browser...
File content/browser/browser_main_loop.cc (right):

https://codereview.chromium.org/1915053004/diff/20001/content/browser/browser...
content/browser/browser_main_loop.cc:1202: WaitForMojoShellInitialize();
On 2016/04/26 03:28:55, Ken Rockot wrote:
> Do you think there may ever be a future where we can avoid blocking startup on
> this connection? IIRC this is only needed because mus (or the window server,
or
> mus client code or... something, I don't remember) itself needs to completely
> block the main thread until it receives some message, and that message can't
be
> received unless the shell connection has been initialized (which of course
can't
> happen while the main thread is blocked).
> 
> If this might be possible to avoid, please preserve my TODO from the other
file
> here. If not, I would like to understand why.

I have added back the TODO.

It might be possible to avoid the blocking behaviour of
WindowManagerConnection/ScreenMus. One way would be to init with a default set
if displays in ScreenMus, and update local state when the mus-server sends back
information about the real set of displays to it asynchronously (instead of
blocking on the response). I plan on trying this out and see how well, if at
all, it works.

[sadrul, 2016-04-26 03:44:48]

Description was changed from

==========
mus/chrome: Always wait for the mojo shell.

When chrome is launched in mus, it always needs to block on Initialize
to complete, regardless of whether run as 'chrome --mash' or as
'mojo_runner mojo:mash_session'.

BUG=594852
==========

to

==========
mus/chrome: Always wait for the mojo shell in the browser process.

When chrome is launched in mus, it always needs to block on Initialize
to complete, regardless of whether run as 'chrome --mash' or as
'mojo_runner mojo:mash_session'.

BUG=594852
==========

[jam, 2016-04-26 16:25:27]

lgtm

[sky, 2016-04-26 16:44:10]

LGTM

[sadrul, 2016-04-26 17:32:58]

The CQ bit was checked by sadrul@chromium.org

[sadrul, 2016-04-26 17:32:58]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org
Link to the patchset: https://codereview.chromium.org/1915053004/#ps40001
(title: ".")

[commit-bot: I haz the power, 2016-04-26 17:33:20]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1915053004/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1915053004/40001

[commit-bot: I haz the power, 2016-04-26 17:37:35]

Description was changed from

==========
mus/chrome: Always wait for the mojo shell in the browser process.

When chrome is launched in mus, it always needs to block on Initialize
to complete, regardless of whether run as 'chrome --mash' or as
'mojo_runner mojo:mash_session'.

BUG=594852
==========

to

==========
mus/chrome: Always wait for the mojo shell in the browser process.

When chrome is launched in mus, it always needs to block on Initialize
to complete, regardless of whether run as 'chrome --mash' or as
'mojo_runner mojo:mash_session'.

BUG=594852
==========

[commit-bot: I haz the power, 2016-04-26 17:37:36]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-04-26 17:39:04]

Description was changed from

==========
mus/chrome: Always wait for the mojo shell in the browser process.

When chrome is launched in mus, it always needs to block on Initialize
to complete, regardless of whether run as 'chrome --mash' or as
'mojo_runner mojo:mash_session'.

BUG=594852
==========

to

==========
mus/chrome: Always wait for the mojo shell in the browser process.

When chrome is launched in mus, it always needs to block on Initialize
to complete, regardless of whether run as 'chrome --mash' or as
'mojo_runner mojo:mash_session'.

BUG=594852

Committed: https://crrev.com/2d26a030a05d92fbb7489d62cfb7759b8f045335
Cr-Commit-Position: refs/heads/master@{#389816}
==========

[commit-bot: I haz the power, 2016-04-26 17:39:05]

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