600711 - use posix_spawnp for nsIProcess on Mac OS X

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Josh Aas]

Attachment: fix v1.0

We should use posix_spawnp for nsIProcess on Mac OS X so we can request preference for the current architecture when launching the new process.

Comment 1

[Benjamin Smedberg]

Why would we request preference for the current architecture? Just for our tests?

Comment 2

[Josh Aas]

In case it is used to launch anything from Mozilla, really. Tests, sub-applications, Firefox. I'm not 100% happy with this solution but I haven't settled on another one yet.

Comment 3

[Benjamin Smedberg]

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

>diff --git a/xpcom/threads/nsProcessCommon.cpp b/xpcom/threads/nsProcessCommon.cpp

> //Constructor
> nsProcess::nsProcess()
>-    : mThread(nsnull),
>-      mLock(PR_NewLock()),
>-      mShutdown(PR_FALSE),
>-      mPid(-1),
>-      mObserver(nsnull),
>-      mWeakObserver(nsnull),
>-      mExitValue(-1),
>-      mProcess(nsnull)
>+    :   mThread(nsnull)
>+      , mLock(PR_NewLock())
>+      , mShutdown(PR_FALSE)

This is strange indenting, please make the colon and the commas line up

  : mThread(nsnull)
  , mLock(PR_NewLock())
...

>+    cpu_type_t cpu_types[CPU_ATTR_COUNT];
>+#if defined(__i386__)
>+    cpu_types[0] = CPU_TYPE_X86;
>+#elif defined(__x86_64__)
>+    cpu_types[0] = CPU_TYPE_X86_64;
>+#elif defined(__ppc__)
>+    cpu_types[0] = CPU_TYPE_POWERPC;
>+#endif
>+    cpu_types[1] = CPU_TYPE_ANY;

Can this block be moved into a static-const at the top of the file and use an array-initializer? That way I don't think you need CPU_ATTR_COUNT at all:

static const cpu_type_t cpu_types[] = {
#if defined(__i386__)
  CPU_TYPE_X86,
#...
#endif
  CPU_TYPE_ANY
};

And below you can use use NS_ARRAY_LENGTH for the static array.

Comment 4

[Josh Aas]

Attachment: fix v1.1

Comment 5

[Benjamin Smedberg]

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

+    char* argv_copy[count + 1];

That's a variable-length stack allocation, which makes me nervous. Can we make it a nsAutoArrayPtr<char*> argv_copy = new char*[count + 1] instead?

r=me with that

Comment 6

[Josh Aas]

Attachment: fix v1.2

Comment 7

[Josh Aas]

Comment on attachment 479890 [details] [diff] [review]
fix v1.2

This has a bug, its 'waitpid' usage is incorrect and another bug not introduced by this patch, an incorrect argument count, prevents arguments from being passed correctly. New patch coming up.

Comment 8

[Josh Aas]

Attachment: fix v1.3

This fixes 'waitpid' usage and records the PID for the spawned process correctly. Still need to fix the argument count bug.

Comment 9

[Josh Aas]

Attachment: fix v1.4

This patch fixes some insanity in how arguments are passed through the nsIProcess implementation. Prior to my patch, nsIProcess's "Run" or "RunAsync" gets called with arguments for the application, then they call "CopyArgsAndRunProcess" which creates a NULL-terminated array out of them and starts it with the program path. This new array is thus (2 + arguments) long. Then "CopyArgsAndRunProcess" calls "RunProcess", passing the new array but the old "count" value - just the number of arguments in the array. Any sane person would assume that "count" in "RunProcess" is the length of the array passed. This isn't true though, it means something else which is arbitrary at this point. A "count" value isn't even necessary since the array is null-terminated. Ugh.

This patch fixes all of that and a few other bad things.

Comment 10

[Josh Aas]

pushed to mozilla-central

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