Bug 71122 – properly end requests when a bad status code return happens

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

[Gavin Peters]

properly end requests when a bad status code return happens

[Gavin Peters]

Created attachment 112873 [details]
Patch

[Gavin Peters]

      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.

[Gavin Peters]

japhet: *ping* ?

[Nate Chapin]

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?

[Gavin Peters]

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.

[Gavin Peters]

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.

[Gavin Peters]

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.

[WebKit Review Bot]

Comment on attachment 112873 [details]
Patch

Clearing flags on attachment: 112873

Committed r98987: <http://trac.webkit.org/changeset/98987>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.