Bug 16071 – Curl backend handles EINTR incorrectly

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Rodney Dawes]

The curl backend is handling EINTR incorrectly by just stopping the stream. It instead needs to try again when EINTR occurs in select(). We also need to use perror() to print the error message when debug is enabled, so that the message is useful, and not plainly "select() return -1". Failing to try again on EINTR means that content is not downloadable as it should be. This occurs quite often on YouTube and similar sites which stream data to a plug-in. By trying again on EINTR we can watch the full videos and have the site behave as expected. With this patch, the CPU usage and network usage patterns match those of firefox when watching the same video on YouTube, where this problem occurs.

[Rodney Dawes]

Created attachment 17418 [details]
Patch to handle EINTR correctly in curl backend

[Alp Toker]

Comment on attachment 17418 [details]
Patch to handle EINTR correctly in curl backend

Good catch. The recursion definitely needs to go. See my proposed changes

[Alp Toker]

Comment on attachment 17418 [details]
Patch to handle EINTR correctly in curl backend

Good catch. The recursion definitely needs to go. See my proposed changes

[Alp Toker]

I looked into this a bit more and now have a feeling this is just a symptom of bad curl API use. I'd suspect that it's curl's job to check for situations like this, not ours. Can you find a reproducible non-plugin test case for this?

[Rodney Dawes]

No. I can't seem to reproduce this problem exactly. When I try to load a movie file directly, it just gets dumped to the view, and I think ends up spinlocking somewhere in the process.

Is someone rewriting the network backend and has that code in a state that can replace the current curl code?

[Alp Toker]

Can you try with no modifications except wrapping the select() with this guard?

    setDeferringTimers(true);
    rc = ::select(maxfd + 1, &fdread, &fdwrite, &fdexcep, &timeout);
    setDeferringTimers(false);

Tell me how it goes.

[Rodney Dawes]

(In reply to comment #6)
> Can you try with no modifications except wrapping the select() with this guard?
> 
>     setDeferringTimers(true);
>     rc = ::select(maxfd + 1, &fdread, &fdwrite, &fdexcep, &timeout);
>     setDeferringTimers(false);
> 
> Tell me how it goes.

This seems to also work sufficiently.

[Alp Toker]

Created attachment 17431 [details]
Defer timers during select()

[Mark Rowe (bdash)]

Comment on attachment 17431 [details]
Defer timers during select()

r=me, though I think the while should be written as while (condition) { } if you're going to squish it all on one line.  It's too easy to overlook the semicolon, and some versions of GCC like to emit warnings for code like this.

[Alp Toker]

Landed in r27948.

[Rodney Dawes]

Hrmm. This worked when I tested it, but still fails now in SVN.

[Darin Adler]

Comment on attachment 17431 [details]
Defer timers during select()

Clearing the review flag since this patch was landed but the bug must remain open.

[Rodney Dawes]

Created attachment 17776 [details]
Patch to deal with EINTR

This is the patch I'm currently using to deal with the EINTR case which is very commonly occuring with plug-ins requesting data through the browser.

[Darin Adler]

Comment on attachment 17776 [details]
Patch to deal with EINTR

r=me

[Rodney Dawes]

Created attachment 17976 [details]
Updated patch with ChangeLog

Here's an updated patch that fixes tabs v. spaces in the code, and includes a ChangeLog entry. Can we please get this committed? Thanks.

[Alp Toker]

Landed in r28834.

[Alp Toker]

Comment on attachment 17976 [details]
Updated patch with ChangeLog

r=darin