Bug 54118

Summary: XMLHttpRequest::abort() doesn't clear response data
Product: WebKit Reporter: Jarred Nicholls <jarred>
Component: XMLAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Minor CC: ap, commit-queue, darin
Priority: P4    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Proposed patch
darin: review-, darin: commit-queue-
Proposed patch
none
Proposed patch
darin: review+
Proposed patch
ap: review-, ap: commit-queue-
Proposed patch none

Description Jarred Nicholls 2011-02-09 10:23:04 PST
In the case of ArrayBuffer responses, abort() will not clear the binary buffer.  So it just wastes space.  Subsequent open() calls (which must come before send(), since abort() resets the state to UNSENT) will clear the buffer properly, but that's not necessarily going to happen.  So this is just more memory efficient.

clearResponse() can replace the code in abort(), because it's accomplishing the same thing, it's neater, and it's one function to maintain; an omission like the above can be better avoided.
Comment 1 Jarred Nicholls 2011-02-09 10:26:52 PST
Created attachment 81831 [details]
Proposed patch
Comment 2 Darin Adler 2011-02-09 10:30:42 PST
Comment on attachment 81831 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81831&action=review

> Source/WebCore/ChangeLog:6
> +        XMLHttpRequest::abort() should call clearResponse()
> +        https://bugs.webkit.org/show_bug.cgi?id=54118

This change log entry is not sufficient. It doesn’t state why this is a good change. As you do in your bug, but more briefly, you need to state the benefit of the change: Less memory used after an abort(), presumably.

> Source/WebCore/ChangeLog:8
> +        No new tests. (OOPS!)

We can’t land a patch with a line like this. You need to change this line into an explanation of why it’s OK to make this change without adding any new tests.
Comment 3 Jarred Nicholls 2011-02-09 10:31:26 PST
I honestly dont' know what I was thinking, I totally meant to type a description in the changelog.  One sec...
Comment 4 Jarred Nicholls 2011-02-09 10:38:07 PST
Created attachment 81833 [details]
Proposed patch
Comment 5 Jarred Nicholls 2011-02-09 10:39:50 PST
Created attachment 81834 [details]
Proposed patch
Comment 6 Darin Adler 2011-02-09 10:49:31 PST
Comment on attachment 81834 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81834&action=review

> Source/WebCore/ChangeLog:16
> +        abort() is not properly clearing ArrayBuffer binary buffered data. This
> +        change will remove the cleanup code from abort() and instead call
> +        clearResponse() to do the cleanup, which not only does the original abort()
> +        cleanup, but also covers the binary response buffer as well. Future cleanup
> +        omissions can be avoided by only having to maintain clearResponse().
> +        
> +        Clearing the data on abort() is more memory efficient.  By not clearing
> +        this data, it will remain in memory until the XHR object is cleaned up or
> +        until a subsequent call to open().

This comment is too long. It would be better to say something like this:

    Calling clearResponse frees more memory at abort time.

    No new tests because there is no observable effect except for less memory used.

The longer paragraph is something people probably won’t read.
Comment 7 Jarred Nicholls 2011-02-09 10:50:26 PST
Ok fair enough, I'll change it for another review.  Thanks Darin.
Comment 8 Jarred Nicholls 2011-02-09 10:53:18 PST
Created attachment 81836 [details]
Proposed patch
Comment 9 Alexey Proskuryakov 2011-02-09 14:53:05 PST
Comment on attachment 81836 [details]
Proposed patch

One hidden change here is that m_response is now cleared. This has observable consequences - for example, XMLHttpRequest.status is lost after aborting.

How did you verify that this is OK?
Comment 10 Jarred Nicholls 2011-02-09 15:07:33 PST
You are right, that behavior is changed.  But the current behavior of status is against the W3 standard as it is.  Resetting the ResourceResponse is the appropriate action, because status should return 0 when the error flag is true, which abort sets.  See http://dev.w3.org/2006/webapi/XMLHttpRequest-2/#the-abort-method and http://dev.w3.org/2006/webapi/XMLHttpRequest-2/#the-status-attribute.  We are actually fixing two things at once here...
Comment 11 Alexey Proskuryakov 2011-02-09 15:22:32 PST
Comment on attachment 81836 [details]
Proposed patch

Thanks, that's convincing. I only picked one example, for a case where I could quickly tell that there is a behavior change. Can you go over uses of m_response in the file and check that it is always expected to have null response after abort()?

> We are actually fixing two things at once here...

I think that this needs a regression test then. Please add one (setting r- to that effect). It would be nice to know if the test passes in both IE and Firefox.
Comment 12 Jarred Nicholls 2011-02-09 15:45:48 PST
Yes I agree, thanks.  I'll create tests and see how other browsers behave, and I'll survey the code affected code thoroughly.
Comment 13 Darin Adler 2011-02-09 15:47:30 PST
(In reply to comment #9)
> One hidden change here is that m_response is now cleared. This has observable consequences - for example, XMLHttpRequest.status is lost after aborting.

I tried to review the class to see what effects clearing m_response would have. Somehow I missed that XMLHttpRequest.status effect.
Comment 14 Jarred Nicholls 2011-02-09 17:08:50 PST
Ok, I am convinced that clearing the response is the right (i.e. safe) thing to do here.  I was debating on whether to revise this patch and create a clearResponseBuffers() function that was used in both clearResponse() and abort().  However, anytime a response error occurs (abort, network, generic, timeout), all of the response attributes/getters (status, statusText, headers, etc.) are not meant to be accessible according to W3.  Therefore, clearing the response "shouldn't matter" if we were compliant.  I would even further do checks on m_state and m_error in status/statusText, but it's technically not necessary since we do proper checks on the response variables; in other words, clearing the response nets the same effect as checking m_state/m_error in status/statusText.

Firefox allows access to status, statusText, and all headers even after an abort.
Opera throws DOM exceptions when accessing any of these fields after a response error/abort.
Haven't tested IE yet.

WebKit will allow access to status and statusText, but throw INVALID_STATE exceptions if trying to retrieve headers.  WebKit is completely inconsistent, and out of the 3 tested browsers, the only inconsistent one.

W3 says that an abort will both 1) set error flag to true, and 2) change the state to UNSENT.  It also says that for status, statusText, and the header getter functions, that if the error flag is true or the status is < HEADERS_RECEIVED, that these response attributes/getters should return 0/""/null values.  We don't follow the standard on status and statusText, and we throw an INVALID_STATE exception on the header getters.  We should make ourselves consistent across the board; but that is a separate issue and we can open another bug for that discussion.

With that said, this patch will get us "one step closer" to the W3 standard by making status and statusText behave according to spec.  We can talk about the header getters and whether an exception is the right thing to do in another conversation.  The spec 

I'll create the regression test(s) and report my findings in IE 6 & 8.
Comment 15 Alexey Proskuryakov 2011-02-09 17:22:08 PST
If the spec doesn't match at least one of IE or Firefox, the spec just needs to be fixed. We should also fix our inconsistencies, of course.

Depending on how much historical baggage we uncover, this choices may not be easy to make. Hopefully, sites don't use XMLHttpRequest.abort() all that much.
Comment 16 Jarred Nicholls 2011-02-09 17:33:24 PST
Yes that's true.  If they do abort(), they apparently don't care about accessing statuses in Opera.  If IE follows the Firefox approach then we have a hard decision to make.  If IE is like Opera, then Firefox is the odd man out and we're golden.

In my experience, I rarely abort; and when I do, I've never been interested in just the status code.  But that's just me :)  If I wanted the status code, I would just wait for readyState to be 2 and then get the code, and if later I wanted to abort, fine...I have the status code captured already.  Meh.
Comment 17 Jarred Nicholls 2011-02-09 19:10:22 PST
Ok, so IE acts like Opera.  It throws exceptions.
Comment 18 Jarred Nicholls 2011-02-09 19:33:53 PST
Created attachment 81913 [details]
Proposed patch

A different patch that doesn't change behavior and stays true to this bug.  Will open a new bug regarding behavior (i.e. internal inconsistencies).
Comment 19 Alexey Proskuryakov 2011-02-10 00:06:20 PST
Comment on attachment 81913 [details]
Proposed patch

Thanks for testing! I'd be fine with following the spec then, and just going with your original patch here (plus a regression test).

The new function name is not so great, because it's not just buffers that are cleared. I'm going to say r+ with hope that this function will go away very soon.
Comment 20 Jarred Nicholls 2011-02-10 00:18:07 PST
(In reply to comment #19)
> (From update of attachment 81913 [details])
> Thanks for testing! I'd be fine with following the spec then, and just going with your original patch here (plus a regression test).
> 
> The new function name is not so great, because it's not just buffers that are cleared. I'm going to say r+ with hope that this function will go away very soon.

See https://bugs.webkit.org/show_bug.cgi?id=54162

We could set this bug as depending upon 54162, and use the original patch (proper regression tests are in 54162).

Let me know what you think.
Comment 21 Jarred Nicholls 2011-02-14 18:00:26 PST
Comment on attachment 81913 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=81913&action=review

Well, might as well get it committed :)

> Source/WebCore/xml/XMLHttpRequest.cpp:687
> +    clearResponseBuffers();

they are all pretty much buffers (hold response data/binary), with the exception of m_createdDocument being a flag.
Comment 22 Darin Adler 2011-02-15 09:15:57 PST
Comment on attachment 81913 [details]
Proposed patch

I agree with Alexey about the fact that the name isn’t great.
Comment 23 WebKit Commit Bot 2011-02-15 11:10:26 PST
Comment on attachment 81913 [details]
Proposed patch

Clearing flags on attachment: 81913

Committed r78591: <http://trac.webkit.org/changeset/78591>
Comment 24 WebKit Commit Bot 2011-02-15 11:10:32 PST
All reviewed patches have been landed.  Closing bug.