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

Status: RESOLVED FIXED

Description

[Mike Hommey [:glandium]]

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!

Comment 1

[Mike Hommey [:glandium]]

Created attachment 326043 [details] [diff] [review]
patch

Replacing system() with nsIProcess seems to fix the issue.

Comment 2

[Mike Hommey [:glandium]]

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

[Boris Zbarsky [:bz] (no decent commit message means r-)]

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()?

Comment 4

[Mike Hommey [:glandium]]

(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.

Comment 5

[Mike Hommey [:glandium]]

> 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

[Boris Zbarsky [:bz] (no decent commit message means r-)]

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.

Comment 7

[Mike Hommey [:glandium]]

Created attachment 326100 [details] [diff] [review]
patch v2

What about this ?

Comment 8

[Boris Zbarsky [:bz] (no decent commit message means r-)]

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

Comment 9

[Mike Hommey [:glandium]]

Created attachment 329193 [details] [diff] [review]
patch v3

Here you are

Comment 10

[Mike Hommey [:glandium]]

Created attachment 329194 [details] [diff] [review]
patch v3.0.1

That was the wrong one

Comment 11

[Boris Zbarsky [:bz] (no decent commit message means r-)]

Comment on attachment 329194 [details] [diff] [review]
patch v3.0.1

Looks great

Comment 12

[Reed Loden [:reed] (use needinfo?)]

Pushed as 15839:3268e0025bba.

Comment 13

[Samuel Sidler (old account; do not CC)]

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

[Samuel Sidler (old account; do not CC)]

Comment on attachment 329194 [details] [diff] [review]
patch v3.0.1

Clearing approval request until comment 13 is addressed.

Comment 15

[Ilguiz Latypov]

I see the same failure to remember associations in 3.1b3pre (nightly build of Firefox, Shiretoko for Windows).