Closed Bug 440840 Opened 12 years ago Closed 11 years ago

mailcap handling may fail due to race conditions between thread waiting and system()

Categories

(Core Graveyard :: File Handling, defect)

All
Linux
defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla1.9.1a1

People

(Reporter: glandium, Assigned: glandium)

References

Details

Attachments

(1 file, 3 obsolete files)

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!
Attached patch patch (obsolete) — Splinter Review
Replacing system() with nsIProcess seems to fix the issue.
Assignee: nobody → mh+mozilla
Status: NEW → ASSIGNED
Attachment #326043 - Flags: review?(bzbarsky)
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 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()?
(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.
> 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.
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.
Attached patch patch v2 (obsolete) — Splinter Review
What about this ?
Attachment #326043 - Attachment is obsolete: true
Attachment #326100 - Flags: review?(bzbarsky)
Attachment #326043 - Flags: review?(bzbarsky)
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+
Attached patch patch v3 (obsolete) — Splinter Review
Here you are
Attachment #326100 - Attachment is obsolete: true
Attached patch patch v3.0.1Splinter Review
That was the wrong one
Attachment #329193 - Attachment is obsolete: true
Comment on attachment 329194 [details] [diff] [review]
patch v3.0.1

Looks great
Attachment #329194 - Flags: superreview+
Attachment #329194 - Flags: review+
Pushed as 15839:3268e0025bba.
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.1a1
Attachment #329194 - Flags: approval1.9.0.2?
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 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?
I see the same failure to remember associations in 3.1b3pre (nightly build of Firefox, Shiretoko for Windows).
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.