> The main motivation here is to support Chrome OS where we want system services to be controlled by the Service Manager.

In Chrome OS, each system service is started and managed by upstart.
We already have a lot of scripts written in upstart's conf file format, so I think it's more feasible to let the service manager interact with upstart, instead of letting it launch services itself.

> - public mojom for the service (presumably kept in sync with service repo)

Chrome OS has a dedicated git repository to share constants, switches, and D-Bus interfaces with Chrome.
https://chromium.googlesource.com/chromiumos/platform/system_api/
It's already in Chromium checkout under src/third_party/cros_system_api
https://chromium.googlesource.com/chromium/src/+/0d0424e6f93cf283c1fd37e99669b7e9fd878fbd/DEPS#327

Thanks, yeah that makes sense, and simplifies things. In general SM will need a cross-platform way to start/kill/restart service processes for external binaries.  

If on Chrome OS that functionality is implemented as d-bus control messages to upstart, that seems fine by me. One concern I would have would be whether or not we can give extra command line flags to a service we want to be started/restarted.

It seems likely that the way system services will talk to SM is by connecting to a named socket SM exposes. Ideally we could pass a unique token to a service when starting it, so it has some way to authenticate its identity when establishing communication with the service manager.

Alternatively maybe there's some way we can leverage permission_broker so that SM can create a named socket, but only trusted system services can open it to connect as clients.

Does this mean system services's processes have to be initialized and owned by SM? In this case for boards that chrome are not present (jetstream, lakitu, etc.), we may end up with different system services behaviors...

Also according to https://www.chromium.org/chromium-os/chromiumos-design-docs/boot-design, currently some system services (powerd, cyptohomed, networkd, etc.) are launched in parallel with chrome. Potentially there could be some boot time performance impact.

No, it's a goal to make it so that system services can be controlled e.g. by upstart directly.

We would add some service manifest fields to inform the SM about how to locate the service (i.e. a D-Bus service/object path or whatever), and SM would use that to send over a file descriptor for bootstrapping Mojo IPC.

Is there a design doc for this somewhere?

Security-architecture-wise, the entire Chrome process tree is on of the least trusted parts of Chrome OS as a whole. I might misunderstand things here, but let me clarify that a service manager process that runs within the Chrome process tree will definitely *not* be allowed to control lifetime of arbitrary system services in general.

There is no work being done here at the moment and no design doc. The idea that SM would ever control system services is a long-term goal, and I think we should do whatever is necessary to make it trustworthy enough for that to happen.

In any case, the more immediate focus of this bug would be on having a mechanism for Service Manager to simply *locate* existing system services (controlled by more trusted bits) so that it can include them in the graph of mojo processes and route interface requests to them like it would to any service it actually does control.

IIUC the service manager will run as a Chrome OS system service directly started by upstart which doesn't belong to Chrome's process tree (issue 809322).

I have an naive question: will the crash of service manager process also cause other chrome processes to crash (e.g. browser process)? 

Also generally does each service (managed by service manager) run in its own process or it runs in the same process as service manager?

Let's back up a second, because this is all hypothetical: we still don't ever run a standalone service manager process.

In general if we did and the service manager crashed, anything which depends on services (so yes all of chrome and its subprocesses) would effectively need to be restarted. There are conceivable ways that we could relax this constraint in the future, but it's a relatively hard problem and I doubt it's much of an issue in practice (the service manager is supposed to be extremely simple, so if it crashes something is probably very wrong).

With the exception of tight resource constraints like Android devices, I doubt we would ever consider running arbitrary services within the service manager's own process. Especially on Chrome OS that would seem like an obviously bad idea.

As for service/process layout in general, it's entirely a matter of configuration. Today we have:
- many services which each run within their own isolated processes
- many services which run within the shared Chrome browser process
- some services (like ash and ui service) which share a single isolated process

Ideally every service instance would be isolated from each other, but it's not a hard technical requirement.

Based on more recent discussions, support for external binaries is planned to come in two different flavors and be Chrome OS-only for now:

A) A manifest key which tells the Service Manager how it can connect to a system service via D-Bus in order to bootstrap a Service pipe proactively when the service is needed.

B) Support for a manifest key which tells the Service Manager which executable path on disk is expected to advertise itself as the service. The Service Manager will listen on a socket server and accept connections, using the manifest information and system APIs to validate that the connecting peer is who it claims to be.

Tagging with a label to get this in more immediate backlog.

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/14c0b6045dd3036d297e23b6a849f622c15a4339

commit 14c0b6045dd3036d297e23b6a849f622c15a4339
Author: Ken Rockot <rockot@google.com>
Date: Sat Nov 03 00:29:42 2018

[mojo] Add IPC support to core shared library

Updates the mojo_core shared library so that it is actually suitable
for production IPC use. Namely:

- MojoInitialize now spins up a background thread for I/O and gives
  it to the core impl.
- A flag is added to Initialize(), allowing shared library consumers
  to initialize themselves as a broker process.
- The thunks helper correctly loads the mojo_core library with
  RTLD_DEEPBIND set, avoiding heap crossover within the core impl
  due to e.g. malloc and free lazy-binding to different heaps
- Fixes the thunks header to compile as C once again, as it was
  missing some struct keywords.
- Adds MojoShutdown to the core ABI as a necessary call for shared
  library consumers who want clean shutdown.

This also adds a new multiprocess test consuming the shared library
to tie together and validate all of the above changes.

Bug: 809320
Change-Id: Ic1c56d99c86fd4a2dc7f812ee152994ced35ece6
Reviewed-on: https://chromium-review.googlesource.com/c/1306347
Commit-Queue: Ken Rockot <rockot@google.com>
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605109}
[modify] https://crrev.com/14c0b6045dd3036d297e23b6a849f622c15a4339/mojo/core/BUILD.gn
[modify] https://crrev.com/14c0b6045dd3036d297e23b6a849f622c15a4339/mojo/core/entrypoints.cc
[modify] https://crrev.com/14c0b6045dd3036d297e23b6a849f622c15a4339/mojo/core/mojo_core.cc
[modify] https://crrev.com/14c0b6045dd3036d297e23b6a849f622c15a4339/mojo/core/mojo_core_unittest.cc
[modify] https://crrev.com/14c0b6045dd3036d297e23b6a849f622c15a4339/mojo/core/run_all_core_unittests.cc
[modify] https://crrev.com/14c0b6045dd3036d297e23b6a849f622c15a4339/mojo/public/c/system/functions.h
[modify] https://crrev.com/14c0b6045dd3036d297e23b6a849f622c15a4339/mojo/public/c/system/tests/core_unittest_pure_c.c
[modify] https://crrev.com/14c0b6045dd3036d297e23b6a849f622c15a4339/mojo/public/c/system/thunks.cc
[modify] https://crrev.com/14c0b6045dd3036d297e23b6a849f622c15a4339/mojo/public/c/system/thunks.h
[modify] https://crrev.com/14c0b6045dd3036d297e23b6a849f622c15a4339/mojo/public/c/system/types.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/6b5b3a46d1040119cf78b5ccadaea847a5279d76

commit 6b5b3a46d1040119cf78b5ccadaea847a5279d76
Author: Kunihiko Sakamoto <ksakamoto@chromium.org>
Date: Mon Nov 05 03:52:03 2018

Revert "[mojo] Add IPC support to core shared library"

This reverts commit 14c0b6045dd3036d297e23b6a849f622c15a4339.

Reason for revert: Following tests are timing out
- MojoCoreTest.SanityCheck
- MojoCoreTest.BasicMultiprocess

on MSAN bots:
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20MSan%20Tests/12687
https://ci.chromium.org/p/chromium/builders/luci.chromium.ci/Linux%20ChromiumOS%20MSan%20Tests/9416


Original change's description:
> [mojo] Add IPC support to core shared library
> 
> Updates the mojo_core shared library so that it is actually suitable
> for production IPC use. Namely:
> 
> - MojoInitialize now spins up a background thread for I/O and gives
>   it to the core impl.
> - A flag is added to Initialize(), allowing shared library consumers
>   to initialize themselves as a broker process.
> - The thunks helper correctly loads the mojo_core library with
>   RTLD_DEEPBIND set, avoiding heap crossover within the core impl
>   due to e.g. malloc and free lazy-binding to different heaps
> - Fixes the thunks header to compile as C once again, as it was
>   missing some struct keywords.
> - Adds MojoShutdown to the core ABI as a necessary call for shared
>   library consumers who want clean shutdown.
> 
> This also adds a new multiprocess test consuming the shared library
> to tie together and validate all of the above changes.
> 
> Bug: 809320
> Change-Id: Ic1c56d99c86fd4a2dc7f812ee152994ced35ece6
> Reviewed-on: https://chromium-review.googlesource.com/c/1306347
> Commit-Queue: Ken Rockot <rockot@google.com>
> Reviewed-by: Reilly Grant <reillyg@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#605109}

TBR=rockot@google.com,reillyg@chromium.org

# Not skipping CQ checks because original CL landed > 1 day ago.

Bug: 809320
Change-Id: I19816cf592bacd58dbb452cd32bea4df7a8077be
Reviewed-on: https://chromium-review.googlesource.com/c/1317297
Reviewed-by: Kunihiko Sakamoto <ksakamoto@chromium.org>
Commit-Queue: Kunihiko Sakamoto <ksakamoto@chromium.org>
Cr-Commit-Position: refs/heads/master@{#605240}
[modify] https://crrev.com/6b5b3a46d1040119cf78b5ccadaea847a5279d76/mojo/core/BUILD.gn
[modify] https://crrev.com/6b5b3a46d1040119cf78b5ccadaea847a5279d76/mojo/core/entrypoints.cc
[modify] https://crrev.com/6b5b3a46d1040119cf78b5ccadaea847a5279d76/mojo/core/mojo_core.cc
[modify] https://crrev.com/6b5b3a46d1040119cf78b5ccadaea847a5279d76/mojo/core/mojo_core_unittest.cc
[modify] https://crrev.com/6b5b3a46d1040119cf78b5ccadaea847a5279d76/mojo/core/run_all_core_unittests.cc
[modify] https://crrev.com/6b5b3a46d1040119cf78b5ccadaea847a5279d76/mojo/public/c/system/functions.h
[modify] https://crrev.com/6b5b3a46d1040119cf78b5ccadaea847a5279d76/mojo/public/c/system/tests/core_unittest_pure_c.c
[modify] https://crrev.com/6b5b3a46d1040119cf78b5ccadaea847a5279d76/mojo/public/c/system/thunks.cc
[modify] https://crrev.com/6b5b3a46d1040119cf78b5ccadaea847a5279d76/mojo/public/c/system/thunks.h
[modify] https://crrev.com/6b5b3a46d1040119cf78b5ccadaea847a5279d76/mojo/public/c/system/types.h

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9669c640138e1e3ba74ac85e1b0056a4381b1900

commit 9669c640138e1e3ba74ac85e1b0056a4381b1900
Author: Ken Rockot <rockot@google.com>
Date: Tue Nov 06 02:43:04 2018

Reland "[mojo] Add IPC support to core shared library"

This is a reland of 14c0b6045dd3036d297e23b6a849f622c15a4339

Original change's description:
> [mojo] Add IPC support to core shared library
>
> Updates the mojo_core shared library so that it is actually suitable
> for production IPC use. Namely:
>
> - MojoInitialize now spins up a background thread for I/O and gives
>   it to the core impl.
> - A flag is added to Initialize(), allowing shared library consumers
>   to initialize themselves as a broker process.
> - The thunks helper correctly loads the mojo_core library with
>   RTLD_DEEPBIND set, avoiding heap crossover within the core impl
>   due to e.g. malloc and free lazy-binding to different heaps
> - Fixes the thunks header to compile as C once again, as it was
>   missing some struct keywords.
> - Adds MojoShutdown to the core ABI as a necessary call for shared
>   library consumers who want clean shutdown.
>
> This also adds a new multiprocess test consuming the shared library
> to tie together and validate all of the above changes.
>
> Bug: 809320
> Change-Id: Ic1c56d99c86fd4a2dc7f812ee152994ced35ece6
> Reviewed-on: https://chromium-review.googlesource.com/c/1306347
> Commit-Queue: Ken Rockot <rockot@google.com>
> Reviewed-by: Reilly Grant <reillyg@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#605109}

TBR=reillyg@chromium.org

Bug: 809320
Change-Id: I8a8b2c1acfb16bf42823f266dffbae8e897075f3
Reviewed-on: https://chromium-review.googlesource.com/c/1318061
Reviewed-by: Reilly Grant <reillyg@chromium.org>
Reviewed-by: Ken Rockot <rockot@google.com>
Commit-Queue: Ken Rockot <rockot@google.com>
Cr-Commit-Position: refs/heads/master@{#605576}
[modify] https://crrev.com/9669c640138e1e3ba74ac85e1b0056a4381b1900/mojo/core/BUILD.gn
[modify] https://crrev.com/9669c640138e1e3ba74ac85e1b0056a4381b1900/mojo/core/entrypoints.cc
[modify] https://crrev.com/9669c640138e1e3ba74ac85e1b0056a4381b1900/mojo/core/mojo_core.cc
[modify] https://crrev.com/9669c640138e1e3ba74ac85e1b0056a4381b1900/mojo/core/mojo_core_unittest.cc
[modify] https://crrev.com/9669c640138e1e3ba74ac85e1b0056a4381b1900/mojo/core/run_all_core_unittests.cc
[modify] https://crrev.com/9669c640138e1e3ba74ac85e1b0056a4381b1900/mojo/public/c/system/functions.h
[modify] https://crrev.com/9669c640138e1e3ba74ac85e1b0056a4381b1900/mojo/public/c/system/tests/core_unittest_pure_c.c
[modify] https://crrev.com/9669c640138e1e3ba74ac85e1b0056a4381b1900/mojo/public/c/system/thunks.cc
[modify] https://crrev.com/9669c640138e1e3ba74ac85e1b0056a4381b1900/mojo/public/c/system/thunks.h
[modify] https://crrev.com/9669c640138e1e3ba74ac85e1b0056a4381b1900/mojo/public/c/system/types.h