592951 - use 'posix_spawnp' to launch child processes on Mac OS X

Status: RESOLVED FIXED

(Core :: IPC, defect)

Description

[Josh Aas]

Bug 587747 switched our child process creation code to using fork/execvp instead of 'posix_spawnp'. We need to use 'posix_spawnp' in order to use the spawn attribute created with 'posix_spawnattr_setbinpref_np', which will allow us to select a particular architecture from a universal binary when spawning a child process.

Comment 1

[Josh Aas]

Attachment: fix v0.8

This won't actually work but before I make it work I'd like to know that I'm on the right path here. Does this seem like the right idea Ted?

The biggest remaining problem with this patch is linking - the child binary apparently doesn't have symbols for the mach message code.

Comment 2

[Josh Aas]

Attachment: fix v1.0

Comment 3

[(not currently active) Ted Mielczarek]

Comment on attachment 472177 [details] [diff] [review]
fix v1.0

>diff --git a/ipc/glue/GeckoChildProcessHost.cpp b/ipc/glue/GeckoChildProcessHost.cpp
>--- a/ipc/glue/GeckoChildProcessHost.cpp
>+++ b/ipc/glue/GeckoChildProcessHost.cpp
>@@ -43,6 +43,10 @@
> #include "base/string_util.h"
> #include "chrome/common/chrome_switches.h"
> #include "chrome/common/process_watcher.h"
>+#ifdef XP_MACOSX
>+#include "chrome/common/mach_ipc_mac.h"
>+#include "base/rand_util.h"
>+#endif
> 
> #include "prprf.h"
> 
>@@ -224,9 +228,6 @@ GeckoChildProcessHost::PerformAsyncLaunc
>   }
> 
>   base::ProcessHandle process;
>-#if defined(XP_MACOSX)
>-  task_t child_task;
>-#endif

You can't remove this, this will break the OOP hang detection code.

>+  if (child_message.GetTranslatedPort(0) == MACH_PORT_NULL) {
>+    LOG(ERROR) << "parent GetTranslatedPort(0) failed.";
>+    return false;
>+  }

Just assign directly to child_task and then check child_task against MACH_PORT_NULL.


>+  if (child_message.GetTranslatedPort(1) == MACH_PORT_NULL) {
>+    LOG(ERROR) << "parent GetTranslatedPort(1) failed.";
>+    return false;
>+  }
>+  MachPortSender parent_sender(child_message.GetTranslatedPort(1));
>+
>+  MachSendMessage parent_message(/* id= */0);
>+  if (!parent_message.AddDescriptor(bootstrap_port)) {
>+    LOG(ERROR) << "parent AddDescriptor(" << bootstrap_port << ") failed.";
>+    return false;
>+  }
>+
>+  err = parent_sender.SendMessage(parent_message, kTimeoutMs);
>+  if (err != KERN_SUCCESS) {
>+    std::string errString = StringPrintf("0x%x %s", err, mach_error_string(err));
>+    LOG(ERROR) << "parent SendMessage() failed: " << errString;
>+    return false;
>+  }

I'm not actually sure why the Chromium code sends the parent bootstrap port back to the child, I don't think we actually need that (but you could try removing it and testing).

>diff --git a/toolkit/xre/nsEmbedFunctions.cpp b/toolkit/xre/nsEmbedFunctions.cpp
>--- a/toolkit/xre/nsEmbedFunctions.cpp
>+++ b/toolkit/xre/nsEmbedFunctions.cpp
>@@ -75,6 +75,9 @@
> 
> #include "mozilla/Omnijar.h"
> #ifdef MOZ_IPC
>+#if defined(XP_MACOSX)
>+#include "chrome/common/mach_ipc_mac.h"
>+#endif
> #include "nsX11ErrorHandler.h"
> #include "base/at_exit.h"
> #include "base/command_line.h"
>@@ -309,6 +312,77 @@ XRE_InitChildProcess(int aArgc,
> 
>   sChildProcessType = aProcess;
> 
>+  // Complete 'task_t' exchange for Mac OS X. This structure has the same size
>+  // regardless of architecture so we don't have any cross-arch issues here.
>+#ifdef XP_MACOSX

You're doing this exchange regardless of whether crash reporting is enabled. I'm not sure it matters much, but just thought I'd point that out.

r=me with those changes.

Comment 4

[Josh Aas]

Attachment: fix v1.1

This addresses some of Ted's comments but it doesn't address the comments having to do with what is strictly necessary for crash reporter to work. I don't want to risk messing that up with this patch right now.

This also increases the timeout for the initial data exchange via mach port to 1000ms so that the operation doesn't time out under gdb by default.

Comment 5

[Benjamin Smedberg]

Comment on attachment 473081 [details] [diff] [review]
fix v1.1

I'm on vacation until Monday, please ask ted or cjones for review if you need it this week.

Comment 6

[Josh Aas]

Attachment: fix v1.2

Windows hang fix.

Comment 7

[Josh Aas]

Attachment: fix v1.3

My Windows setup makes bad patches, this is the same Windows hang fix but in a better form.

Comment 8

[Josh Aas]

here are the changesets for the push that was backed out because of the Windows hang:

http://hg.mozilla.org/mozilla-central/rev/52388a9a6337

http://hg.mozilla.org/mozilla-central/rev/9e09716ddaf7
http://hg.mozilla.org/mozilla-central/rev/0f9e4e3b77ec

Comment 9

[Josh Aas]

pushed to mozilla-central with the Windows hang fix

http://hg.mozilla.org/mozilla-central/rev/ab61a67aebb0