Accept inbound connections on unix domain socket

Accept inbound connections on unix domain socket

In order for CrOS Core services to speak Mojo, the mojo_shell will need to be able to accept incoming connections from externally-managed processes.

POR is to have the shell listen on a unix domain socket and convert incoming connections to a proper Mojo MessagePipe and then wire it to an InterfaceImpl<ExternalApplicationRegistrar>, which will make the appropriate calls on ApplicationManager.

BUG=418289
TEST=mojo_external_application_tests
STATUS=Fixed

Committed: https://crrev.com/d500c98bc5ab976e9f590c76aeb9c7f7f347ec58
Cr-Commit-Position: refs/heads/master@{#297184}

[Chris Masone, 2014-08-29 00:28:12]

cmasone@chromium.org changed reviewers:
+ aa@chromium.org

[Chris Masone, 2014-08-29 00:28:12]

Look down near the bottom of external_application_listener_unittest.cc

[Chris Masone, 2014-09-20 01:17:31]

cmasone@chromium.org changed reviewers:
+ davemoore@chromium.org
- aa@chromium.org

[Chris Masone, 2014-09-20 01:17:31]

I've created this ExternalApplicationListener class that will open a unix domain
socket and listen on it for incoming connections. It'll use those connections to
allow externally-running applications to register and receive a Shell handle.

Opening this socket, as well as listening, accepting and closing, must all
happen on an IO thread.

Currently, this object is owned by Context, which is created on the main thread.
The problem is that I need to call StartListening on an IO thread -- which I can
do, but then the ExternalApplicationListener gets destroyed (and the socket gets
closed) on the wrong thread.

[Chris Masone, 2014-09-23 14:21:47]

Ok! Now, ExternalApplicationListener creates, uses, and destroys an
IncomingConnectionListener all on an IO thread. When an incoming connection
happens, the FD of that socket is extracted and passed back to the shell-runner
thread to be promoted to a message pipe and have a
InterfaceImpl<ExternalApplicationRegistrar> bound to it.

[Chris Masone, 2014-09-23 14:22:18]

On 2014/09/23 14:21:47, Chris Masone wrote:
> Ok! Now, ExternalApplicationListener creates, uses, and destroys an
> IncomingConnectionListener all on an IO thread. When an incoming connection
> happens, the FD of that socket is extracted and passed back to the
shell-runner
> thread to be promoted to a message pipe and have a
> InterfaceImpl<ExternalApplicationRegistrar> bound to it.

Oh, and the GYP build isn't up to date in this patch, but GN is so use that.

[Chris Masone, 2014-09-24 22:51:02]

On 2014/09/23 14:22:18, Chris Masone wrote:
> On 2014/09/23 14:21:47, Chris Masone wrote:
> > Ok! Now, ExternalApplicationListener creates, uses, and destroys an
> > IncomingConnectionListener all on an IO thread. When an incoming connection
> > happens, the FD of that socket is extracted and passed back to the
> shell-runner
> > thread to be promoted to a message pipe and have a
> > InterfaceImpl<ExternalApplicationRegistrar> bound to it.
> 
> Oh, and the GYP build isn't up to date in this patch, but GN is so use that.

Ok, Dave. I've added many comments here. Some of the unittest code is still
rough, but I'd appreciate a look at this to see if there's anything going here
that looks horrible to you.

[Chris Masone, 2014-09-25 14:31:43]

cmasone@chromium.org changed reviewers:
+ aa@chromium.org

[Chris Masone, 2014-09-25 14:31:44]

Aaron, this is the CL that I started a while ago to try to enable
externally-running applications to connect to the shell, where I was initially
having the issues around passing pipes over other pipes. You saw a verrrrry
early version of this.

Sending it to you for your edification, and also so you can have a glance at the
method I added to ApplicationManager.

[Chris Masone, 2014-09-26 01:07:42]

Dave? How does this all strike you?

[DaveMoore, 2014-09-26 22:48:57]

We should only create the listener when told to with a command line flag, but
other than that, LGTM

https://codereview.chromium.org/522443003/diff/120001/mojo/shell/incoming_con...
File mojo/shell/incoming_connection_listener.cc (right):

https://codereview.chromium.org/522443003/diff/120001/mojo/shell/incoming_con...
mojo/shell/incoming_connection_listener.cc:73: if (rv == net::OK) {
Nit: no braces

[Chris Masone, 2014-09-27 00:46:30]

Added the flag, as well: --enable-external-applications

running trybots now

https://codereview.chromium.org/522443003/diff/120001/mojo/shell/incoming_con...
File mojo/shell/incoming_connection_listener.cc (right):

https://codereview.chromium.org/522443003/diff/120001/mojo/shell/incoming_con...
mojo/shell/incoming_connection_listener.cc:73: if (rv == net::OK) {
On 2014/09/26 22:48:57, DaveMoore wrote:
> Nit: no braces

Done.

[Chris Masone, 2014-09-27 17:18:41]

The CQ bit was checked by cmasone@chromium.org

[Chris Masone, 2014-09-27 17:18:43]

The CQ bit was unchecked by cmasone@chromium.org

[Chris Masone, 2014-09-29 02:14:18]

Hey, Dave. In order to gracefully deal with the platform issues, I did the
following:

external_application_listener.{h,cc} =>
external_application_listener_posix.{h,cc}
incoming_connection_listener.{h,cc} => incoming_connection_listener_posix.{h,cc}
Created a new abstract base class in external_application_listener.h
Created ExternalApplicationListenerStub in
external_application_listener_win.{h,cc}

The _posix files are pretty much just moves of the originals, but given that I
had to introduce a new interface I figured you should give it another once-over.

I initially tried the trick where you have one platform-independent header and
then per-platform implementations of the class, but ExternalApplicationListener
had a pointer to an IncomingConnectionListener, so I had to then do the same
with THAT class. Which seemed stupid, so I just made a proper interface and did
per-platform classes for Posix and Win.

[DaveMoore, 2014-09-29 15:51:18]

lgtm

[Chris Masone, 2014-09-29 15:59:38]

The CQ bit was checked by cmasone@chromium.org

[commit-bot: I haz the power, 2014-09-29 16:00:08]

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

[commit-bot: I haz the power, 2014-09-29 16:04:58]

Committed patchset #18 (id:340001) as 86cb272b23ca0099e86ff3b10904a9157f4acbb1

[commit-bot: I haz the power, 2014-09-29 16:05:42]

Patchset 18 (id:??) landed as
https://crrev.com/d500c98bc5ab976e9f590c76aeb9c7f7f347ec58
Cr-Commit-Position: refs/heads/master@{#297184}