Eliminate Quit() from Shell, and roll Shell & Connector together.

Eliminate Quit() from Shell, and roll Shell & Connector together.

R=sky@chromium.org
BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/189e32b3508f0362cc7571af1554c223f353c2cd
Cr-Commit-Position: refs/heads/master@{#377702}

[Ben Goodger (Google), 2016-02-24 23:55:12]

Description was changed from

==========
Eliminate Quit() from Shell, and roll Shell & Connector together.

R=sky@chromium.org
BUG=
==========

to

==========
Eliminate Quit() from Shell, and roll Shell & Connector together.

R=sky@chromium.org
BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

[sky, 2016-02-25 17:13:10]

LGTM

https://codereview.chromium.org/1736663003/diff/100001/content/renderer/rende...
File content/renderer/render_frame_impl.h (right):

https://codereview.chromium.org/1736663003/diff/100001/content/renderer/rende...
content/renderer/render_frame_impl.h:1166: mojo::shell::mojom::ConnectorPtr
mojo_shell_;
mojo_shell_-> connector_?

https://codereview.chromium.org/1736663003/diff/100001/mojo/shell/public/cpp/...
File mojo/shell/public/cpp/lib/shell_connection.cc (right):

https://codereview.chromium.org/1736663003/diff/100001/mojo/shell/public/cpp/...
mojo/shell/public/cpp/lib/shell_connection.cc:121: if
(base::MessageLoop::current() &&
ick!

[Ben Goodger (Google), 2016-02-25 17:30:15]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2016-02-25 17:30:16]

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

[commit-bot: I haz the power, 2016-02-25 17:33:26]

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

[commit-bot: I haz the power, 2016-02-25 17:41:55]

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

[commit-bot: I haz the power, 2016-02-25 17:41:56]

Try jobs failed on following builders:
  android_arm64_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...)
  android_chromium_gn_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_chromium_gn_compile_rel on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_chro...)
  android_clang_dbg_recipe on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  android_compile_dbg on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
  cast_shell_android on tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...)
  cast_shell_linux on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  chromeos_daisy_chromium_compile_only_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_daisy_...)
  chromium_presubmit on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_compile_dbg_ng on tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  linux_site_isolation on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_site_isol...)
  ios_dbg_simulator_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_dbg_simulator...)
  ios_rel_device_ninja on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios_rel_device_ni...)
  mac_chromium_compile_dbg_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
  mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
  mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
  win8_chromium_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_ng/...)
  win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Ben Goodger (Google), 2016-02-25 17:48:11]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2016-02-25 17:48:11]

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

[commit-bot: I haz the power, 2016-02-25 17:48:46]

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

[yzshen1, 2016-02-25 18:54:18]

Will this change result in losing connect requests when an application instance
is shutting down?

I feel that if we want to remove the quit handshake, one possible way is to (1)
have AcceptConnection() have a callback to report whether a connection is
accepted and (2) if it is not accepted due to application instance shutdown, the
shell should be able to do a second attempt for the same connect request:

AcceptConnection(string requestor_url,
                 uint32 requestor_id,
                 uint32 requestor_user_id,
                 array<string> allowed_interfaces,
                 string resolved_url) => (InterfaceProvider? local_interfaces,
                                          InterfaceProvider&?
remote_interfaces);

Correspondingly, Connect() needs to become:
Connect(string url,
        uint32 user_id) => (uint32 application_id,
                            InterfaceProvider&? local_interfaces,
InterfaceProvider? remote_interfaces);

That will be pretty inconvenient to use given that we usually want to use the
remote InterfaceProvider immediately after calling Connect.

One possible solution is to support "connecting" two message pipes into one,
that way we can keep the existing Connect() signature.
That will need new mojo system API support. Maybe doable with the current EDK
implementation.

[Ben Goodger (Google), 2016-02-25 19:17:51]

On 2016/02/25 18:54:18, yzshen1 wrote:
> Will this change result in losing connect requests when an application
instance
> is shutting down?

This is an interesting thing to ponder. I expect there'll be some issues here.
One problem with the solution before this change is that the scenario you
describe could occur if the instance doesn't cooperate with the shutdown
handshake, e.g. it crashes. This is a danger of having an API that relies on
cooperation from the client.

From my perspective it's desirable to have a solution that assumes the client
may disappear for any reason and the application manager has to recover somehow.
We can treat application exits and crashes equivalently then.

I have a stream of changes that this is part of, so I'll add this to my TODO
list. Right now this doesn't really impact chrome since we statically load
everything and so nothing ever "quits".

[commit-bot: I haz the power, 2016-02-25 20:12:24]

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

[commit-bot: I haz the power, 2016-02-25 20:12:25]

Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[Ben Goodger (Google), 2016-02-25 20:43:51]

The CQ bit was checked by ben@chromium.org

[Ben Goodger (Google), 2016-02-25 20:43:51]

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

[commit-bot: I haz the power, 2016-02-25 20:45:25]

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

[commit-bot: I haz the power, 2016-02-25 23:22:04]

Committed patchset #9 (id:160001)

[commit-bot: I haz the power, 2016-02-25 23:23:47]

Description was changed from

==========
Eliminate Quit() from Shell, and roll Shell & Connector together.

R=sky@chromium.org
BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation
==========

to

==========
Eliminate Quit() from Shell, and roll Shell & Connector together.

R=sky@chromium.org
BUG=
CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation

Committed: https://crrev.com/189e32b3508f0362cc7571af1554c223f353c2cd
Cr-Commit-Position: refs/heads/master@{#377702}
==========

[commit-bot: I haz the power, 2016-02-25 23:23:49]

Patchset 9 (id:??) landed as
https://crrev.com/189e32b3508f0362cc7571af1554c223f353c2cd
Cr-Commit-Position: refs/heads/master@{#377702}

[Ben Goodger (Google), 2016-03-05 04:32:02]

On 2016/02/25 18:54:18, yzshen1 wrote:
> Will this change result in losing connect requests when an application
instance
> is shutting down?

Can we think about this from the perspective of a client that wishes to connect
to some service that is being shut down...

Won't the client ultimately receive broken pipes as a result of the connect
request? i.e. the pair of IP pipes will break. In the shell client lib, this
will result in the callback specified in
Connection::SetRemoteInterfaceProviderConnectionErrorHandler() being called. Can
they not then recover & re-issue the connect?

This is what I'd expect to happen in the case of a crash too.

[yzshen1, 2016-03-05 08:10:48]

On 2016/03/05 04:32:02, Ben Goodger (Google) wrote:
> On 2016/02/25 18:54:18, yzshen1 wrote:
> > Will this change result in losing connect requests when an application
> instance
> > is shutting down?
> 
> Can we think about this from the perspective of a client that wishes to
connect
> to some service that is being shut down...
> 
> Won't the client ultimately receive broken pipes as a result of the connect
> request? i.e. the pair of IP pipes will break. In the shell client lib, this
> will result in the callback specified in
> Connection::SetRemoteInterfaceProviderConnectionErrorHandler() being called.
Can
> they not then recover & re-issue the connect?
> 
> This is what I'd expect to happen in the case of a crash too.

If the client

[yzshen1, 2016-03-05 08:26:42]

On 2016/03/05 08:10:48, yzshen1 wrote:
> On 2016/03/05 04:32:02, Ben Goodger (Google) wrote:
> > On 2016/02/25 18:54:18, yzshen1 wrote:
> > > Will this change result in losing connect requests when an application
> > instance
> > > is shutting down?
> > 
> > Can we think about this from the perspective of a client that wishes to
> connect
> > to some service that is being shut down...
> > 
> > Won't the client ultimately receive broken pipes as a result of the connect
> > request? i.e. the pair of IP pipes will break. In the shell client lib, this
> > will result in the callback specified in
> > Connection::SetRemoteInterfaceProviderConnectionErrorHandler() being called.
> Can
> > they not then recover & re-issue the connect?
> > 
> > This is what I'd expect to happen in the case of a crash too.
> 
> If the client
(Sorry, mistakenly hit send.)

I agree that if the client observes IP disconnection, it can recover/re-issue
the connect.

I raised the question because I wondered whether normal shutdown should be
treated in the same way as crash. For example, normal shutdown may happen more
often than crashes and people may have less tolerance of data loss.

(In our pervious conversation, you pointed out to me that currently we don't
shutdown services. So I am not very concerned about this. Just trying to explain
why I raised this question at the beginning.)