Bug 127050 – [XHR] Ensure response return null when error flag is set for blob and arraybuffer

• Status: RESOLVED
• Resolution: FIXED
• Component: XML

[youenn fablet]

According XHR spec, XHR response getters should return null/empty string when the error flag is set.
XHR response getters do not always match that behavior.
This happens for instance if response getters are called within ontimeout callbacks and the HTTP response started to be received (error flag is set and xhr state is DONE).

[youenn fablet]

Created attachment 221270 [details]
First patch

[Build Bot]

Comment on attachment 221270 [details]
First patch

Attachment 221270 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5314705006526464

New failing tests:
fast/files/xhr-response-blob.html

[Build Bot]

Created attachment 221272 [details]
Archive of layout-test-results from webkit-ews-07 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-07  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Build Bot]

Comment on attachment 221270 [details]
First patch

Attachment 221270 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/6184651169202176

New failing tests:
fast/files/xhr-response-blob.html

[Build Bot]

Created attachment 221274 [details]
Archive of layout-test-results from webkit-ews-13 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-13  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.5

[youenn fablet]

Created attachment 221277 [details]
Fixing failing test

[Alexey Proskuryakov]

Comment on attachment 221277 [details]
Fixing failing test

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

> Source/WebCore/xml/XMLHttpRequest.cpp:241
> +    if (m_error)
> +        return "";

Hmm, how are we supposed to handle interrupted connections? We would have responseText attribute have the partial text until the error, at which time it duddenly becomes empty. That's strange.

> Source/WebCore/xml/XMLHttpRequest.cpp:327
> +    if (!doneWithoutErrors())
> +        return nullptr;

Do we now have two places where we essentially null out the response - one here for the ontimeout/onabort case, and another elsewhere for all the cases that already worked correctly?

Can they be merged?

> LayoutTests/http/tests/xmlhttprequest/onabort-response-getters.html:8
> +    <script src="../../../resources/testharness.js"></script>
> +    <script src="../../../resources/testharnessreport.js"></script>

Why the relative paths? I think that this should be just "/resources/testharness.js".

> LayoutTests/http/tests/xmlhttprequest/onabort-response-getters.html:17
> +        var url = "/resources/load-then-wait.cgi?name=../../../http/tests/xmlhttprequest/" + fileName + "&waitFor=0.1&mimeType=" + mimeType

Also unsure about this deep relative path.

[youenn fablet]

(In reply to comment #7)
> (From update of attachment 221277 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221277&action=review
> 
> > Source/WebCore/xml/XMLHttpRequest.cpp:241
> > +    if (m_error)
> > +        return "";
> 
> Hmm, how are we supposed to handle interrupted connections? We would have responseText attribute have the partial text until the error, at which time it duddenly becomes empty. That's strange.

I am not exactly sure to understand which case you are referring to.
Can you give details?

For usual cases, I do not think the behavior is changed.
The response buffer is cleared when m_error is set to true.

One potential case where the patch changes the behavior is if a call to xhr.abort() or xhr.open() triggers a window.onload event (as part of cancelling the loader in internalAbort()).
Inside the window.onload event callback, the m_error flag is already set to true but the response buffer is not yet cleared.

I can see room for improvement in that area (like ensuring that response information remains accessible for the last progress event before the readystatechange DONE event is sent), but that seems like bigger changes than this patch.

> 
> > Source/WebCore/xml/XMLHttpRequest.cpp:327
> > +    if (!doneWithoutErrors())
> > +        return nullptr;
> 
> Do we now have two places where we essentially null out the response - one here for the ontimeout/onabort case, and another elsewhere for all the cases that already worked correctly?
> 
> Can they be merged?

We can factorize the doneWithoutError check in JSXMLHttpRequest::response for all responseType values except ResponseTypeDefault and ResponseTypeText.
This would let the behavior unchanged but the code would look more closely aligned with the spec.
I will upload a new patch accordingly

Note that onabort test works without the patch.
That may change when fixing abort() behavior, as proposed in bug 98404, attachment 221163 [details].
ontimeout cases do not work without the patch right now.

> 
> > LayoutTests/http/tests/xmlhttprequest/onabort-response-getters.html:8
> > +    <script src="../../../resources/testharness.js"></script>
> > +    <script src="../../../resources/testharnessreport.js"></script>
> 
> Why the relative paths? I think that this should be just "/resources/testharness.js".

Right, they should be removed.

> 
> > LayoutTests/http/tests/xmlhttprequest/onabort-response-getters.html:17
> > +        var url = "/resources/load-then-wait.cgi?name=../../../http/tests/xmlhttprequest/" + fileName + "&waitFor=0.1&mimeType=" + mimeType
> 
> Also unsure about this deep relative path.

This is inspired from xmlhttprequest-timeout.js
I will remove it as well.

[youenn fablet]

Created attachment 221356 [details]
factorize doneWithoutError check

[EFL EWS Bot]

Comment on attachment 221356 [details]
factorize doneWithoutError check

Attachment 221356 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/5834558519902208

[EFL EWS Bot]

Comment on attachment 221356 [details]
factorize doneWithoutError check

Attachment 221356 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/4692406255484928

[Build Bot]

Comment on attachment 221356 [details]
factorize doneWithoutError check

Attachment 221356 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/5478046505107456

[Build Bot]

Comment on attachment 221356 [details]
factorize doneWithoutError check

Attachment 221356 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5962754502426624

[youenn fablet]

Created attachment 221374 [details]
fixed compilation error

[youenn fablet]

Created attachment 221377 [details]
Readded precondition in responseXML

[EFL EWS Bot]

Comment on attachment 221377 [details]
Readded precondition in responseXML

Attachment 221377 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/4942846872256512

[youenn fablet]

Created attachment 221380 [details]
Fixed typo

[youenn fablet]

Created attachment 222319 [details]
Made tests more robust

[Darin Adler]

Comment on attachment 222319 [details]
Made tests more robust

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

> Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:-236
> +    default:
> +        return jsUndefined();
>      }
>  
> -    return jsUndefined();

Why this change? The point of leaving out the default is that we get a warning from compilers like clang if any enum value does not have a case. By adding a default, that warning is turned off.

> Source/WebCore/xml/XMLHttpRequest.cpp:291
> +    ASSERT(m_responseTypeCode == ResponseTypeBlob && doneWithoutErrors());

This should be two assertions, not one. If the assertion fails we’d like to know which half failed.

    ASSERT(m_responseTypeCode == ResponseTypeBlob);
    ASSERT(doneWithoutErrors());

> Source/WebCore/xml/XMLHttpRequest.cpp:321
> +    ASSERT(m_responseTypeCode == ResponseTypeArrayBuffer && doneWithoutErrors());

Ditto.

[Alexey Proskuryakov]

> > Hmm, how are we supposed to handle interrupted connections? We would have responseText attribute have the partial text until the error, at which time it duddenly becomes empty. That's strange.

> I am not exactly sure to understand which case you are referring to.
> Can you give details?

Here is the scenario I'm referring to:

1. A load starts.
2. With every onreadystatechange, JS code looks at xhr.responseText, which contains all the data that has been received so far.
3. The server suddenly decides to drop the connection.
4. The error is detected client side.
5. At this point xhr.responseText suddenly becomes empty.

This seems weird.

[Alexey Proskuryakov]

Comment on attachment 222319 [details]
Made tests more robust

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

> LayoutTests/http/tests/xmlhttprequest/onabort-response-getters.html:72
> +    runTest("getting XML response within abort event callback",
> +        "onabort-response-getters.html","text/html",
> +        function(test, client) {},
> +        function(test, client) {assert_true(client.responseXML == null, "xml response must be empty")}
> +    )

I don't think that this test actually works. Parsing a malformed XML document results in a null responseXML anyway.

We'd probably need a test case that has many space characters after the XML, and only abort once the whole XML part has been received.

[youenn fablet]

(In reply to comment #19)
> (From update of attachment 222319 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222319&action=review
> 
> > Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:-236
> > +    default:
> > +        return jsUndefined();
> >      }
> >  
> > -    return jsUndefined();
> 
> Why this change? The point of leaving out the default is that we get a warning from compilers like clang if any enum value does not have a case. By adding a default, that warning is turned off.

Some plaforms use -Werror -Wswitch, leading to failing builds.
That is what happened in https://bugs.webkit.org/attachment.cgi?id=221356

> 
> > Source/WebCore/xml/XMLHttpRequest.cpp:291
> > +    ASSERT(m_responseTypeCode == ResponseTypeBlob && doneWithoutErrors());
> 
> This should be two assertions, not one. If the assertion fails we’d like to know which half failed.
> 
>     ASSERT(m_responseTypeCode == ResponseTypeBlob);
>     ASSERT(doneWithoutErrors());
> 
> > Source/WebCore/xml/XMLHttpRequest.cpp:321
> > +    ASSERT(m_responseTypeCode == ResponseTypeArrayBuffer && doneWithoutErrors());
> 
> Ditto.

OK, I will fix this

[youenn fablet]

(In reply to comment #21)
> (From update of attachment 222319 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222319&action=review
> 
> > LayoutTests/http/tests/xmlhttprequest/onabort-response-getters.html:72
> > +    runTest("getting XML response within abort event callback",
> > +        "onabort-response-getters.html","text/html",
> > +        function(test, client) {},
> > +        function(test, client) {assert_true(client.responseXML == null, "xml response must be empty")}
> > +    )
> 
> I don't think that this test actually works. Parsing a malformed XML document results in a null responseXML anyway.
> 
> We'd probably need a test case that has many space characters after the XML, and only abort once the whole XML part has been received.

I think that the load-then-wait.cgi should already handle that: it sends the whole document and waits some time before ending the response.
It is true though that the test may suffer from changes in the sending of progress events. To ensure that the test is actually working, abort() should be called when onprogress event loaded = total.

[youenn fablet]

(In reply to comment #20)
> > > Hmm, how are we supposed to handle interrupted connections? We would have responseText attribute have the partial text until the error, at which time it duddenly becomes empty. That's strange.
> 
> > I am not exactly sure to understand which case you are referring to.
> > Can you give details?
> 
> Here is the scenario I'm referring to:
> 
> 1. A load starts.
> 2. With every onreadystatechange, JS code looks at xhr.responseText, which contains all the data that has been received so far.
> 3. The server suddenly decides to drop the connection.
> 4. The error is detected client side.
> 5. At this point xhr.responseText suddenly becomes empty.
> 
> This seems weird.
This is already happening today.
In case of a network error, XMLHttpRequest::genericError will probably be called and the response will be cleared before m_error is even set.
In that case, the proposed change in XMLHttpRequest::responseText will not change the behavior (responseText will return "" anyway). It will just change the code path.

The proposed change will only change the behavior in some specific cases when m_error is set to true and the response is not yet cleared.

To ease the review, I will split that change in a different bug.

[youenn fablet]

Created attachment 222587 [details]
Limiting change to arraybuffer and blob

[Alexey Proskuryakov]

Comment on attachment 222587 [details]
Limiting change to arraybuffer and blob

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

> Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:193
> +    // FIXME: Use CachedAttribute for other types than JSON as well.

I'm not sure why this is something we'd want to do.

Other types are pure DOM ones, so caching in XMLHttpRequest is sufficient - and if the returned object has a wrapper already, we'll pick up that wrapper.

I see that you only moved this comment, yet it would be good to clarify it.

[youenn fablet]

(In reply to comment #26)
> (From update of attachment 222587 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=222587&action=review
> 
> > Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp:193
> > +    // FIXME: Use CachedAttribute for other types than JSON as well.
> 
> I'm not sure why this is something we'd want to do.
> 
> Other types are pure DOM ones, so caching in XMLHttpRequest is sufficient - and if the returned object has a wrapper already, we'll pick up that wrapper.
> 
> I see that you only moved this comment, yet it would be good to clarify it.

I guess that, When responseType is not "default", all response builders could be cleared once the response object is in cache.
Response builders are currently cleared for "json", "blob" and "arrayBuffer", but not for "text" and "document".

Since the code for caching is already there, this may also slightly reduce code footprint.

[WebKit Commit Bot]

Comment on attachment 222587 [details]
Limiting change to arraybuffer and blob

Clearing flags on attachment: 222587

Committed r163527: <http://trac.webkit.org/changeset/163527>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.