Changeset 203494 in webkit

Timestamp:

Jul 20, 2016, 11:51:05 PM (10 years ago)

Author:

commit-queue@webkit.org

Message:

[XHR] Cache response JS object in case of arraybuffer and blob response types
​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:

Location:

trunk

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/http/tests/xmlhttprequest/onabort-response-getters-expected.txt (modified) (1 diff)
• LayoutTests/http/tests/xmlhttprequest/onabort-response-getters.html (modified) (4 diffs)
• Source/WebCore/CMakeLists.txt (modified) (1 diff)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/DerivedSources.make (modified) (1 diff)
• Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp (modified) (3 diffs)
• Source/WebCore/bindings/js/WebCoreBuiltinNames.h (modified) (2 diffs)
• Source/WebCore/xml/XMLHttpRequest.cpp (modified) (3 diffs)
• Source/WebCore/xml/XMLHttpRequest.h (modified) (4 diffs)
• Source/WebCore/xml/XMLHttpRequest.idl (modified) (3 diffs)
• Source/WebCore/xml/XMLHttpRequest.js (added)

trunk/LayoutTests/ChangeLog

@@ +1 @@
+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>
 

trunk/LayoutTests/http/tests/xmlhttprequest/onabort-response-getters-expected.txt

@@ -1 +1 @@
 
 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) 
 

trunk/LayoutTests/http/tests/xmlhttprequest/onabort-response-getters.html

@@ -8 +8 @@
     <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)
@@ -29 +37 @@
                 checkResponse(test, client)
                 client.isResponseChecked = true
+                checkCachedResponse(client);
             }
         })
@@ -39 +48 @@
             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)
@@ -69 +97 @@
     )
 
+    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>

trunk/Source/WebCore/CMakeLists.txt

@@ -3727 +3727 @@
     ${WEBCORE_DIR}/Modules/streams/WritableStream.js
     ${WEBCORE_DIR}/Modules/streams/WritableStreamInternals.js
+    ${WEBCORE_DIR}/xml/XMLHttpRequest.js
 )
 

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/DerivedSources.make

@@ -1300 +1300 @@
     $(WebCore)/Modules/streams/WritableStream.js \
     $(WebCore)/Modules/streams/WritableStreamInternals.js \
+    $(WebCore)/xml/XMLHttpRequest.js \
 #
 

trunk/Source/WebCore/bindings/js/JSXMLHttpRequestCustom.cpp

@@ -64 +64 @@
     if (Document* responseDocument = wrapped().optionalResponseXML())
         visitor.addOpaqueRoot(responseDocument);
-
-    if (ArrayBuffer* responseArrayBuffer = wrapped().optionalResponseArrayBuffer())
-        visitor.addOpaqueRoot(responseArrayBuffer);
-
-    if (Blob* responseBlob = wrapped().optionalResponseBlob())
-        visitor.addOpaqueRoot(responseBlob);
 }
 
@@ -152 +146 @@
 }
 
-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();
 
@@ -171 +161 @@
         return jsNull();
 
+    JSValue value;
     switch (type) {
     case XMLHttpRequest::ResponseType::EmptyString:
     case XMLHttpRequest::ResponseType::Text:
         ASSERT_NOT_REACHED();
+        return jsUndefined();
+
+    case XMLHttpRequest::ResponseType::Json:
+        value = JSONParse(&state, wrapped().responseTextIgnoringResponseType());
+        if (!value)
+            value = jsNull();
         break;
 
-    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);
-        }
-
+    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;
 }
 

trunk/Source/WebCore/bindings/js/WebCoreBuiltinNames.h

@@ -63 +63 @@
     macro(readyPromiseCapability) \
     macro(removeTrack) \
+    macro(responseCacheIsValid) \
+    macro(retrieveResponse) \
+    macro(response) \
     macro(setStatus) \
     macro(state) \
@@ -86 +89 @@
     macro(RTCIceCandidate) \
     macro(RTCSessionDescription) \
+    macro(XMLHttpRequest)
 
 class WebCoreBuiltinNames {

trunk/Source/WebCore/xml/XMLHttpRequest.cpp

@@ -172 +172 @@
 }
 
-void XMLHttpRequest::didCacheResponseJSON()
-{
-    ASSERT(m_responseType == ResponseType::Json);
+void XMLHttpRequest::didCacheResponse()
+{
     ASSERT(doneWithoutErrors());
     m_responseCacheIsValid = true;
@@ -219 +218 @@
 }
 
-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();
-        }
-    }
-
-    return m_responseBlob.get();
-}
-
-ArrayBuffer* XMLHttpRequest::responseArrayBuffer()
+    if (!m_binaryResponseBuilder)
+        return Blob::create();
+
+    // 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);
+}
+
+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;
 }
 
@@ -820 +807 @@
     m_createdDocument = false;
     m_responseDocument = nullptr;
-    m_responseBlob = nullptr;
     m_binaryResponseBuilder = nullptr;
-    m_responseArrayBuffer = nullptr;
     m_responseCacheIsValid = false;
 }

trunk/Source/WebCore/xml/XMLHttpRequest.h

@@ -89 +89 @@
     String responseTextIgnoringResponseType() const { return m_responseBuilder.toStringPreserveCapacity(); }
     String responseMIMEType() const;
+
+    Document* optionalResponseXML() const { return m_responseDocument.get(); }
     Document* responseXML(ExceptionCode&);
-    Document* optionalResponseXML() const { return m_responseDocument.get(); }
-    Blob* responseBlob();
-    Blob* optionalResponseBlob() const { return m_responseBlob.get(); }
+
+    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.
@@ -109 +112 @@
 
     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);
@@ -188 +187 @@
     bool m_async { true };
     bool m_includeCredentials { false };
-    RefPtr<Blob> m_responseBlob;
 
     RefPtr<ThreadableLoader> m_loader;
@@ -202 +200 @@
     bool m_createdDocument { false };
     RefPtr<Document> m_responseDocument;
-    
+
     RefPtr<SharedBuffer> m_binaryResponseBuilder;
-    RefPtr<JSC::ArrayBuffer> m_responseArrayBuffer;
 
     bool m_error { false };

trunk/Source/WebCore/xml/XMLHttpRequest.idl

@@ -45 +45 @@
     JSGenerateToJSObject,
     ExportMacro=WEBCORE_EXPORT,
+    PublicIdentifier,
+    PrivateIdentifier,
 ] interface XMLHttpRequest : XMLHttpRequestEventTarget {
     attribute EventHandler onreadystatechange;
@@ -79 +81 @@
 
     [SetterRaisesException] attribute XMLHttpRequestResponseType responseType;
-    [GetterRaisesException, CachedAttribute, CustomGetter] readonly attribute Object response;
+    [JSBuiltin] readonly attribute Object response;
 
     readonly attribute unsigned short status;
@@ -85 +87 @@
     readonly attribute DOMString responseURL;
 
+    [PrivateIdentifier] boolean responseCacheIsValid();
+    [PrivateIdentifier, Custom] any retrieveResponse();
+
     // Extension
     [RaisesException] void overrideMimeType(DOMString override);