Closed
Bug 600711
Opened 14 years ago
Closed 14 years ago
use posix_spawnp for nsIProcess on Mac OS X
Categories
(Core :: XPCOM, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | beta7+ |
People
(Reporter: jaas, Assigned: jaas)
References
Details
Attachments
(1 file, 4 obsolete files)
(deleted),
patch
|
benjamin
:
review+
|
Details | Diff | Splinter Review |
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.
Attachment #479594 -
Flags: review?(benjamin)
Comment 1•14 years ago
|
||
Why would we request preference for the current architecture? Just for our tests?
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•14 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-
Attachment #479594 -
Attachment is obsolete: true
Attachment #479861 -
Flags: review?(benjamin)
Comment 5•14 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+
Attachment #479861 -
Attachment is obsolete: true
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
This fixes 'waitpid' usage and records the PID for the spawned process correctly. Still need to fix the argument count bug.
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)
Updated•14 years ago
|
Attachment #480026 -
Flags: review?(benjamin) → review+
Assignee | ||
Comment 10•14 years ago
|
||
pushed to mozilla-central
http://hg.mozilla.org/mozilla-central/rev/e089470f19df
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•