Bug 71122 - properly end requests when a bad status code return happens
Summary: properly end requests when a bad status code return happens
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Gavin Peters
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2011-10-28 09:06 PDT by Gavin Peters
Modified: 2011-11-01 11:55 PDT (History)
4 users (show)

See Also:


Attachments
Patch (2.15 KB, patch)
2011-10-28 09:18 PDT, Gavin Peters
no flags Details | Formatted Diff | Diff
python script to replicate bug in chrome (1.40 KB, text/x-python-script)
2011-11-01 09:43 PDT, Gavin Peters
no flags Details
alternative reproduction (1.30 KB, text/x-python-script)
2011-11-01 09:44 PDT, Gavin Peters
no flags Details

Note You need to log in before you can comment on or make changes to this bug.
Description Gavin Peters 2011-10-28 09:06:31 PDT
properly end requests when a bad status code return happens
Comment 1 Gavin Peters 2011-10-28 09:18:45 PDT
Created attachment 112873 [details]
Patch
Comment 2 Gavin Peters 2011-10-28 09:22:21 PDT
      Calling error without ending the request set up the CachedResourceRequest so that it could
        actually send out two notifyFinished() events.  This probably was the root cause of
        lots of crashing instability; I know from crbug.com/75604 that this bug was causing lots
        of crashes in ScriptRunner/ScriptElement for instance.

        The fix is easy: just properly end the request instead of just calling error, and we won't
        re-notify.

I'm in the process of landing a test for this in chromium first; by instrumenting Chromium's network stack, I was able to create a crash on every load, see http://codereview.chromium.org/8404001/ for the ongoing review of that test.

My plan is:

1. Land that chromium test with a crash expectation.
2. Remove the crash expectation in the chromium test.
3. Land this change.
4. Add a doesn't-crash expectation to the chromium test.

I tried to create a Layout test for this, but it's not really possible to do in Lighthttpd.  I can do a reproduction of this problem with Chromium's network stack using a python based web server.  I am not sure the crash I'm getting would reproduce in a WebKit port that has an in-process network stack.
Comment 3 Gavin Peters 2011-11-01 04:56:56 PDT
japhet: *ping* ?
Comment 4 Nate Chapin 2011-11-01 08:55:57 PDT
Comment on attachment 112873 [details]
Patch

Code looks fine.

I assume the logging code you add to diagnose this will be removed in a separate patch?

Is there any hope of a manual test for this?  I'm guessing not, since it requires doing crazy things with the network?
Comment 5 Gavin Peters 2011-11-01 09:43:42 PDT
Created attachment 113178 [details]
python script to replicate bug in chrome

This script can be used for manual testing in chrome.  It runs as a web server on port 9095, so run this script, and navigate to http://localhost:9095/ locally to see the replication.
Comment 6 Gavin Peters 2011-11-01 09:44:48 PDT
Created attachment 113179 [details]
alternative reproduction

This script also operates as a web server, on port 9096.  Run this script, and navigate to http://localhost:9096/ to replicate this bug.
Comment 7 Gavin Peters 2011-11-01 09:46:41 PDT
Thanks Nate.  I've attached two python scripts that can help reproduce this; they act as web servers, and yes, they do some funny network juju.  I wish there was a good way to test this that fits into our manual testing, but I spent some effort and couldn't.

Is there a mechanism in lighty to grab the socket for a CGI?  If so, the ABORT reproduction (continue.py above) would be possible in a layout test, and I'd like that.

Otherwise, I'll land this soon, and enable a browser_test in chromium to look for regressions.  In a separate CL next week I'll remove all my instrumentation, if this crash actually goes away in the wild after my test is uploaded.
Comment 8 WebKit Review Bot 2011-11-01 11:55:07 PDT
Comment on attachment 112873 [details]
Patch

Clearing flags on attachment: 112873

Committed r98987: <http://trac.webkit.org/changeset/98987>
Comment 9 WebKit Review Bot 2011-11-01 11:55:11 PDT
All reviewed patches have been landed.  Closing bug.