Summary: | Curl backend handles EINTR incorrectly | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Rodney Dawes <dobey> | ||||||||||
Component: | Page Loading | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | alp | ||||||||||
Priority: | P2 | Keywords: | Curl | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | Linux | ||||||||||||
Attachments: |
|
Description
Rodney Dawes
2007-11-20 08:09:26 PST
Created attachment 17418 [details]
Patch to handle EINTR correctly in curl backend
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
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
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? 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? 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. (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. Created attachment 17431 [details]
Defer timers during select()
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.
Landed in r27948. Hrmm. This worked when I tested it, but still fails now in SVN. Comment on attachment 17431 [details]
Defer timers during select()
Clearing the review flag since this patch was landed but the bug must remain open.
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.
Comment on attachment 17776 [details]
Patch to deal with EINTR
r=me
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.
Landed in r28834. Comment on attachment 17976 [details]
Updated patch with ChangeLog
r=darin
|