[XHR] Cache response JS object in case of arraybuffer and blob response types
authorcommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 Jul 2016 06:51:05 +0000 (06:51 +0000)
committercommit-queue@webkit.org <commit-queue@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 Jul 2016 06:51:05 +0000 (06:51 +0000)
https://bugs.webkit.org/show_bug.cgi?id=128903

Patch by Youenn Fablet <youenn@apple.com> on 2016-07-20
Reviewed by Alex Christensen.

Source/WebCore:

Covered by existing and modified tests.

Making response getter a JS builtin that caches response in @response private slot.
Handling invalidation of cached response with @responseCacheIsValid new private method.
Handling creation of cached response with @retrieveResponse new private method which reuses most of
JSXMLHttpRequest::response previous code.

Caching of responses is activated whenever load ended without any error for blob and arraybuffer response types.

Caching of response for document is also activated in case the response getter is used but not if responseXML getter is used.

* CMakeLists.txt: Adding XMLHttpRequest.js.
* DerivedSources.make: Ditto.
* bindings/js/JSXMLHttpRequestCustom.cpp:
(WebCore::JSXMLHttpRequest::retrieveResponse): Implements creation of to-be-cached response.
(WebCore::JSXMLHttpRequest::response): Deleted.
* bindings/js/WebCoreBuiltinNames.h: Adding new private names.
* xml/XMLHttpRequest.cpp:
(WebCore::XMLHttpRequest::didCacheResponse): Renamed from didCacheResponseJSON as all response types are now cached.
(WebCore::XMLHttpRequest::didCacheResponseJSON): Deleted.
* xml/XMLHttpRequest.h:
* xml/XMLHttpRequest.idl:

LayoutTests:

* http/tests/xmlhttprequest/onabort-response-getters-expected.txt:
* http/tests/xmlhttprequest/onabort-response-getters.html:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@203494 268f45cc-cd09-0410-ab3c-d52691b4dbfc

12 files changed:
LayoutTests/ChangeLog
LayoutTests/http/tests/xmlhttprequest/onabort-response-getters-expected.txt
LayoutTests/http/tests/xmlhttprequest/onabort-response-getters.html
Source/WebCore/CMakeLists.txt
Source/WebCore/ChangeLog
Source/WebCore/DerivedSources.make
Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp
Source/WebCore/bindings/js/WebCoreBuiltinNames.h
Source/WebCore/xml/XMLHttpRequest.cpp
Source/WebCore/xml/XMLHttpRequest.h
Source/WebCore/xml/XMLHttpRequest.idl
Source/WebCore/xml/XMLHttpRequest.js [new file with mode: 0644]

index d54046e..fa624e3 100644 (file)
@@ -1,3 +1,13 @@
+2016-07-20  Youenn Fablet  <youenn@apple.com>
+
+        [XHR] Cache response JS object in case of arraybuffer and blob response types
+        https://bugs.webkit.org/show_bug.cgi?id=128903
+
+        Reviewed by Alex Christensen.
+
+        * http/tests/xmlhttprequest/onabort-response-getters-expected.txt:
+        * http/tests/xmlhttprequest/onabort-response-getters.html:
+
 2016-07-19  Filip Pizlo  <fpizlo@apple.com>
 
         Switching on symbols should be fast
index 85be3bb..2461cc0 100644 (file)
@@ -1,6 +1,12 @@
 
 PASS getting arraybuffer response within abort event callback 
+PASS getting arraybuffer response within abort event callback (aborting in loadend) 
 PASS getting blob response within abort event callback 
+PASS getting blob response within abort event callback (aborting in loadend) 
 PASS getting json response within abort event callback 
+PASS getting json response within abort event callback (aborting in loadend) 
 PASS getting document response within abort event callback 
+PASS getting document response within abort event callback (aborting in loadend) 
+PASS getting text response within abort event callback 
+PASS getting text response within abort event callback (aborting in loadend) 
 
index e31aa93..ae227d2 100644 (file)
@@ -7,6 +7,14 @@
   <body>
     <div id="log"></div>
     <script>
+    function checkCachedResponse(client) {
+        assert_true(client.response === client.response);
+        if (client.responseType == "text")
+            assert_true(client.responseText === client.response);
+        if (client.responseType == "document")
+            assert_true(client.responseXML === client.response);
+    }
+
     function runTest(name, fileName, mimeType, setupClient, checkResponse) {
       var test = async_test(name)
       test.step(function() {
@@ -28,6 +36,7 @@
             if (client.readyState == 4) {
                 checkResponse(test, client)
                 client.isResponseChecked = true
+                checkCachedResponse(client);
             }
         })
 
         client.onloadend = test.step_func(function () {
             assert_true(client.hasAborted, "xhr should have aborted")
             assert_true(client.isResponseChecked, "xhr response should have been checked")
+            checkCachedResponse(client);
             test.done()
         })
         client.send(null)
       })
+
+      var test2 = async_test(name + " (aborting in loadend)")
+      test2.step(function() {
+        var client = new XMLHttpRequest()
+        var url = "/resources/load-then-wait.cgi?name=../xmlhttprequest/" + fileName + "&waitFor=1&mimeType=" + mimeType
+        client.open("GET", url, true)
+        setupClient(test2, client)
+
+        client.onloadend = test2.step_func(function () {
+            assert_true(client.response != null);
+            checkCachedResponse(client);
+            client.abort();
+            checkResponse(test2, client);
+            checkCachedResponse(client);
+            test2.done();
+        })
+        client.send(null)
+      })
     }
 
     runTest("getting arraybuffer response within abort event callback",
         function(test, client) {assert_true(client.response == null, "document response must be empty")}
     )
 
+    runTest("getting text response within abort event callback",
+        "resources/test.json","text/plain",
+        function(test, client) {client.responseType = "text"},
+        function(test, client) {assert_true(client.response == "", "text response must be an empty string")}
+    )
+
     </script>
   </body>
 </html>
index 7a661d6..c01aef8 100644 (file)
@@ -3726,6 +3726,7 @@ set(WebCore_BUILTINS_SOURCES
     ${WEBCORE_DIR}/Modules/streams/StreamInternals.js
     ${WEBCORE_DIR}/Modules/streams/WritableStream.js
     ${WEBCORE_DIR}/Modules/streams/WritableStreamInternals.js
+    ${WEBCORE_DIR}/xml/XMLHttpRequest.js
 )
 
 set(BUILTINS_GENERATOR_SCRIPTS
index f24ee06..3cb0990 100644 (file)
@@ -1,5 +1,35 @@
 2016-07-20  Youenn Fablet  <youenn@apple.com>
 
+        [XHR] Cache response JS object in case of arraybuffer and blob response types
+        https://bugs.webkit.org/show_bug.cgi?id=128903
+
+        Reviewed by Alex Christensen.
+
+        Covered by existing and modified tests.
+
+        Making response getter a JS builtin that caches response in @response private slot.
+        Handling invalidation of cached response with @responseCacheIsValid new private method.
+        Handling creation of cached response with @retrieveResponse new private method which reuses most of
+        JSXMLHttpRequest::response previous code.
+
+        Caching of responses is activated whenever load ended without any error for blob and arraybuffer response types.
+
+        Caching of response for document is also activated in case the response getter is used but not if responseXML getter is used.
+
+        * CMakeLists.txt: Adding XMLHttpRequest.js.
+        * DerivedSources.make: Ditto.
+        * bindings/js/JSXMLHttpRequestCustom.cpp:
+        (WebCore::JSXMLHttpRequest::retrieveResponse): Implements creation of to-be-cached response.
+        (WebCore::JSXMLHttpRequest::response): Deleted.
+        * bindings/js/WebCoreBuiltinNames.h: Adding new private names.
+        * xml/XMLHttpRequest.cpp:
+        (WebCore::XMLHttpRequest::didCacheResponse): Renamed from didCacheResponseJSON as all response types are now cached.
+        (WebCore::XMLHttpRequest::didCacheResponseJSON): Deleted.
+        * xml/XMLHttpRequest.h:
+        * xml/XMLHttpRequest.idl:
+
+2016-07-20  Youenn Fablet  <youenn@apple.com>
+
         Remove crossOriginRequestPolicy from ThreadableLoaderOptions
         https://bugs.webkit.org/show_bug.cgi?id=159417
 
index 9d489e2..b8524bc 100644 (file)
@@ -1299,6 +1299,7 @@ WebCore_BUILTINS_SOURCES = \
     $(WebCore)/Modules/streams/StreamInternals.js \
     $(WebCore)/Modules/streams/WritableStream.js \
     $(WebCore)/Modules/streams/WritableStreamInternals.js \
+    $(WebCore)/xml/XMLHttpRequest.js \
 #
 
 BUILTINS_GENERATOR_SCRIPTS = \
index 44225f3..523db16 100644 (file)
@@ -63,12 +63,6 @@ void JSXMLHttpRequest::visitAdditionalChildren(SlotVisitor& visitor)
 
     if (Document* responseDocument = wrapped().optionalResponseXML())
         visitor.addOpaqueRoot(responseDocument);
-
-    if (ArrayBuffer* responseArrayBuffer = wrapped().optionalResponseArrayBuffer())
-        visitor.addOpaqueRoot(responseArrayBuffer);
-
-    if (Blob* responseBlob = wrapped().optionalResponseBlob())
-        visitor.addOpaqueRoot(responseBlob);
 }
 
 class SendFunctor {
@@ -151,12 +145,8 @@ JSValue JSXMLHttpRequest::responseText(ExecState& state) const
     return jsOwnedStringOrNull(&state, text);
 }
 
-JSValue JSXMLHttpRequest::response(ExecState& state) const
+JSValue JSXMLHttpRequest::retrieveResponse(ExecState& state)
 {
-    // FIXME: Use CachedAttribute for other types than JSON as well.
-    if (m_response && wrapped().responseCacheIsValid())
-        return m_response.get();
-
     auto type = wrapped().responseType();
 
     switch (type) {
@@ -170,42 +160,36 @@ JSValue JSXMLHttpRequest::response(ExecState& state) const
     if (!wrapped().doneWithoutErrors())
         return jsNull();
 
+    JSValue value;
     switch (type) {
     case XMLHttpRequest::ResponseType::EmptyString:
     case XMLHttpRequest::ResponseType::Text:
         ASSERT_NOT_REACHED();
-        break;
+        return jsUndefined();
 
     case XMLHttpRequest::ResponseType::Json:
-        {
-            JSValue value = JSONParse(&state, wrapped().responseTextIgnoringResponseType());
-            if (!value)
-                value = jsNull();
-            m_response.set(state.vm(), this, value);
-            wrapped().didCacheResponseJSON();
-            return value;
-        }
-
-    case XMLHttpRequest::ResponseType::Document:
-        {
-            ExceptionCode ec = 0;
-            Document* document = wrapped().responseXML(ec);
-            if (ec) {
-                setDOMException(&state, ec);
-                return jsUndefined();
-            }
-            return toJS(&state, globalObject(), document);
-        }
+        value = JSONParse(&state, wrapped().responseTextIgnoringResponseType());
+        if (!value)
+            value = jsNull();
+        break;
 
+    case XMLHttpRequest::ResponseType::Document: {
+        ExceptionCode ec = 0;
+        auto document = wrapped().responseXML(ec);
+        ASSERT(!ec);
+        value = toJS(&state, globalObject(), document);
+        break;
+    }
     case XMLHttpRequest::ResponseType::Blob:
-        return toJS(&state, globalObject(), wrapped().responseBlob());
+        value = toJSNewlyCreated(&state, globalObject(), wrapped().createResponseBlob());
+        break;
 
     case XMLHttpRequest::ResponseType::Arraybuffer:
-        return toJS(&state, globalObject(), wrapped().responseArrayBuffer());
+        value = toJS(&state, globalObject(), wrapped().createResponseArrayBuffer());
+        break;
     }
-
-    ASSERT_NOT_REACHED();
-    return jsUndefined();
+    wrapped().didCacheResponse();
+    return value;
 }
 
 } // namespace WebCore
index f07f070..aad72dc 100644 (file)
@@ -62,6 +62,9 @@ namespace WebCore {
     macro(readRequests) \
     macro(readyPromiseCapability) \
     macro(removeTrack) \
+    macro(responseCacheIsValid) \
+    macro(retrieveResponse) \
+    macro(response) \
     macro(setStatus) \
     macro(state) \
     macro(started) \
@@ -85,6 +88,7 @@ namespace WebCore {
     macro(ReadableStreamController) \
     macro(RTCIceCandidate) \
     macro(RTCSessionDescription) \
+    macro(XMLHttpRequest)
 
 class WebCoreBuiltinNames {
 public:
index 26ed750..ff6105c 100644 (file)
@@ -171,9 +171,8 @@ String XMLHttpRequest::responseText(ExceptionCode& ec)
     return responseTextIgnoringResponseType();
 }
 
-void XMLHttpRequest::didCacheResponseJSON()
+void XMLHttpRequest::didCacheResponse()
 {
-    ASSERT(m_responseType == ResponseType::Json);
     ASSERT(doneWithoutErrors());
     m_responseCacheIsValid = true;
     m_responseBuilder.clear();
@@ -218,42 +217,30 @@ Document* XMLHttpRequest::responseXML(ExceptionCode& ec)
     return m_responseDocument.get();
 }
 
-Blob* XMLHttpRequest::responseBlob()
+Ref<Blob> XMLHttpRequest::createResponseBlob()
 {
     ASSERT(m_responseType == ResponseType::Blob);
     ASSERT(doneWithoutErrors());
 
-    if (!m_responseBlob) {
-        if (m_binaryResponseBuilder) {
-            // FIXME: We just received the data from NetworkProcess, and are sending it back. This is inefficient.
-            Vector<uint8_t> data;
-            data.append(m_binaryResponseBuilder->data(), m_binaryResponseBuilder->size());
-            String normalizedContentType = Blob::normalizedContentType(responseMIMEType()); // responseMIMEType defaults to text/xml which may be incorrect.
-            m_responseBlob = Blob::create(WTFMove(data), normalizedContentType);
-            m_binaryResponseBuilder = nullptr;
-        } else {
-            // If we errored out or got no data, we still return a blob, just an empty one.
-            m_responseBlob = Blob::create();
-        }
-    }
+    if (!m_binaryResponseBuilder)
+        return Blob::create();
 
-    return m_responseBlob.get();
+    // FIXME: We just received the data from NetworkProcess, and are sending it back. This is inefficient.
+    Vector<uint8_t> data;
+    data.append(m_binaryResponseBuilder->data(), m_binaryResponseBuilder->size());
+    m_binaryResponseBuilder = nullptr;
+    String normalizedContentType = Blob::normalizedContentType(responseMIMEType()); // responseMIMEType defaults to text/xml which may be incorrect.
+    return Blob::create(WTFMove(data), normalizedContentType);
 }
 
-ArrayBuffer* XMLHttpRequest::responseArrayBuffer()
+RefPtr<ArrayBuffer> XMLHttpRequest::createResponseArrayBuffer()
 {
     ASSERT(m_responseType == ResponseType::Arraybuffer);
     ASSERT(doneWithoutErrors());
 
-    if (!m_responseArrayBuffer) {
-        if (m_binaryResponseBuilder)
-            m_responseArrayBuffer = m_binaryResponseBuilder->createArrayBuffer();
-        else
-            m_responseArrayBuffer = ArrayBuffer::create(nullptr, 0);
-        m_binaryResponseBuilder = nullptr;
-    }
-
-    return m_responseArrayBuffer.get();
+    auto result = m_binaryResponseBuilder ? m_binaryResponseBuilder->createArrayBuffer() : ArrayBuffer::create(nullptr, 0);
+    m_binaryResponseBuilder = nullptr;
+    return result;
 }
 
 void XMLHttpRequest::setTimeout(unsigned timeout, ExceptionCode& ec)
@@ -819,9 +806,7 @@ void XMLHttpRequest::clearResponseBuffers()
     m_responseEncoding = String();
     m_createdDocument = false;
     m_responseDocument = nullptr;
-    m_responseBlob = nullptr;
     m_binaryResponseBuilder = nullptr;
-    m_responseArrayBuffer = nullptr;
     m_responseCacheIsValid = false;
 }
 
index e49ffd5..c3ed3be 100644 (file)
@@ -88,15 +88,18 @@ public:
     String responseText(ExceptionCode&);
     String responseTextIgnoringResponseType() const { return m_responseBuilder.toStringPreserveCapacity(); }
     String responseMIMEType() const;
-    Document* responseXML(ExceptionCode&);
+
     Document* optionalResponseXML() const { return m_responseDocument.get(); }
-    Blob* responseBlob();
-    Blob* optionalResponseBlob() const { return m_responseBlob.get(); }
+    Document* responseXML(ExceptionCode&);
+
+    Ref<Blob> createResponseBlob();
+    RefPtr<JSC::ArrayBuffer> createResponseArrayBuffer();
+
     unsigned timeout() const { return m_timeoutMilliseconds; }
     void setTimeout(unsigned timeout, ExceptionCode&);
 
     bool responseCacheIsValid() const { return m_responseCacheIsValid; }
-    void didCacheResponseJSON();
+    void didCacheResponse();
 
     // Expose HTTP validation methods for other untrusted requests.
     static bool isAllowedHTTPMethod(const String&);
@@ -109,10 +112,6 @@ public:
 
     String responseURL() const;
 
-    // response attribute has custom getter.
-    JSC::ArrayBuffer* responseArrayBuffer();
-    JSC::ArrayBuffer* optionalResponseArrayBuffer() const { return m_responseArrayBuffer.get(); }
-
     void setLastSendLineAndColumnNumber(unsigned lineNumber, unsigned columnNumber);
     void setLastSendURL(const String& url) { m_lastSendURL = url; }
 
@@ -187,7 +186,6 @@ private:
     String m_mimeTypeOverride;
     bool m_async { true };
     bool m_includeCredentials { false };
-    RefPtr<Blob> m_responseBlob;
 
     RefPtr<ThreadableLoader> m_loader;
     State m_state { UNSENT };
@@ -201,9 +199,8 @@ private:
     StringBuilder m_responseBuilder;
     bool m_createdDocument { false };
     RefPtr<Document> m_responseDocument;
-    
+
     RefPtr<SharedBuffer> m_binaryResponseBuilder;
-    RefPtr<JSC::ArrayBuffer> m_responseArrayBuffer;
 
     bool m_error { false };
 
index ee51f37..e4c87e0 100644 (file)
@@ -44,6 +44,8 @@ enum XMLHttpRequestResponseType {
     JSGenerateToNativeObject,
     JSGenerateToJSObject,
     ExportMacro=WEBCORE_EXPORT,
+    PublicIdentifier,
+    PrivateIdentifier,
 ] interface XMLHttpRequest : XMLHttpRequestEventTarget {
     attribute EventHandler onreadystatechange;
 
@@ -78,12 +80,15 @@ enum XMLHttpRequestResponseType {
     [GetterRaisesException] readonly attribute Document responseXML;
 
     [SetterRaisesException] attribute XMLHttpRequestResponseType responseType;
-    [GetterRaisesException, CachedAttribute, CustomGetter] readonly attribute Object response;
+    [JSBuiltin] readonly attribute Object response;
 
     readonly attribute unsigned short status;
     readonly attribute DOMString statusText;
     readonly attribute DOMString responseURL;
 
+    [PrivateIdentifier] boolean responseCacheIsValid();
+    [PrivateIdentifier, Custom] any retrieveResponse();
+
     // Extension
     [RaisesException] void overrideMimeType(DOMString override);
 };
diff --git a/Source/WebCore/xml/XMLHttpRequest.js b/Source/WebCore/xml/XMLHttpRequest.js
new file mode 100644 (file)
index 0000000..39d27c8
--- /dev/null
@@ -0,0 +1,39 @@
+/*
+ * Copyright (C) 2016 Apple Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions
+ * are met:
+ * 1. Redistributions of source code must retain the above copyright
+ *    notice, this list of conditions and the following disclaimer.
+ * 2. Redistributions in binary form must reproduce the above copyright
+ *    notice, this list of conditions and the following disclaimer in the
+ *    documentation and/or other materials provided with the distribution.
+ *
+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY
+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE INC. OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+function response()
+{
+    "use strict";
+
+    // FIXME: Add a helper routine for that kind of checks.
+    if (!(this instanceof @XMLHttpRequest))
+        throw new @TypeError("The XMLHttpRequest.response getter can only be used on instances of XMLHttpRequest");
+
+    if (@XMLHttpRequest.prototype.@responseCacheIsValid.@call(this))
+        return this.@response;
+
+    this.@response = @XMLHttpRequest.prototype.@retrieveResponse.@call(this);
+    return this.@response;
+}