Open Bug 1480205 Opened 6 years ago Updated 2 years ago

Improve Mach IPC integration

Categories

(Core :: IPC, enhancement, P3)

Unspecified
macOS
enhancement

Tracking

()

People

(Reporter: jld, Assigned: jld)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

It would be good if IPC channels on OS X could transfer Mach ports as part of a message, the same way (as far as the interface is concerned) they can transfer Unix file descriptors or Windows handles. Currently there's some code in SharedMemoryBasic_mach.mm that does this separately from the rest of IPC, but it has a few issues: it creates a pair of threads for each content process and each pair of child processes that communicate via Mach messages, and it does blocking Mach IPC every time it sends a port (to get the numeric value of the port in the remote process and then send it back as data via Unix IPC). There's also some demand for Mach IPC from graphics (bug 1265824 comment #145); currently this is wedged awkwardly into SharedMemoryBasic and can block its thread in WaitForTextureIdsToUnlock. Even if the core of that were extracted, it would still wind up duplicating IPDL's message dispatch, and still have the possibility of lock contention looking up Mach info by pid, etc. (Disclaimer: I don't know Mach IPC very well, so hopefully I'm not mangling the terminology too much or assuming it can do something it can't.) My general idea is giving each IPC channel a pair of Mach ports in parallel with the socketpair, and having IPC::Message also hold a list of ports as well as bytes/fds. Endpoint<> could include both a send right and a receive right, but the IPC infrastructure doesn't make it easy to express unique capabilities (see also bug 1441651), so it might be easier to just pass a send right and then set up the other direction as part of the hello message. Bootstrapping the first actor could work roughly the way things work now. The I/O thread would block if the Mach port's buffer fills up before the socket buffer; I don't have a clear picture of how to handle that, but it may not be a problem in practice. Chromium has support for this in Mojo, so it may help to inspect how they handle it.
On second thought (and some more reading[1][2]), there might be an easier and more Windows-like approach here. The way the old Chromium IPC expects to work is that you use DuplicateHandle to give the process a copy of the handle, and then include that handle as bytes in the message data, and the IPC channels don't deal with that except for maybe keeping the remote process id around. mach_port_{allocate,extract_right,insert_right} look like they can do similar things to DuplicateHandle as far as transferring rights between processes without the other process needing to do anything. But the task port that's required looks dangerous for a sandboxed child process to have (much like a Windows process handle with duplicate rights), so it would have to use some kind of broker in the parent. On Windows the sandbox brokers DuplicateHandle, but upstream Chromium seems to have removed that (or at least that interface to the functionality?) in https://crbug.com/493414. [1] http://web.mit.edu/darwin/src/modules/xnu/osfmk/man/ [2] https://codesearch.chromium.org/chromium/src/mojo/core/mach_port_relay.cc?sq=package:chromium&g=0 But there's more simplification that's possible, if we just want a “minimum viable product” instead of trying to support arbitrary port-passing that may or may not be called for. If we use shm_open for shared memory, the only other consumer is graphics, which wants to do Mach IPC alongside the PCompositorBridge protocol. So, ContentParent could pass the task port into GPUProcessManager::CreateContentBridges, and if I understand correctly it could: 1. Allocate a receive right in both the content and GPU processes 2. Extract them into itself as a pair of corresponding send rights 3. Send (queued) each send right to the other process 4. Send each process the mach_port_t for its own receive right as data via IPDL Currently the “GPU process” on Mac is the parent process (I don't know if it's planned or even technically possible to do OOP compositing on Mac) but in that case you'd do the same thing with mach_task_self(). Also, Mach has port sets (MACH_PORT_RIGHT_PORT_SET, mach_port_move_member) for multiplexing receive rights; this may be useful for the compositor (or, alternately, to reduce the overhead of the current design).
FWIW, we do plan on doing OOP compositing on Mac at some point.
I was going to leave this as a low-priority back-burner bug, but this morning I couldn't sleep and had some ideas about this, so I started writing them out as code, and I wound up with a working proof-of-concept of the “minimum viable product” in comment #1. So I guess I own this bug now. Simple version of the API: the parent process creates a pair of `MachEndpoint`s, obtaining task ports from GeckoChildProcessHost; these can be sent over IPDL to the processes designated at construction (or used as-is if it's the same process) and used in that process to initialize a MachBridge, which is basically a ReceivePort+MachPortSender. It's explicit about being its own parallel IPC mechanism, and requires clients to use MachMessage directly, but TextureSync was already doing that. Also, MachEndpoint is an empty stub class on non-Mac so there don't need to be ifdefs everywhere. It should work with OOP compositing, in theory; the port exchange doesn't care what the recipient processes are, and the OOP case at least type-checks. Aside from making Mach IPC more usable, this also means that TextureSync is no longer uncomfortably sharing threads with other uses (and maybe blocking them for a few ms every now and then, although that doesn't seem to have been a performance problem in practice), and there's only 1 server thread per content process instead of 2 (or 4 with OOP compositing). Also it's fewer lines of code than what we're doing now, but maybe not once I'm done commenting things. I'll post the patch once I've at least fixed the build breakage I just got on Try.
Assignee: nobody → jld
Attached patch WIP (deleted) — Splinter Review
Experimental patch with a lot of FIXME and missing comments; based on the WIP patches for bug 1465669 and bug 1479930 (attachment 9008589 [details] [diff] [review] and attachment 9008591 [details] [diff] [review]), respectively. Comment #3 explains the general idea here. (This bug isn't very high priority at the moment — the current system works, and the per-process overhead is higher than I'd like but it's not very significant compared to the other work being done in that area.)
Blocks: 901050
Priority: -- → P3
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: