1161166 - Replace OS X SharedMemory implementation with something more OS X like

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 2

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 3

[Boris Zbarsky [:bzbarsky]]

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...

Comment 4

[Jed Davis [:jld] ⟨⏰|UTC-7⟩ ⟦he/him⟧]

(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.)

Comment 5

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 6

[Randell Jesup [:jesup] (needinfo me)]

> > 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.

Comment 7

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 8

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: 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()

Comment 9

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 10

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: f_allocate.patch

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.

Comment 11

[Jeff Muizelaar [:jrmuizel]]

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

I'm not the best reviewer for this

Comment 12

[Jeff Muizelaar [:jrmuizel]]

(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

Comment 13

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: 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?

Comment 14

[Jeff Muizelaar [:jrmuizel]]

Here's a document from Chrome about fixing this problem on their side:
https://docs.google.com/document/d/1_dD3M3ZErxeN8BHgV-m4thlIvmuSuf4n1LbWcKMiNsQ/edit

Comment 15

[Mike Conley (:mconley) (:⚙️)]

(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?

Comment 16

[Jeff Muizelaar [:jrmuizel]]

(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

Comment 17

[Brad Lassey [:blassey] (use needinfo?)]

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 18

[Brad Lassey [:blassey] (use needinfo?)]

Comment on attachment 8604255 [details] [diff] [review]
SharedMemoryBasic_posix.patch

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

Bill, are the right reviewer for this?

Comment 19

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 20

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: mach vm WIP

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.

Comment 21

[Brad Lassey [:blassey] (use needinfo?)]

(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

Comment 22

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: SharedMemoryBasic_osx.patch

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

Comment 23

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(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.

Comment 24

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 25

[Brad Lassey [:blassey] (use needinfo?)]

(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.

Comment 26

[Brad Lassey [:blassey] (use needinfo?)]

(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?

Comment 27

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(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.

Comment 28

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: 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.

Comment 29

[Jeff Muizelaar [:jrmuizel]]

(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.

Comment 30

[Matt Woodrow (:mattwoodrow)]

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.

Comment 31

[Jeff Muizelaar [:jrmuizel]]

mstange and I made a test program that just mach_vm_maps directly into the child:
https://github.com/mstange/mach-shared-memory

Comment 32

[Markus Stange [:mstange]]

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

Comment 33

[Jeff Muizelaar [:jrmuizel]]

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];

Comment 34

[Jeff Muizelaar [:jrmuizel]]

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

Comment 35

[Jeff Muizelaar [:jrmuizel]]

For the record here's another chromium doc about mach ipc:
https://docs.google.com/document/d/14-twSrkEhgPI87Pi757auoRhzFYRPqbzdHorHhJn0Kk

Comment 36

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 37

[Jeff Muizelaar [:jrmuizel]]

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.

Comment 38

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8604255 [details] [diff] [review]
SharedMemoryBasic_posix.patch

Will review in the other bug.

Comment 39

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: SharedMemoryBasic_osx.patch

This works, but clearly isn't ready to be reviewed. I'll clean it up tomorrow.

Comment 40

[Brad Lassey [:blassey] (use needinfo?)]

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

Comment 41

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: ipc_headers.patch

This moves the calls to EndianU32_LtoN to the .mm so that the header doesn't pull in unnecessary system headers when included.

Comment 42

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: SharedMemoryBasic_mach.patch

Comment 43

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: SharedMemoryBasic_mach.patch

Comment 44

[George Wright (:gw280) (needinfo me!)]

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 45

[Markus Stange [:mstange]]

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.

Comment 46

[Markus Stange [:mstange]]

(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 47

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 48

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 49

[Brad Lassey [:blassey] (use needinfo?)]

(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.

Comment 50

[Brad Lassey [:blassey] (use needinfo?)]

(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.

Comment 51

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: SharedMemoryBasic_mach.patch

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.

Comment 52

[Brad Lassey [:blassey] (use needinfo?)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=4841f9ecbf9e

This patch supports sharing in both directions, but not between siblings

Comment 53

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 54

[Brad Lassey [:blassey] (use needinfo?)]

(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.

Comment 55

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

(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.

Comment 56

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: SharedMemoryBasic_mach.patch

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.

Comment 57

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: SharedMemoryBasic_mach.patch

here's a stab at cleaning up. 

https://hg.mozilla.org/try/rev/cd326727fa4d

Comment 58

[Brad Lassey [:blassey] (use needinfo?)]

That didn't do the trick

Comment 59

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: SharedMemoryBasic_mach.patch

Strangely, we were leaking the monitor. This fixes that locally.

Comment 60

[Brad Lassey [:blassey] (use needinfo?)]

Try push for that patch https://hg.mozilla.org/try/rev/f495ee2a87c7

Comment 61

[Brad Lassey [:blassey] (use needinfo?)]

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 62

[Brad Lassey [:blassey] (use needinfo?)]

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 63

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: alternate aproach for port timeouts (applies on top)

Comment 64

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 65

[Brad Lassey [:blassey] (use needinfo?)]

(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.

Comment 66

[Brad Lassey [:blassey] (use needinfo?)]

(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()

Comment 67

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: SharedMemoryBasic_mach.patch

Comment 68

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: SharedMemoryBasic_mach.patch

added a few comments

Comment 69

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Really sorry, I had too much stuff to review today. It's at the top of my queue for tomorrow.

Comment 70

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Attachment: fixes

Here are some style fixes and stuff on top of your patch.

Comment 71

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 72

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: ipc_headers.patch patch for checkin

Comment 73

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: SharedMemoryBasic_mach.patch patch for checkin

Comment 74

[Brad Lassey [:blassey] (use needinfo?)]

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])

Comment 75

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8ee308c30146
https://hg.mozilla.org/integration/mozilla-inbound/rev/1be05715932f

Comment 76

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Backed out for apparently making debug OSX mochitest 2 and 3 fail:

https://treeherder.mozilla.org/logviewer.html#?job_id=10554026&repo=mozilla-inbound

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ad406426e392
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b8461156432b

Comment 77

[Ryan VanderMeulen [:RyanVM]]

Confirmed green post-backout.

Comment 78

[Pulsebot]

Backout:
https://hg.mozilla.org/mozilla-central/rev/f4479a936cc6
https://hg.mozilla.org/mozilla-central/rev/d9156ad9b7a2

Comment 79

[Brad Lassey [:blassey] (use needinfo?)]

(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

Comment 80

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: monitors.patch

Comment 81

[Brad Lassey [:blassey] (use needinfo?)]

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).

Comment 82

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: billms_fixes.patch

Comment 83

[Brad Lassey [:blassey] (use needinfo?)]

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.

Comment 84

[Brad Lassey [:blassey] (use needinfo?)]

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 85

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

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.

Comment 86

[Brad Lassey [:blassey] (use needinfo?)]

Attachment: SharedMemoryBasic_mach.patch patch for checkin

Comment 87

[Brad Lassey [:blassey] (use needinfo?)]

Landing order:
* ipc_headers.patch patch for checkin attachment 8617323 [details] [diff] [review] 
* SharedMemoryBasic_mach.patch patch for checkin attachment 8621432 [details] [diff] [review]

Comment 88

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/4055cc03c5c8
https://hg.mozilla.org/integration/mozilla-inbound/rev/bf251327c30e

Comment 89

[Carsten Book [:Tomcat]]

sorry had to back this out, seems this caused https://treeherder.mozilla.org/logviewer.html#?job_id=10708269&repo=mozilla-inbound

Comment 90

[Pulsebot]

Backout:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9f2682a7fd70
https://hg.mozilla.org/integration/mozilla-inbound/rev/34e969da81d9

Comment 91

[Mike Conley (:mconley) (:⚙️)]

Hey blassey, were you still working on this?

Comment 92

[Brad Lassey [:blassey] (use needinfo?)]

I plan to

Comment 93

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/47b6fd494f73
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d6dd2b86d44

Comment 94

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/47b6fd494f73
https://hg.mozilla.org/mozilla-central/rev/7d6dd2b86d44