use posix_spawnp for nsIProcess on Mac OS X

RESOLVED FIXED

Status

()

Core
XPCOM
RESOLVED FIXED
8 years ago
8 years ago

People

(Reporter: Josh Aas, Assigned: Josh Aas)

Tracking

Trunk
All
Mac OS X
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 beta7+)

Details

Attachments

(1 attachment, 4 obsolete attachments)

13.19 KB, patch
Benjamin Smedberg
: review+
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
Created attachment 479594 [details] [diff] [review]
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.
(Assignee)

Updated

8 years ago
Blocks: 599477
(Assignee)

Updated

8 years ago
Attachment #479594 - Flags: review?(benjamin)

Comment 1

8 years ago
Why would we request preference for the current architecture? Just for our tests?
(Assignee)

Comment 2

8 years ago
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

8 years ago
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.
Attachment #479594 - Flags: review?(benjamin) → review-
(Assignee)

Comment 4

8 years ago
Created attachment 479861 [details] [diff] [review]
fix v1.1
Attachment #479594 - Attachment is obsolete: true
Attachment #479861 - Flags: review?(benjamin)

Comment 5

8 years ago
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
Attachment #479861 - Flags: review?(benjamin) → review+
(Assignee)

Comment 6

8 years ago
Created attachment 479890 [details] [diff] [review]
fix v1.2
Attachment #479861 - Attachment is obsolete: true
(Assignee)

Comment 7

8 years ago
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.
Attachment #479890 - Attachment is obsolete: true
(Assignee)

Comment 8

8 years ago
Created attachment 480018 [details] [diff] [review]
fix v1.3

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

Comment 9

8 years ago
Created attachment 480026 [details] [diff] [review]
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.
Attachment #480018 - Attachment is obsolete: true
Attachment #480026 - Flags: review?(benjamin)
(Assignee)

Updated

8 years ago
blocking2.0: --- → beta7+

Updated

8 years ago
Attachment #480026 - Flags: review?(benjamin) → review+
(Assignee)

Comment 10

8 years ago
pushed to mozilla-central

http://hg.mozilla.org/mozilla-central/rev/e089470f19df
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.