Closed
Bug 440840
Opened 16 years ago
Closed 16 years ago
mailcap handling may fail due to race conditions between thread waiting and system()
Categories
(Core Graveyard :: File Handling, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
mozilla1.9.1a1
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(1 file, 3 obsolete files)
2.21 KB,
patch
|
bzbarsky
:
review+
bzbarsky
:
superreview+
|
Details | Diff | Splinter Review |
There are strange issues happening with /etc/mailcap handling. A quite easy way to reproduce is to try opening the same url, alterning between saving the file and launching an application. Following mailcap parsing with NSPR_LOG_MODULES=HelperAppService:5 shows that when mailcap entries are parsed right before displaying the "Opening ..." dialog, some but not all tests (when there are test=... fields in the mailcap entries) fail, which can, for example, make a different application be selected randomly. Tracing the whole thing revealed that there is some wait() happening in a thread for something unrelated (i wonder if it's not only thread waiting) catching the SIGCHLD from the system() call child ending, instead of the wait() from the system() call itself. The wait from system() then gets a "No child process" error, which is what system() returns, making the mailcap entry marked as not matching. Interestingly, the comment just before the system() call is // XXX this should not use system(), since that can block the UI thread!
Assignee | ||
Comment 1•16 years ago
|
||
Replacing system() with nsIProcess seems to fix the issue.
Assignee | ||
Comment 2•16 years ago
|
||
Comment on attachment 326043 [details] [diff] [review] patch Maybe the last if should read if (NS_SUCCEEDED(process->Run(PR_TRUE, args, 2, NULL))) ?
Comment 3•16 years ago
|
||
Comment on attachment 326043 [details] [diff] [review] patch >+ LOG(("Running Test: %s %d\n", testCommand.get(), rv)); 'rv' should always be success here, right? Why log it? Put single quotes around the %s in the string to make it easy to tell where it starts/ends? >+ if (process->Run(PR_TRUE, args, 2, NULL)) { Uh... have you tested this? Using what tests? Is passing null for the out param even legal? I wouldn't have thought so, given typical IDL contracts... Also, does Run() really throw if the process exits with a nonzero error code? Or should you be looking at process->GetExitValue()?
Assignee | ||
Comment 4•16 years ago
|
||
(In reply to comment #3) > (From update of attachment 326043 [details] [diff] [review]) > >+ LOG(("Running Test: %s %d\n", testCommand.get(), rv)); > > 'rv' should always be success here, right? Why log it? That is a shameful piece left from debugging... > Put single quotes around the %s in the string to make it easy to tell where it > starts/ends? > > >+ if (process->Run(PR_TRUE, args, 2, NULL)) { > > Uh... have you tested this? Using what tests? > > Is passing null for the out param even legal? I wouldn't have thought so, > given typical IDL contracts... Actually, the function does nothing with the argument... it doesn't return a pid. But I agree it doesn't hurt to replace it. > Also, does Run() really throw if the process > exits with a nonzero error code? Or should you be looking at > process->GetExitValue()? Looking more closely to the code, you are right. I shouldn't write such patches late in the night.
Assignee | ||
Comment 5•16 years ago
|
||
> Actually, the function does nothing with the argument... it doesn't return a
> pid. But I agree it doesn't hurt to replace it.
Thinking more about it, even if pid was set in Run(), it shouldn't be set in the blocking case.
Comment 6•16 years ago
|
||
Logically, yes. But the IDL signature is such that passing in NULL means the callee would be perfectly within its rights to crash. So we shouldn't do that.
Assignee | ||
Comment 7•16 years ago
|
||
What about this ?
Attachment #326043 -
Attachment is obsolete: true
Attachment #326100 -
Flags: review?(bzbarsky)
Attachment #326043 -
Flags: review?(bzbarsky)
Comment 8•16 years ago
|
||
Comment on attachment 326100 [details] [diff] [review] patch v2 >+ if (NS_FAILED(rv = process->Init(file))) Break this up into two lines, please? >+ process->GetExitValue(&exitValue); Check the rv of this? With those two changes, r=bzbarsky
Attachment #326100 -
Flags: review?(bzbarsky) → review+
Assignee | ||
Comment 10•16 years ago
|
||
That was the wrong one
Attachment #329193 -
Attachment is obsolete: true
Comment 11•16 years ago
|
||
Comment on attachment 329194 [details] [diff] [review] patch v3.0.1 Looks great
Attachment #329194 -
Flags: superreview+
Attachment #329194 -
Flags: review+
Updated•16 years ago
|
Keywords: checkin-needed
Comment 12•16 years ago
|
||
Pushed as 15839:3268e0025bba.
Status: ASSIGNED → RESOLVED
Closed: 16 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a1
Updated•16 years ago
|
Attachment #329194 -
Flags: approval1.9.0.2?
Comment 13•16 years ago
|
||
I'm not clear of the risk of this patch. There's no test for it (automated or otherwise). Is there any way we can get one before approving for 1.9.0.2? What's the advantage to taking it vs waiting for 1.9.1? (Reed should know better than to request approval on a patch with no info about it...)
Comment 14•16 years ago
|
||
Comment on attachment 329194 [details] [diff] [review] patch v3.0.1 Clearing approval request until comment 13 is addressed.
Attachment #329194 -
Flags: approval1.9.0.2?
Comment 15•16 years ago
|
||
I see the same failure to remember associations in 3.1b3pre (nightly build of Firefox, Shiretoko for Windows).
See Also: → https://launchpad.net/bugs/243704
Updated•8 years ago
|
Product: Core → Core Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•