Replace OS X SharedMemory implementation with something more OS X like

RESOLVED FIXED in Firefox 41

Status

()

defect
RESOLVED FIXED
4 years ago
Last month

People

(Reporter: jrmuizel, Assigned: blassey)

Tracking

(Blocks 2 bugs)

unspecified
mozilla41
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox41 fixed)

Details

Attachments

(5 attachments, 18 obsolete attachments)

3.08 KB, patch
billm
: review+
Details | Diff | Splinter Review
32.74 KB, patch
billm
: review+
Details | Diff | Splinter Review
3.17 KB, patch
Details | Diff | Splinter Review
29.16 KB, patch
billm
: review+
Details | Diff | Splinter Review
38.07 KB, patch
Details | Diff | Splinter Review
Reporter

Description

4 years ago
The current implementation uses memory mapped files. Measuring allocation speed shows that allocations are about 300x slower than our non-shared-memory allocator. 

We are likely better off using an implemenation similar to WebKit's

https://github.com/WebKit/webkit/blob/master/Source/WebKit2/Platform/mac/SharedMemoryMac.cpp

Hopefully, this will solve our performance problems and will help with running into file descriptor limits too.
Duplicate of this bug: 1160831
According to jrmuizel, this is something that might fall under the responsibilities of the IPC folks.

Also, he suggests extracting the Gecko shared memory implementation, and the WebKit shared memory implementations, and doing direct comparisons to ensure that WebKit's is indeed faster (and by how much) before attempting to adopt the solution.
tracking-e10s: --- → ?
Would this possibly fix bug 1122203 because we'd stop using thousands of file descriptors for gfx code?  That would sure be nice, since it'd allow me (and, I suspect, others) to actually start testing e10s...
(In reply to Not doing reviews right now from comment #3)
> Would this possibly fix bug 1122203 because we'd stop using thousands of
> file descriptors for gfx code?

Bug 1122203 seems as if it should also apply to other POSIX platforms; this bug is specifically about OS X.  (And possibly Windows, but I don't know it well enough to have an idea of when it would run into issues with allocating very large numbers of shared memory handles.)

Also, with respect to the comment about ipc::Shmem creation/destruction being slow, I'd point out that GeckoMediaPlugin IPC had problems with that and worked around it with a subsystem-specific caching layer.  Is it time to consider introducing a “shared memory arena” abstraction that can more efficiently provide what shmem users actually want?  (Note that, if we suballocate a file into multiple shared memory regions, passing the fd will grant the process access to any of them – and probably similarly for other native shared memory facilities — so the way this interacts with IPC needs to be considered carefully.)
Reporter

Comment 5

4 years ago
(In reply to Jed Davis [:jld] {UTC-7} from comment #4)
> Also, with respect to the comment about ipc::Shmem creation/destruction
> being slow, I'd point out that GeckoMediaPlugin IPC had problems with that
> and worked around it with a subsystem-specific caching layer.  Is it time to
> consider introducing a “shared memory arena” abstraction that can more
> efficiently provide what shmem users actually want?  (Note that, if we
> suballocate a file into multiple shared memory regions, passing the fd will
> grant the process access to any of them – and probably similarly for other
> native shared memory facilities — so the way this interacts with IPC needs
> to be considered carefully.)

FWIW, we already have a caching layer in front in graphics. In this case where this shows up the cache is probably churning too much, but even with a cache, it still makes sense to use more appropriate APIs on OS X to handle the uncached case.
> > Also, with respect to the comment about ipc::Shmem creation/destruction
> > being slow, I'd point out that GeckoMediaPlugin IPC had problems with that
> > and worked around it with a subsystem-specific caching layer.  Is it time to
> > consider introducing a “shared memory arena” abstraction that can more
> > efficiently provide what shmem users actually want?  
 
> FWIW, we already have a caching layer in front in graphics. In this case
> where this shows up the cache is probably churning too much, but even with a
> cache, it still makes sense to use more appropriate APIs on OS X to handle
> the uncached case.

A shared caching implementation would be handy and avoid roll-it-N-tmes issues (and share bug-bashing).  The GMP sharing implementation is rather ad-hoc and tuned for GMP (and the specifics of the data getting passed); this could be abstracted better for sharing.
Reporter

Comment 7

4 years ago
The difficult part of this might be sending mach ports. I believe we looked at using mach ipc in the past, but I don't remember why we ended up not using it.
Posted patch shm_osx.patch (obsolete) — Splinter Review
I was thinking that using shared memory directly would be better, rather than interacting with the file system. I've hit a wall though in that shmget() can't allocate 260kb, which seems broken.

with my added logging and nspr logging for shm, I get this on the console: 
bool base::SharedMemory::CreateOrOpen(const std::wstring &, int, size_t)
created /var/folders/h0/z9bcn7196znbqf9_wtm31ftc0000gp/T/org.chromium.obtvzf
2059825936[10891c070]: _MD_OpenSharedMemory(): shmget() failed, errno: 12 Cannot allocate memory
2059825936[10891c070]: _MD_OpenSharedMemory(): shmget() failed, size: 266240
2059825936[10891c070]: _MD_OpenSharedMemory(): shmget() failed, path: /var/folders/h0/z9bcn7196znbqf9_wtm31ftc0000gp/T/org.chromium.obtvzf
created shared memory object: 0x0
bool base::SharedMemory::Unmap()
Attachment #8602743 - Flags: feedback?(jmuizelaar)
Reporter

Comment 9

4 years ago
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #8)
> Created attachment 8602743 [details] [diff] [review]
> shm_osx.patch
> 
> I was thinking that using shared memory directly would be better, rather
> than interacting with the file system. I've hit a wall though in that
> shmget() can't allocate 260kb, which seems broken.
> 
> with my added logging and nspr logging for shm, I get this on the console: 
> bool base::SharedMemory::CreateOrOpen(const std::wstring &, int, size_t)
> created /var/folders/h0/z9bcn7196znbqf9_wtm31ftc0000gp/T/org.chromium.obtvzf
> 2059825936[10891c070]: _MD_OpenSharedMemory(): shmget() failed, errno: 12
> Cannot allocate memory
> 2059825936[10891c070]: _MD_OpenSharedMemory(): shmget() failed, size: 266240
> 2059825936[10891c070]: _MD_OpenSharedMemory(): shmget() failed, path:
> /var/folders/h0/z9bcn7196znbqf9_wtm31ftc0000gp/T/org.chromium.obtvzf
> created shared memory object: 0x0
> bool base::SharedMemory::Unmap()

OS X has a very low limit on the amount of sysv shared memory you can create. It looks like you can make NSPR use posix shared memory instead of sysv. You could try that. That being said I still think it would be better to use mach shared memory instead of posix or sysv.
Posted patch f_allocate.patch (obsolete) — Splinter Review
with patch:
  mconley:PresShell::Paint validations
  Samples: 1136
  Total: 2832
  Average: 2.492958
  Median: 0.000000

  mconley:AllocateForSurface times
  Samples: 1316
  Total: 169ms
  Average: 0.129112ms
  Median: 0.169999

without patch:
  mconley:PresShell::Paint validations
  Samples: 1352
  Total: 3045
  Average: 2.252219
  Median: 0.000000

  mconley:AllocateForSurface times
  Samples: 1449
  Total: 210ms
  Average: 0.145557ms
  Median: 0.191457

so this appears to be a 12% improvement. Not earth shattering, but should help.
Attachment #8603032 - Flags: review?(jmuizelaar)
Reporter

Comment 11

4 years ago
Comment on attachment 8603032 [details] [diff] [review]
f_allocate.patch

I'm not the best reviewer for this
Attachment #8603032 - Flags: review?(jmuizelaar)
Reporter

Comment 12

4 years ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #7)
> The difficult part of this might be sending mach ports. I believe we looked
> at using mach ipc in the past, but I don't remember why we ended up not
> using it.

Bug 901050 is the relavant bug
Posted patch SharedMemoryBasic_posix.patch (obsolete) — Splinter Review
So this bubbles things up to the next level, but with an interesting effect. It makes the average much worse but the median much better

mconley:PresShell::Paint validations
Samples: 1938
Total: 4242
Average: 2.188854
Median: 0.000000

mconley:AllocateForSurface times
Samples: 2213
Total: 16630ms
Average: 7.514698ms
Median: 0.005802


Mike, is that the right tradeoff to make?
Attachment #8604255 - Flags: feedback?(mconley)
Reporter

Comment 14

4 years ago
Here's a document from Chrome about fixing this problem on their side:
https://docs.google.com/document/d/1_dD3M3ZErxeN8BHgV-m4thlIvmuSuf4n1LbWcKMiNsQ/edit
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #13)
> Created attachment 8604255 [details] [diff] [review]
> SharedMemoryBasic_posix.patch
> 
> So this bubbles things up to the next level, but with an interesting effect.
> It makes the average much worse but the median much better
> 
> mconley:PresShell::Paint validations
> Samples: 1938
> Total: 4242
> Average: 2.188854
> Median: 0.000000
> 
> mconley:AllocateForSurface times
> Samples: 2213
> Total: 16630ms
> Average: 7.514698ms
> Median: 0.005802
> 
> 
> Mike, is that the right tradeoff to make?

I think I'll have to defer to jrmuizel's expertise to answer that.

Also, Jeff - that document appears to be behind some kind of membership wall. Do you have a static copy we can look at?
Flags: needinfo?(jmuizelaar)
Reporter

Comment 16

4 years ago
(In reply to Jeff Muizelaar [:jrmuizel] from comment #14)
> Here's a document from Chrome about fixing this problem on their side:
> https://docs.google.com/document/d/1_dD3M3ZErxeN8BHgV-
> m4thlIvmuSuf4n1LbWcKMiNsQ/edit

This outlines the different possible approaches for using mach shared memory without having to use mach ipc.
I've made a copy that's public:
https://docs.google.com/document/d/1zsjP8wqOC4e3mtUcrGNpFBR4FixmsqncjAR84_AaQM0/edit?pli=1
my system was getting really laggy when I ran the test for those numbers. I've now rebooted and it looks more sane:

with latest patch:
mconley:PresShell::Paint validations
Samples: 1499
Total: 3677
Average: 2.452969
Median: 0.000000

mconley:AllocateForSurface times
Samples: 2195
Total: 246ms
Average: 0.112095ms
Median: 0.133254

without:
mconley:PresShell::Paint validations
Samples: 751
Total: 2679
Average: 3.567244
Median: 0.000000

mconley:AllocateForSurface times
Samples: 1399
Total: 184ms
Average: 0.131879ms
Median: 0.175780
Comment on attachment 8604255 [details] [diff] [review]
SharedMemoryBasic_posix.patch

Review of attachment 8604255 [details] [diff] [review]:
-----------------------------------------------------------------

Bill, are the right reviewer for this?
Attachment #8604255 - Flags: feedback?(mconley) → feedback?(wmccloskey)
Comment on attachment 8604255 [details] [diff] [review]
SharedMemoryBasic_posix.patch

Review of attachment 8604255 [details] [diff] [review]:
-----------------------------------------------------------------

Could you elaborate on what's changing here? The chromium code seems to use a similar approach to what you're doing (creating a temp file and mmaping it). What part of the chromium code is slower?

::: ipc/glue/SharedMemoryBasic.h
@@ +13,2 @@
>  #else
> +#  include "mozilla/ipc/SharedMemoryBasic_posix.h"

This file isn't in your patch.
Posted patch mach vm WIP (obsolete) — Splinter Review
this doesn't work yet. I suspect what is falling down is the serialization of the mach_port_t. If anyone knows how to do that right, let me know.
(In reply to Bill McCloskey (:billm) from comment #19)
> Comment on attachment 8604255 [details] [diff] [review]
> SharedMemoryBasic_posix.patch
> 
> Review of attachment 8604255 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Could you elaborate on what's changing here? The chromium code seems to use
> a similar approach to what you're doing (creating a temp file and mmaping
> it). What part of the chromium code is slower?
> 
> ::: ipc/glue/SharedMemoryBasic.h
> @@ +13,2 @@
> >  #else
> > +#  include "mozilla/ipc/SharedMemoryBasic_posix.h"
> 
> This file isn't in your patch.

It is, but its just a rename so it doesn't show up in splinter

diff --git a/ipc/glue/SharedMemoryBasic_android.h b/ipc/glue/SharedMemoryBasic_posix.h
rename from ipc/glue/SharedMemoryBasic_android.h
rename to ipc/glue/SharedMemoryBasic_posix.h
Posted patch SharedMemoryBasic_osx.patch (obsolete) — Splinter Review
Made some progress. Rather than fail silently, this now crashes and logs that we couldn't map the memory, presumably because we're not serializing the port correctly 

Failed to map shared memory (266240 bytes). (ipc/send) invalid port right (1000000a)

###!!! [Child][DispatchAsyncMessage] Error: (msgtype=0xFFFC,name=???) Payload error: message could not be deserialized

Failed to map shared memory (8192 bytes). (os/kern) invalid object (1d)

###!!! [Child][DispatchAsyncMessage] Error: (msgtype=0xFFFC,name=???) Payload error: message could not be deserialized

IPDL protocol error: Error deserializing 'data' (Shmem) member of 'SurfaceDescriptorShmem'
IPDL protocol error: Error deserializing 'SurfaceDescriptor'

###!!! [Child][DispatchAsyncMessage] Error: (msgtype=0x7E0006,name=PLayerTransaction::Msg_PTextureConstructor) Value error: message was deserialized, but contained an illegal value

mconley:PresShell::Paint: 2 validations
[Parent 12577] WARNING: pipe error: Broken pipe: file /Users/blassey/src/mozilla-central/ipc/chromium/src/chrome/common/ipc_channel_posix.cc, line 760

###!!! [Parent][MessageChannel] Error: (msgtype=0x200076,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv


###!!! [Parent][MessageChannel] Error: (msgtype=0x200076,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv


###!!! [Parent][MessageChannel] Error: (msgtype=0x200076,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv


###!!! [Parent][MessageChannel] Error: (msgtype=0x200076,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv


###!!! [Parent][MessageChannel] Error: (msgtype=0x200076,name=PBrowser::Msg_Destroy) Channel error: cannot send/recv
Attachment #8602743 - Attachment is obsolete: true
Attachment #8603032 - Attachment is obsolete: true
Attachment #8604491 - Attachment is obsolete: true
Attachment #8602743 - Flags: feedback?(jmuizelaar)
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #21)
> (In reply to Bill McCloskey (:billm) from comment #19)
> > Could you elaborate on what's changing here? The chromium code seems to use
> > a similar approach to what you're doing (creating a temp file and mmaping
> > it). What part of the chromium code is slower?

Could you answer this question? I still don't really understand the motivation for this patch. Is it related to the other one you're working on?

> It is, but its just a rename so it doesn't show up in splinter
> 
> diff --git a/ipc/glue/SharedMemoryBasic_android.h
> b/ipc/glue/SharedMemoryBasic_posix.h
> rename from ipc/glue/SharedMemoryBasic_android.h
> rename to ipc/glue/SharedMemoryBasic_posix.h

OK, I see.
Flags: needinfo?(blassey.bugs)
Reporter

Comment 24

4 years ago
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #22)

It looks like you're just reading and writing the port as an int. That won't work. The google doc has information on a workable approach.
Flags: needinfo?(jmuizelaar)
(In reply to Bill McCloskey (:billm) from comment #23)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #21)
> > (In reply to Bill McCloskey (:billm) from comment #19)
> > > Could you elaborate on what's changing here? The chromium code seems to use
> > > a similar approach to what you're doing (creating a temp file and mmaping
> > > it). What part of the chromium code is slower?
> 
> Could you answer this question? I still don't really understand the
> motivation for this patch. Is it related to the other one you're working on?

Sorry, didn't see this question at first. You're right that it isn't much different. It is somewhat simpler though, keeping any path information as an internal implementation detail (you can't reference the mmaped file by the path) and thus not having to deal with any file path manipulation other than the initial creation.

The real savings I believe comes from avoiding two calls to fstat and one call to fseek. Again, since callers can only get a reference to the mmap'd memory, there is no need to seek within the file.
Flags: needinfo?(blassey.bugs)
(In reply to Jeff Muizelaar [:jrmuizel] from comment #24)
> (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #22)
> 
> It looks like you're just reading and writing the port as an int. That won't
> work. The google doc has information on a workable approach.

I don't have access to that doc. Can you describe that approach here?
Flags: needinfo?(jmuizelaar)
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #26)
> (In reply to Jeff Muizelaar [:jrmuizel] from comment #24)
> > (In reply to Brad Lassey [:blassey] (use needinfo?) from comment #22)
> > 
> > It looks like you're just reading and writing the port as an int. That won't
> > work. The google doc has information on a workable approach.
> 
> I don't have access to that doc. Can you describe that approach here?

Are you sure?  There's a public link in comment 16.  I am able to access that one.
Flags: needinfo?(jmuizelaar)
Posted patch SharedMemoryBasic_osx.patch (obsolete) — Splinter Review
We are exclusively creating the shared memory in the child process and with my current patch we fail to map it into the parent process.

> Failed to map shared memory (266240 bytes) into 707, port cc03. (ipc/send) invalid port right (1000000a)

Since the error its giving is about the port right, I started playing with inserting the port right. But this stuff is almost entirely undocumented for use with shared memory, so I really don't know what I'm doing at this point.
Attachment #8604889 - Attachment is obsolete: true
Reporter

Comment 29

4 years ago
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #28)
> Created attachment 8607588 [details] [diff] [review]
> SharedMemoryBasic_osx.patch
> 
> We are exclusively creating the shared memory in the child process and with
> my current patch we fail to map it into the parent process.
> 
> > Failed to map shared memory (266240 bytes) into 707, port cc03. (ipc/send) invalid port right (1000000a)
> 
> Since the error its giving is about the port right, I started playing with
> inserting the port right. But this stuff is almost entirely undocumented for
> use with shared memory, so I really don't know what I'm doing at this point.

It might be worth trying the other approach suggested in the google doc of just mach_vm_mapping the memory into the child process.
It looks like the mach_port_t is being sent to the other process using a normal (posix) socket.

I believe you need to use the mach ipc (mach_msg) methods with MACH_MSG_PORT_DESCRIPTOR as the data.type value.

That should transfer the rights across to the other process correctly.
Reporter

Comment 31

4 years ago
mstange and I made a test program that just mach_vm_maps directly into the child:
https://github.com/mstange/mach-shared-memory
Hmm, the bootstrap port thing that this does apparently doesn't work on 10.7, see the "Edit (2011.08.28):" part on http://fdiv.net/2011/01/14/machportt-inter-process-communication
Reporter

Comment 33

4 years ago
Chrome registers a bootstrap port as described as the alternative in that link:

   // Register the port with the bootstrap server. Because bootstrap_register
    // is deprecated, this has to be wraped in an ObjC interface.
    NSPort* ns_port = [NSMachPort portWithMachPort:port
                                           options:NSMachPortDeallocateNone];
    NSString* name = base::SysUTF8ToNSString(broker_->GetMachPortName());
    return [[NSMachBootstrapServer sharedInstance] registerPort:ns_port
                                                           name:name];
Reporter

Comment 34

4 years ago
Chrome also uses:
      // Use the kernel audit information to make sure this message is from
      // a task that this process spawned. The kernel audit token contains the
      // unspoofable pid of the task that sent the message.
      //
      // TODO(rsesek): In the 10.7 SDK, there's audit_token_to_pid().
      pid_t child_pid;
      audit_token_to_au32(msg.trailer.msgh_audit,
          NULL, NULL, NULL, NULL, NULL, &child_pid, NULL, NULL);

to ensure that the messages that it gets from the children on the registered port are from actual children
Reporter

Comment 35

4 years ago
For the record here's another chromium doc about mach ipc:
https://docs.google.com/document/d/14-twSrkEhgPI87Pi757auoRhzFYRPqbzdHorHhJn0Kk
FWIW, we do set up already set up a bootstrap port which could be used:
https://mxr.mozilla.org/mozilla-central/source/toolkit/xre/nsEmbedFunctions.cpp#390

I'm going to split off another bug an put the generic posix patch on it.
Depends on: 1167280
Reporter

Comment 37

4 years ago
If we want to allocate in the child we're going to need bug 1167381. If we do the allocation in the parent we can have it map directly into the child.
Depends on: 1167381
Comment on attachment 8604255 [details] [diff] [review]
SharedMemoryBasic_posix.patch

Will review in the other bug.
Attachment #8604255 - Flags: feedback?(wmccloskey)
Assignee: nobody → gwright
Posted patch SharedMemoryBasic_osx.patch (obsolete) — Splinter Review
This works, but clearly isn't ready to be reviewed. I'll clean it up tomorrow.
Assignee: gwright → blassey.bugs
Attachment #8604255 - Attachment is obsolete: true
Attachment #8607588 - Attachment is obsolete: true
measurements:
Samples: 1394
Total: 5663
Average: 4.062410
Median: 0.000000

mconley:AllocateForSurface times
Samples: 3513
Total: 230ms
Average: 0.065688ms
Median: 0.054192

so that looks to be a 4x improvement on the median
This moves the calls to EndianU32_LtoN to the .mm so that the header doesn't pull in unnecessary system headers when included.
Attachment #8613362 - Flags: review?(wmccloskey)
Posted patch SharedMemoryBasic_mach.patch (obsolete) — Splinter Review
Attachment #8613310 - Attachment is obsolete: true
Attachment #8613363 - Flags: review?(wmccloskey)
Posted patch SharedMemoryBasic_mach.patch (obsolete) — Splinter Review
Attachment #8613363 - Attachment is obsolete: true
Attachment #8613363 - Flags: review?(wmccloskey)
Attachment #8613370 - Flags: review?(wmccloskey)
Comment on attachment 8613370 [details] [diff] [review]
SharedMemoryBasic_mach.patch

Review of attachment 8613370 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/SharedMemoryBasic_mach.cpp
@@ +214,5 @@
>    if (!mMemory) {
>      return;
>    }
> +  vm_address_t address = toVMAddress(mMemory);
> +  kern_return_t kr = vm_deallocate(mach_task_self(), address, vm_page_size);

Do we want to deallocate here? I'm not sure, but isn't the behaviour we want to only deallocate when the last mapping goes away? Or is this API just badly named?
Comment on attachment 8613370 [details] [diff] [review]
SharedMemoryBasic_mach.patch

Can you rewrite the send_port / recv_port functions using the chromium mach wrapper classes (MachSendMessage / MachReceiveMessage / MachMsgPortDescriptor)?
It would also be good to add some comments. For example, it would be nice if the places where you have a handle that's only valid in the *other* process.

I like this approach. The fact that you need to block in ShareToProcess while waiting for the port id is  a little unfortunate, but the only way to fix that would be very invasive, so I think this is fine for now.
(In reply to Markus Stange [:mstange] from comment #45)
> It would also be good to add some comments. For example, it would be nice if
> the places where you have a handle that's only valid in the *other* process
... were annotated with a comment.
Comment on attachment 8613370 [details] [diff] [review]
SharedMemoryBasic_mach.patch

Review of attachment 8613370 [details] [diff] [review]:
-----------------------------------------------------------------

The main thing I'd like to see here is a comment explaining how it works. It looks like the parent process starts a service that's supposed to receive a port from a child and reply with a handle that can be used to reference it. But what if the parent wants to send a Shmem to the child? I think we might do that for GMP. Also, we can have IPDL channels between two processes, neither of which is the chrome process. GMP does that too. Maybe I'm just misreading, but this seems bad.

::: ipc/glue/SharedMemoryBasic_mach.cpp
@@ +25,5 @@
>  namespace mozilla {
>  namespace ipc {
>  
> +static kern_return_t
> +recv_port (mach_port_t recv_port, mach_port_t *port)

Can't this be replaced by ReceivePort::WaitForMessage?

@@ +55,5 @@
> +    kern_return_t err = recv_port(ports->receiver->GetPort(), &port);
> +    if (err != KERN_SUCCESS) {
> +      LOG_ERROR("Wait for message failed 0x%x %s\n", err, mach_error_string(err));
> +    } else {
> +      gPorts.push_back(port);

Race on gPorts.

@@ +161,5 @@
>    return true;
>  }
>  
> +static int
> +send_port (mach_port_t remote_port, mach_port_t port)

Similarly, it seems like MachPortSender::SendMessage could substitute here.

@@ +214,5 @@
>    if (!mMemory) {
>      return;
>    }
> +  vm_address_t address = toVMAddress(mMemory);
> +  kern_return_t kr = vm_deallocate(mach_task_self(), address, vm_page_size);

This should be fine. vm_deallocate doesn't affect other processes.
Attachment #8613370 - Flags: review?(wmccloskey) → review-
Comment on attachment 8613362 [details] [diff] [review]
ipc_headers.patch

Review of attachment 8613362 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/chromium/src/chrome/common/mach_ipc_mac.h
@@ -9,5 @@
>  #include <mach/message.h>
>  #include <servers/bootstrap.h>
>  #include <sys/types.h>
>  
> -#include <CoreServices/CoreServices.h>

Do you need to include CoreServices.h in the .mm file?

::: ipc/chromium/src/chrome/common/mach_ipc_mac.mm
@@ +68,5 @@
> +void MachMessage::SetMessageID(int32_t message_id) {
> +  GetDataPacket()->id = EndianU32_NtoL(message_id);
> +}
> +
> +int32_t MachMessage::GetMessageID() { return EndianU32_LtoN(GetDataPacket()->id); }

Please make this three lines like the others.
Attachment #8613362 - Flags: review?(wmccloskey) → review+
(In reply to Bill McCloskey (:billm) from comment #47)
> Comment on attachment 8613370 [details] [diff] [review]
> SharedMemoryBasic_mach.patch
> 
> Review of attachment 8613370 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> The main thing I'd like to see here is a comment explaining how it works. It
> looks like the parent process starts a service that's supposed to receive a
> port from a child and reply with a handle that can be used to reference it.
> But what if the parent wants to send a Shmem to the child? I think we might
> do that for GMP. Also, we can have IPDL channels between two processes,
> neither of which is the chrome process. GMP does that too. Maybe I'm just
> misreading, but this seems bad.

I have only implemented child to parent sharing. Parent to child sharing is relatively trivial to add, but I haven't seen any case (can't say I tested GMP) where this was needed so I left it out. I hadn't considered sharing between siblings, but I don't think it would be terribly complex if you think its needed.

> ::: ipc/glue/SharedMemoryBasic_mach.cpp
> @@ +25,5 @@
> >  namespace mozilla {
> >  namespace ipc {
> >  
> > +static kern_return_t
> > +recv_port (mach_port_t recv_port, mach_port_t *port)
> 
> Can't this be replaced by ReceivePort::WaitForMessage?

No, because the port is shared with different rights than SendMessage imparts. I thought about adding another method to the MachPortSender called SendCopyMessage that would handle that (and the corresponding WaitForCopyMessage in ReceivePort). Would you prefer that? It would essentially be the same code but in those convenience classes.
> 
> @@ +55,5 @@
> > +    kern_return_t err = recv_port(ports->receiver->GetPort(), &port);
> > +    if (err != KERN_SUCCESS) {
> > +      LOG_ERROR("Wait for message failed 0x%x %s\n", err, mach_error_string(err));
> > +    } else {
> > +      gPorts.push_back(port);
> 
> Race on gPorts.
Right, sorry I meant to protect that before putting up for review
> 
> @@ +161,5 @@
> >    return true;
> >  }
> >  
> > +static int
> > +send_port (mach_port_t remote_port, mach_port_t port)
> 
> Similarly, it seems like MachPortSender::SendMessage could substitute here.
See above
> 
> @@ +214,5 @@
> >    if (!mMemory) {
> >      return;
> >    }
> > +  vm_address_t address = toVMAddress(mMemory);
> > +  kern_return_t kr = vm_deallocate(mach_task_self(), address, vm_page_size);
> 
> This should be fine. vm_deallocate doesn't affect other processes.
(In reply to Bill McCloskey (:billm) from comment #48)
> Comment on attachment 8613362 [details] [diff] [review]
> ipc_headers.patch
> 
> Review of attachment 8613362 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: ipc/chromium/src/chrome/common/mach_ipc_mac.h
> @@ -9,5 @@
> >  #include <mach/message.h>
> >  #include <servers/bootstrap.h>
> >  #include <sys/types.h>
> >  
> > -#include <CoreServices/CoreServices.h>
> 
> Do you need to include CoreServices.h in the .mm file?
I don't believe so. The EndianU32_* definitions must be pulled in by another header, but I didn't bother tracking down which.
Posted patch SharedMemoryBasic_mach.patch (obsolete) — Splinter Review
I was wrong about needing to add a method to MachSendPort and ReceivePort. It turns out you can specify the disposition of the port when constructing the MachMsgPortDescriptor.
Attachment #8613370 - Attachment is obsolete: true
Attachment #8614330 - Flags: review?(wmccloskey)
https://treeherder.mozilla.org/#/jobs?repo=try&revision=4841f9ecbf9e

This patch supports sharing in both directions, but not between siblings
Comment on attachment 8614330 [details] [diff] [review]
SharedMemoryBasic_mach.patch

Review of attachment 8614330 [details] [diff] [review]:
-----------------------------------------------------------------

Here's some more feedback. Also, I think the design may have to change significantly to handle child to child shared memory. I'd like to hold off reviewing the next version until you know how you want to implement that.

::: ipc/glue/GeckoChildProcessHost.cpp
@@ +787,5 @@
>    }
>  
> +  ReceivePort* parent_recv_port_memory = new ReceivePort();
> +  if (!parent_message.AddDescriptor(MachMsgPortDescriptor(parent_recv_port_memory->GetPort()))) {
> +    CHROMIUM_LOG(ERROR) << "parent AddDescriptor(" << bootstrap_port << ") failed.";

Message is wrong.

@@ +793,5 @@
> +  }
> +
> +  ReceivePort* parent_send_port_memory_ack = new ReceivePort();
> +  if (!parent_message.AddDescriptor(MachMsgPortDescriptor(parent_send_port_memory_ack->GetPort()))) {
> +    CHROMIUM_LOG(ERROR) << "parent AddDescriptor(" << bootstrap_port << ") failed.";

Same.

::: ipc/glue/SharedMemoryBasic_mach.cpp
@@ +13,3 @@
>  
> +#ifdef DEBUG
> +#define LOG_ERROR(str, args...) {                     \

Should use PR_BEGIN/END_MACRO here.

@@ +13,4 @@
>  
> +#ifdef DEBUG
> +#define LOG_ERROR(str, args...) {                     \
> +  char *msg = PR_smprintf(str, ## args);              \

This leaks the buffer. I guess you could call PR_smprintf_free after NS_WARNING.

@@ +22,2 @@
>  
> +#define CHECK_MACH_ERROR(kr, msg)                                       \

PR_BEGIN_MACRO

@@ +33,5 @@
> +  MachPortSender* sender;
> +  ReceivePort* receiver;
> +};
> +
> +std::map<pid_t, MemoryPorts> gMemoryPorts;

I'm worried about the fact that this map grows but never shrinks. Especially given how many Shmems we currently have (thousands at a time, and maybe millions over the course of a session), this could actually be a real problem.

@@ +36,5 @@
> +
> +std::map<pid_t, MemoryPorts> gMemoryPorts;
> +
> +MemoryPorts*
> +GetMemoryPortsForPid(pid_t pid) {

The brace in a top-level function definition should be on its own line.

@@ +46,5 @@
> +mozilla::Monitor gPortsMonitor("gPorts");
> +std::vector<mach_port_t> gPorts;
> +
> +void *perform_work(void *argument) {
> +  MemoryPorts* ports = (MemoryPorts*)argument;

static_cast

@@ +48,5 @@
> +
> +void *perform_work(void *argument) {
> +  MemoryPorts* ports = (MemoryPorts*)argument;
> +  MachReceiveMessage child_message;
> +  while(true) {

Should be |while (true) {|.

@@ +50,5 @@
> +  MemoryPorts* ports = (MemoryPorts*)argument;
> +  MachReceiveMessage child_message;
> +  while(true) {
> +    MachReceiveMessage rmsg;
> +    kern_return_t err = ports->receiver->WaitForMessage(&rmsg, 1000);

This is going to wake up every second. MACH_MSG_TIMEOUT_NONE should be used instead.

@@ +54,5 @@
> +    kern_return_t err = ports->receiver->WaitForMessage(&rmsg, 1000);
> +    if (KERN_SUCCESS == err) {
> +      mach_port_t port = rmsg.GetTranslatedPort(0);
> +      int id;
> +      gPortsMonitor.Lock();

It's better to use RAII for this sort of thing. MonitorAutoLock in this case.

@@ +70,5 @@
> +  }
> +  delete ports;
> +}
> +
> +std::map<pid_t, pthread_t> gThreads;

The threads are never shut down, even when the process dies. In the parent, that could be a problem since threads have big stacks.

@@ +79,4 @@
>  {
> +  MemoryPorts *listen_ports = new MemoryPorts({listen_port_ack, listen_port});
> +  pthread_t thread;
> +  pthread_create(&thread, NULL, perform_work, listen_ports);

No error checking here.

@@ +120,2 @@
>  {
> +    mach_vm_address_t address;

Use two-space indent.

@@ +125,5 @@
> +        LOG_ERROR("Failed to allocate mach_vm_allocate shared memory (%zu bytes). %s (%x)\n", size, mach_error_string(kr), kr);
> +        return false;
> +    }
> +
> +    memory_object_size_t  memoryObjectSize = round_page(size);

There are two spaces before memoryObjectSize.

@@ +182,4 @@
>      return false;
>    }
> +  MachReceiveMessage msg;
> +   err = ports->receiver->WaitForMessage(&msg, 10000);

Strange indent.

@@ +213,4 @@
>  }
>  
> + bool
> + SharedMemoryBasic::IsHandleValid(const Handle &aHandle){

Indent is weird and brace should be on its own line. And & should go with Handle.

::: toolkit/xre/nsEmbedFunctions.cpp
@@ +368,5 @@
>      NS_WARNING("Adding descriptor to message failed");
>      return NS_ERROR_FAILURE;
>    }
>  
> +  ReceivePort* ports_out_receiver = new ReceivePort();

I don't think there's any reason these couldn't be stack-allocated. Also, it looks like they're never freed right now.

@@ +403,5 @@
>    err = task_set_bootstrap_port(mach_task_self(),
>                                  parent_message.GetTranslatedPort(0));
> +
> +  if (parent_message.GetTranslatedPort(1) == MACH_PORT_NULL) {
> +    NS_WARNING("child GetTranslatedPort(0) failed");

Error message is wrong.

@@ +408,5 @@
> +    return NS_ERROR_FAILURE;
> +  }
> +  MachPortSender* ports_out_sender = new MachPortSender(parent_message.GetTranslatedPort(1));
> +
> +  if (parent_message.GetTranslatedPort(1) == MACH_PORT_NULL) {

Should be 2.

@@ +409,5 @@
> +  }
> +  MachPortSender* ports_out_sender = new MachPortSender(parent_message.GetTranslatedPort(1));
> +
> +  if (parent_message.GetTranslatedPort(1) == MACH_PORT_NULL) {
> +    NS_WARNING("child GetTranslatedPort(0) failed");

Same.
Attachment #8614330 - Flags: review?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #53)
> Comment on attachment 8614330 [details] [diff] [review]
> SharedMemoryBasic_mach.patch
> 
> Review of attachment 8614330 [details] [diff] [review]:
> -----------------------------------------------------------------
> @@ +33,5 @@
> > +  MachPortSender* sender;
> > +  ReceivePort* receiver;
> > +};
> > +
> > +std::map<pid_t, MemoryPorts> gMemoryPorts;
> 
> I'm worried about the fact that this map grows but never shrinks. Especially
> given how many Shmems we currently have (thousands at a time, and maybe
> millions over the course of a session), this could actually be a real
> problem.
These are the ipc ports, not memory regions, so the size limit is the number of processes we've spawned.

> 
> @@ +70,5 @@
> > +  }
> > +  delete ports;
> > +}
> > +
> > +std::map<pid_t, pthread_t> gThreads;
> 
> The threads are never shut down, even when the process dies. In the parent,
> that could be a problem since threads have big stacks.
I'm at a loss of where a good place to clean them up would be. Any ideas?

> 
> ::: toolkit/xre/nsEmbedFunctions.cpp
> @@ +368,5 @@
> >      NS_WARNING("Adding descriptor to message failed");
> >      return NS_ERROR_FAILURE;
> >    }
> >  
> > +  ReceivePort* ports_out_receiver = new ReceivePort();
> 
> I don't think there's any reason these couldn't be stack-allocated. Also, it
> looks like they're never freed right now.
They can't be copied and need to live beyond the scope of this function.
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #54)
> These are the ipc ports, not memory regions, so the size limit is the number
> of processes we've spawned.

Oops. I meant to put that comment on gPorts, not gMemoryPorts.

> I'm at a loss of where a good place to clean them up would be. Any ideas?

We'll probably need to notify the shared memory code when a process shuts down. Maybe from ~GeckoChildProcessHost().

> They can't be copied and need to live beyond the scope of this function.

I don't think they need to live beyond the scope of the function. XRE_InitChildProcess confusingly doesn't just initialize the child---it runs everything too. Notice the code for plugin-container: http://mxr.mozilla.org/mozilla-central/source/ipc/contentproc/plugin-container.cpp#236.
Posted patch SharedMemoryBasic_mach.patch (obsolete) — Splinter Review
try push https://treeherder.mozilla.org/#/jobs?repo=try&revision=806a64a0a41f

This now handles sharing memory between siblings (required for GMP). I expect the try push to leak because I don't clean up the spawned threads. The clean up in the parent process should be relatively strait forward if someone can point me at a place where the parent handles the child shutting down.
Attachment #8614330 - Attachment is obsolete: true
Attachment #8614507 - Flags: review?(wmccloskey)
Posted patch SharedMemoryBasic_mach.patch (obsolete) — Splinter Review
here's a stab at cleaning up. 

https://hg.mozilla.org/try/rev/cd326727fa4d
Attachment #8614507 - Attachment is obsolete: true
Attachment #8614507 - Flags: review?(wmccloskey)
Attachment #8614518 - Flags: review?(wmccloskey)
That didn't do the trick
Posted patch SharedMemoryBasic_mach.patch (obsolete) — Splinter Review
Strangely, we were leaking the monitor. This fixes that locally.
Attachment #8614518 - Attachment is obsolete: true
Attachment #8614518 - Flags: review?(wmccloskey)
Attachment #8614725 - Flags: review?(wmccloskey)
try push (right link this time) https://treeherder.mozilla.org/#/jobs?repo=try&revision=9f7eb7bdc37e looks green except for jetpack tests, which I have a hard time believing are related (Tab not defined).
Comment on attachment 8614725 [details] [diff] [review]
SharedMemoryBasic_mach.patch

Review of attachment 8614725 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/chromium/src/chrome/common/mach_ipc_mac.mm
@@ +265,5 @@
>    out_message->Head()->msgh_reserved = 0;
>    out_message->Head()->msgh_id = 0;
>  
>    kern_return_t result = mach_msg(out_message->Head(),
> +                                  MACH_RCV_MSG | (timeout == MACH_MSG_TIMEOUT_NONE ? 0 : MACH_RCV_TIMEOUT),

I wanted to note that this changes the behavior of WaitForMessage(). Currently, if you pass in a timeout set to MACH_MSG_TIMEOUT_NONE (or 0) the call will return immediately, rather than wait indefinitely. To complicate matters we have two copies of this class in the tree and the one in our breakpad code handles this differently (it waits indefinitely in WaitForMessage, but will return immediately in SendMessage).

Alternatively, we can keep the current behavior, but add a flag to control this behavior explicitly. I'll attach a patch that does that as an alternative.
Comment on attachment 8614725 [details] [diff] [review]
SharedMemoryBasic_mach.patch

Review of attachment 8614725 [details] [diff] [review]:
-----------------------------------------------------------------

I think my brain is melting. Can you explain the design here? It's hard to tell what messages are getting sent to whom. Did you change it so that the parent can no longer share stuff to its children? I think we might need to implement that for non-e10s GMP.

Also, there's a ton of error checking in here. I'm thinking it would be a good idea to just MOZ_CRASH if any of these Mach calls fail. That could be done at the Chromium layer so that SharedMemoryBasic can ignore these problems.

::: ipc/glue/SharedMemoryBasic_mach.cpp
@@ +18,5 @@
> +  NS_WARNING(msg);                              \
> +  PR_smprintf_free(msg);                        \
> +  PR_END_MACRO
> +#else
> +#define LOG_ERROR(str, args...) printf(str, ## args) //do { /* nothing */ } while(0)

Wait, we don't want this printf, right?

@@ +32,3 @@
>  
>  namespace mozilla {
> +  namespace ipc {

No indent for this.

@@ +122,5 @@
> +  }
> +  return &gMemoryPorts.at(pid);
> +}
> +
> +void *perform_work(void *argument)

This function is really long. Can you break it into separate functions, one per message?

@@ +241,5 @@
> +}
> +
> +void
> +SharedMemoryBasic::Shutdown() {
> +  for (auto it = gThreads.begin(); it != gThreads.end(); ++it) {

It looks like you're only cleaning up threads when the process shuts down. We really need to clean them up when the corresponding process shuts down. So if we have a thread for content process P1, we need to shut down that thread when P1 finishes. I think you can add code to ~GeckoChildProcessHost to see when that happens.

@@ +242,5 @@
> +
> +void
> +SharedMemoryBasic::Shutdown() {
> +  for (auto it = gThreads.begin(); it != gThreads.end(); ++it) {
> +    pthread_cancel(it->second.thread);

These sorts of "kill thread immediately" calls are often unreliable. Are you sure this works?

@@ +249,5 @@
> +    delete it->second.ports;
> +  }
> +  gThreads.clear();
> +
> +  for (auto it=gMemoryPorts.begin(); it!=gMemoryPorts.end(); ++it) {

Similarly, we need to delete these ports as soon as we're done with them. Otherwise we'll end up with thousands and thousands of unused ports and probably run out. When the process that originally shared the region runs ~SharedMemoryBasic, I think we can send a message to the parent saying we're done with the port.
Attachment #8614725 - Flags: review?(wmccloskey)
(In reply to Bill McCloskey (:billm) from comment #64)
> Comment on attachment 8614725 [details] [diff] [review]
> @@ +249,5 @@
> > +    delete it->second.ports;
> > +  }
> > +  gThreads.clear();
> > +
> > +  for (auto it=gMemoryPorts.begin(); it!=gMemoryPorts.end(); ++it) {
> 
> Similarly, we need to delete these ports as soon as we're done with them.
> Otherwise we'll end up with thousands and thousands of unused ports and
> probably run out. When the process that originally shared the region runs
> ~SharedMemoryBasic, I think we can send a message to the parent saying we're
> done with the port.

These are the communication ports, but clearly the variable naming isn't clear. I've renamed them to gMemoryCommPorts.
(In reply to Brad Lassey [:blassey] (use needinfo?) from comment #65)
> (In reply to Bill McCloskey (:billm) from comment #64)
> > Comment on attachment 8614725 [details] [diff] [review]
> > @@ +249,5 @@
> > > +    delete it->second.ports;
> > > +  }
> > > +  gThreads.clear();
> > > +
> > > +  for (auto it=gMemoryPorts.begin(); it!=gMemoryPorts.end(); ++it) {
> > 
> > Similarly, we need to delete these ports as soon as we're done with them.
> > Otherwise we'll end up with thousands and thousands of unused ports and
> > probably run out. When the process that originally shared the region runs
> > ~SharedMemoryBasic, I think we can send a message to the parent saying we're
> > done with the port.
> 
> These are the communication ports, but clearly the variable naming isn't
> clear. I've renamed them to gMemoryCommPorts.

I have also realized that I don't need the gPorts map after all, updating the patch to remove that.

Also, the ports associated with memory regions are dellocated in Destroy()
Posted patch SharedMemoryBasic_mach.patch (obsolete) — Splinter Review
Attachment #8614725 - Attachment is obsolete: true
Attachment #8615387 - Flags: review?(wmccloskey)
added a few comments
Attachment #8615387 - Attachment is obsolete: true
Attachment #8615387 - Flags: review?(wmccloskey)
Attachment #8615467 - Flags: review?(wmccloskey)
Really sorry, I had too much stuff to review today. It's at the top of my queue for tomorrow.
Posted patch fixes (obsolete) — Splinter Review
Here are some style fixes and stuff on top of your patch.
Attachment #8617010 - Flags: review?(blassey.bugs)
Attachment #8615467 - Flags: review?(wmccloskey) → review+
Comment on attachment 8614978 [details] [diff] [review]
alternate aproach for port timeouts (applies on top)

Review of attachment 8614978 [details] [diff] [review]:
-----------------------------------------------------------------

Let's just leave it the way it is.
Attachment #8614978 - Flags: review?(wmccloskey)
Attachment #8617010 - Flags: review?(blassey.bugs) → review+
Landing order should be:
* ipc_headers.patch patch for checkin (attachment 8617323 [details] [diff] [review])
* SharedMemoryBasic_mach.patch patch for checkin (attachment 8617324 [details] [diff] [review])
* fixes (attachment 8617010 [details] [diff] [review])
Keywords: checkin-needed
Confirmed green post-backout.
(In reply to Wes Kocher (:KWierso) from comment #76)
> Backed out for apparently making debug OSX mochitest 2 and 3 fail:
This appears to be caused by Bill's follow up patch
with: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0629371dc4f6
without: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bfca1e6e936e
Flags: needinfo?(blassey.bugs)
Posted patch monitors.patch (obsolete) — Splinter Review
Attachment #8617889 - Flags: review?(wmccloskey)
We've been working on a talos test for tab switching for bug 1166132. The test we have shows a good improvement from this:

with patch
16712, 15588, 14884, 15337, 14988

without patch
179192, 124659, 171258, 106777, 106078

that's ms for 100 tab switches. So with this patch we're averaging around 150ms. Without it, more than a second (and quite variable, probably because we're touching the file system).
Attachment #8617010 - Attachment is obsolete: true
Attachment #8617889 - Attachment is obsolete: true
Attachment #8617889 - Flags: review?(wmccloskey)
Attachment #8620956 - Flags: review?(wmccloskey)
Comment on attachment 8617010 [details] [diff] [review]
fixes

Review of attachment 8617010 [details] [diff] [review]:
-----------------------------------------------------------------

::: ipc/glue/SharedMemoryBasic_mach.cpp
@@ +458,5 @@
>        LOG_ERROR("Wait for message failed 0x%x %s\n", err, mach_error_string(err));
> +      continue;
> +    }
> +
> +    StaticMutexAutoLock smal(gMutex);

The crux of the problem seems to be this lock, we deadlock because the Shutdown() function also takes out this lock so the join hangs. Alternatively if we weren't join()'ing, we could spawn a detached thread in pthread_create and the it would shutdown cleanly after Shutdown() exits and releases its lock.
Attachment #8617010 - Flags: review+ → review-
I should also point out that we don't touch either of the objects that this mutex is intended to guard in that function.
Comment on attachment 8620956 [details] [diff] [review]
billms_fixes.patch

Review of attachment 8620956 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for debugging this! It looks like you removed the mutex from PortServerThread, removed the ShutdownObserver and added the Shutdown() method, which is called in XRE_ShutdownChildProcess? That all sounds good except for my comment below.

::: ipc/glue/SharedMemoryBasic_mach.cpp
@@ +371,5 @@
> +    kern_return_t err = ports->mReceiver->WaitForMessage(&rmsg, MACH_MSG_TIMEOUT_NONE);
> +    if (err != KERN_SUCCESS) {
> +      LOG_ERROR("Wait for message failed 0x%x %s\n", err, mach_error_string(err));
> +      continue;
> +    }

We actually do need to acquire the mutex somewhere here. PortServerThread doesn't use it directly, but some of the code it calls does: CleanupForPid requires the mutex and so does SetupMachMemory, which is called by one of these functions below (forget which). PortServerThread just happens to be a convenient place to acquire it for everyone.

Let's fix it this way:
1. get the message with WaitForMessage
2. if (rmsgs.GetMessage() == kShutdownMsg) { do shutdown stuff and return }
3. otherwise, acquire the mutex and run the switch statement as normal but without the kShutdownMsg case

That avoids having to acquire the mutex in each case statement, which is a little ugly.
Attachment #8620956 - Flags: review?(wmccloskey) → review+
Attachment #8614978 - Attachment is obsolete: true
Attachment #8617324 - Attachment is obsolete: true
Landing order:
* ipc_headers.patch patch for checkin attachment 8617323 [details] [diff] [review] 
* SharedMemoryBasic_mach.patch patch for checkin attachment 8621432 [details] [diff] [review]
Keywords: checkin-needed
sorry had to back this out, seems this caused https://treeherder.mozilla.org/logviewer.html#?job_id=10708269&repo=mozilla-inbound
Flags: needinfo?(blassey.bugs)
Flags: needinfo?(blassey.bugs)
Hey blassey, were you still working on this?
Flags: needinfo?(blassey.bugs)
I plan to
Flags: needinfo?(blassey.bugs)
https://hg.mozilla.org/mozilla-central/rev/47b6fd494f73
https://hg.mozilla.org/mozilla-central/rev/7d6dd2b86d44
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla41
You need to log in before you can comment on or make changes to this bug.