Bug 54118 – XMLHttpRequest::abort() doesn't clear response data

• Component: XML
• Status: RESOLVED FIXED

[Jarred Nicholls]

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.

[Jarred Nicholls]

Created attachment 81831 [details]
Proposed patch

[Darin Adler]

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.

[Jarred Nicholls]

I honestly dont' know what I was thinking, I totally meant to type a description in the changelog.  One sec...

[Jarred Nicholls]

Created attachment 81833 [details]
Proposed patch

[Jarred Nicholls]

Created attachment 81834 [details]
Proposed patch

[Darin Adler]

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.

[Jarred Nicholls]

Ok fair enough, I'll change it for another review.  Thanks Darin.

[Jarred Nicholls]

Created attachment 81836 [details]
Proposed patch

[Alexey Proskuryakov]

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?

[Jarred Nicholls]

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...

[Alexey Proskuryakov]

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.

[Jarred Nicholls]

Yes I agree, thanks.  I'll create tests and see how other browsers behave, and I'll survey the code affected code thoroughly.

[Darin Adler]

(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.

[Jarred Nicholls]

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.

[Alexey Proskuryakov]

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.

[Jarred Nicholls]

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.

[Jarred Nicholls]

Ok, so IE acts like Opera.  It throws exceptions.

[Jarred Nicholls]

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).

[Alexey Proskuryakov]

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.

[Jarred Nicholls]

(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.

[Jarred Nicholls]

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.

[Darin Adler]

Comment on attachment 81913 [details]
Proposed patch

I agree with Alexey about the fact that the name isn’t great.

[WebKit Commit Bot]

Comment on attachment 81913 [details]
Proposed patch

Clearing flags on attachment: 81913

Committed r78591: <http://trac.webkit.org/changeset/78591>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.