Web Inspector: XHR content sometimes shows as error even though load succeeded
authorjoepeck@webkit.org <joepeck@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 8 Aug 2018 17:59:56 +0000 (17:59 +0000)
committerjoepeck@webkit.org <joepeck@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Wed, 8 Aug 2018 17:59:56 +0000 (17:59 +0000)
https://bugs.webkit.org/show_bug.cgi?id=188385
<rdar://problem/42646160>

Source/WebCore:

Reviewed by Devin Rousso.

* inspector/agents/InspectorNetworkAgent.cpp:
(WebCore::InspectorNetworkAgent::didReceiveData):
Avoid a double hash lookup in the common case.
Allow synchronous XHR to have text data appended in the normal case.
Allow synchronous XHR to set base64 encoded data right here for non-text data.

* inspector/NetworkResourcesData.h:
(WebCore::NetworkResourcesData::ResourceData::hasBufferedData const):
Getter to see if data is buffered or not for this resource.

* inspector/NetworkResourcesData.cpp:
(WebCore::NetworkResourcesData::maybeAddResourceData):
Return the updated ResourceData to avoid clients having to do a lookup.

LayoutTests:

Reviewed by Devin Rousso.

* http/tests/inspector/network/xhr-response-body-expected.txt:
* http/tests/inspector/network/xhr-response-body.html:
Extend this test to include synchronous XHR for text and non-text resources.

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

LayoutTests/ChangeLog
LayoutTests/http/tests/inspector/network/xhr-response-body-expected.txt
LayoutTests/http/tests/inspector/network/xhr-response-body.html
Source/WebCore/ChangeLog
Source/WebCore/inspector/NetworkResourcesData.cpp
Source/WebCore/inspector/NetworkResourcesData.h
Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp

index 3f1360d..dbdba41 100644 (file)
@@ -1,3 +1,15 @@
+2018-08-08  Joseph Pecoraro  <pecoraro@apple.com>
+
+        Web Inspector: XHR content sometimes shows as error even though load succeeded
+        https://bugs.webkit.org/show_bug.cgi?id=188385
+        <rdar://problem/42646160>
+
+        Reviewed by Devin Rousso.
+
+        * http/tests/inspector/network/xhr-response-body-expected.txt:
+        * http/tests/inspector/network/xhr-response-body.html:
+        Extend this test to include synchronous XHR for text and non-text resources.
+
 2018-08-08  Truitt Savell  <tsavell@apple.com>
 
         Adjusting test expectations for imported/blink/fast/text/international-iteration-simple-text.html
index 4aeec29..8a46b0e 100644 (file)
@@ -44,3 +44,15 @@ PASS: MIMEType should be 'image/png'.
 PASS: Content should be base64Encoded.
 PASS: Image content should be a Blob.
 
+-- Running test case: Network.getResponseBody.XHR.Sync.Text
+PASS: Resource should be XHR type.
+PASS: MIMEType should be 'text/plain'.
+PASS: Content should not be base64Encoded.
+PASS: Text content should match data.txt.
+
+-- Running test case: Network.getResponseBody.XHR.Sync.PNG
+PASS: Resource should be XHR type.
+PASS: MIMEType should be 'image/png'.
+PASS: Content should be base64Encoded.
+PASS: Image content should be a Blob.
+
index 4690f6b..dd49e83 100644 (file)
@@ -4,9 +4,9 @@
 <meta charset="utf-8">
 <script src="../resources/inspector-test.js"></script>
 <script>
-function xhrGet(url) {
+function xhrGet(url, asyncFlag = true) {
     let xhr = new XMLHttpRequest;
-    xhr.open("GET", url, true);
+    xhr.open("GET", url, asyncFlag);
     xhr.send();
 }
 
@@ -38,6 +38,14 @@ function createXHRForPNG() {
     xhrGet("/resources/square100.png");
 }
 
+function createSyncXHRForText() {
+    xhrGet("resources/data.txt?sync", false);
+}
+
+function createSyncXHRForPNG() {
+    xhrGet("/resources/square100.png?sync", false);
+}
+
 function test()
 {
     let suite = InspectorTest.createAsyncSuite("Network.getResponseBody.XHR");
@@ -74,7 +82,7 @@ function test()
     addTestCase({
         name: "Network.getResponseBody.XHR.Text",
         description: "Get text/plain content as text.",
-        expression: "createXHRForText()",
+        expression: `createXHRForText()`,
         contentHandler({error, sourceCode, content, rawBase64Encoded}) {
             InspectorTest.expectEqual(sourceCode.mimeType, "text/plain", "MIMEType should be 'text/plain'.");
             InspectorTest.expectEqual(rawBase64Encoded, false, "Content should not be base64Encoded.");
@@ -85,7 +93,7 @@ function test()
     addTestCase({
         name: "Network.getResponseBody.XHR.HTML",
         description: "Get text/html content as text.",
-        expression: "createXHRForHTML()",
+        expression: `createXHRForHTML()`,
         contentHandler({error, sourceCode, content, rawBase64Encoded}) {
             InspectorTest.expectEqual(sourceCode.mimeType, "text/html", "MIMEType should be 'text/html'.");
             InspectorTest.expectEqual(rawBase64Encoded, false, "Content should not be base64Encoded.");
@@ -96,7 +104,7 @@ function test()
     addTestCase({
         name: "Network.getResponseBody.XHR.JSON",
         description: "Get application/octet-stream content as base64Encoded text.",
-        expression: "createXHRForJSON()",
+        expression: `createXHRForJSON()`,
         contentHandler({error, sourceCode, content, rawBase64Encoded}) {
             InspectorTest.expectEqual(sourceCode.mimeType, "application/octet-stream", "MIMEType should be 'application/octet-stream'.");
             InspectorTest.expectEqual(rawBase64Encoded, true, "Content should be base64Encoded.");
@@ -109,7 +117,7 @@ function test()
     addTestCase({
         name: "Network.getResponseBody.XHR.JSON2",
         description: "Get application/json content as text.",
-        expression: "createXHRForJSON2()",
+        expression: `createXHRForJSON2()`,
         contentHandler({error, sourceCode, content, rawBase64Encoded}) {
             InspectorTest.expectEqual(sourceCode.mimeType, "application/json", "MIMEType should be 'application/json'.");
             InspectorTest.expectEqual(rawBase64Encoded, false, "Content should not be base64Encoded.");
@@ -120,7 +128,7 @@ function test()
     addTestCase({
         name: "Network.getResponseBody.XHR.JSON3",
         description: "Get arbitrary +json content as text.",
-        expression: "createXHRForJSON3()",
+        expression: `createXHRForJSON3()`,
         contentHandler({error, sourceCode, content, rawBase64Encoded}) {
             InspectorTest.expectEqual(sourceCode.mimeType, "application/vnd.api+json", "MIMEType should be 'application/vnd.api+json'.");
             InspectorTest.expectEqual(rawBase64Encoded, false, "Content should not be base64Encoded.");
@@ -131,7 +139,7 @@ function test()
     addTestCase({
         name: "Network.getResponseBody.XHR.SVG",
         description: "Get image/svg+xml content as text.",
-        expression: "createXHRForSVG()",
+        expression: `createXHRForSVG()`,
         contentHandler({error, sourceCode, content, rawBase64Encoded}) {
             InspectorTest.expectEqual(sourceCode.mimeType, "image/svg+xml", "MIMEType should be 'image/svg+xml'.");
             InspectorTest.expectEqual(rawBase64Encoded, false, "Content should not be base64Encoded.");
@@ -147,7 +155,29 @@ function test()
     addTestCase({
         name: "Network.getResponseBody.XHR.PNG",
         description: "Get image/png content.",
-        expression: "createXHRForPNG()",
+        expression: `createXHRForPNG()`,
+        contentHandler({error, sourceCode, content, rawBase64Encoded}) {
+            InspectorTest.expectEqual(sourceCode.mimeType, "image/png", "MIMEType should be 'image/png'.");
+            InspectorTest.expectEqual(rawBase64Encoded, true, "Content should be base64Encoded.");
+            InspectorTest.expectThat(content instanceof Blob, "Image content should be a Blob.");
+        }
+    });
+
+    addTestCase({
+        name: "Network.getResponseBody.XHR.Sync.Text",
+        description: "Get text/plain content as text from a synchronous XHR.",
+        expression: `createSyncXHRForText()`,
+        contentHandler({error, sourceCode, content, rawBase64Encoded}) {
+            InspectorTest.expectEqual(sourceCode.mimeType, "text/plain", "MIMEType should be 'text/plain'.");
+            InspectorTest.expectEqual(rawBase64Encoded, false, "Content should not be base64Encoded.");
+            InspectorTest.expectEqual(content, "Plain text resource.\n", "Text content should match data.txt.");
+        }
+    });
+
+    addTestCase({
+        name: "Network.getResponseBody.XHR.Sync.PNG",
+        description: "Get image/png content from a synchronous XHR.",
+        expression: `createSyncXHRForPNG()`,
         contentHandler({error, sourceCode, content, rawBase64Encoded}) {
             InspectorTest.expectEqual(sourceCode.mimeType, "image/png", "MIMEType should be 'image/png'.");
             InspectorTest.expectEqual(rawBase64Encoded, true, "Content should be base64Encoded.");
index 61c2db1..10657f3 100644 (file)
@@ -1,3 +1,25 @@
+2018-08-08  Joseph Pecoraro  <pecoraro@apple.com>
+
+        Web Inspector: XHR content sometimes shows as error even though load succeeded
+        https://bugs.webkit.org/show_bug.cgi?id=188385
+        <rdar://problem/42646160>
+
+        Reviewed by Devin Rousso.
+
+        * inspector/agents/InspectorNetworkAgent.cpp:
+        (WebCore::InspectorNetworkAgent::didReceiveData):
+        Avoid a double hash lookup in the common case.
+        Allow synchronous XHR to have text data appended in the normal case.
+        Allow synchronous XHR to set base64 encoded data right here for non-text data.
+
+        * inspector/NetworkResourcesData.h:
+        (WebCore::NetworkResourcesData::ResourceData::hasBufferedData const):
+        Getter to see if data is buffered or not for this resource.
+
+        * inspector/NetworkResourcesData.cpp:
+        (WebCore::NetworkResourcesData::maybeAddResourceData):
+        Return the updated ResourceData to avoid clients having to do a lookup.
+
 2018-08-08  Sihui Liu  <sihui_liu@apple.com>
 
         Assertion failed in Webcore::Process::setIdentifier()
index b0113fa..b25968b 100644 (file)
@@ -219,25 +219,27 @@ static bool shouldBufferResourceData(const NetworkResourcesData::ResourceData& r
     return false;
 }
 
-void NetworkResourcesData::maybeAddResourceData(const String& requestId, const char* data, size_t dataLength)
+NetworkResourcesData::ResourceData const* NetworkResourcesData::maybeAddResourceData(const String& requestId, const char* data, size_t dataLength)
 {
     ResourceData* resourceData = resourceDataForRequestId(requestId);
     if (!resourceData)
-        return;
+        return nullptr;
 
     if (!shouldBufferResourceData(*resourceData))
-        return;
+        return resourceData;
 
     if (resourceData->dataLength() + dataLength > m_maximumSingleResourceContentSize)
         m_contentSize -= resourceData->evictContent();
     if (resourceData->isContentEvicted())
-        return;
+        return resourceData;
 
     if (ensureFreeSpace(dataLength) && !resourceData->isContentEvicted()) {
         m_requestIdsDeque.append(requestId);
         resourceData->appendData(data, dataLength);
         m_contentSize += dataLength;
     }
+
+    return resourceData;
 }
 
 void NetworkResourcesData::maybeDecodeDataToContent(const String& requestId)
index 37d8b7c..782aa38 100644 (file)
@@ -90,6 +90,8 @@ public:
         bool forceBufferData() const { return m_forceBufferData; }
         void setForceBufferData(bool force) { m_forceBufferData = force; }
 
+        bool hasBufferedData() const { return m_dataBuffer; }
+
     private:
         bool hasData() const { return m_dataBuffer; }
         size_t dataLength() const;
@@ -122,7 +124,7 @@ public:
     void setResourceType(const String& requestId, InspectorPageAgent::ResourceType);
     InspectorPageAgent::ResourceType resourceType(const String& requestId);
     void setResourceContent(const String& requestId, const String& content, bool base64Encoded = false);
-    void maybeAddResourceData(const String& requestId, const char* data, size_t dataLength);
+    ResourceData const* maybeAddResourceData(const String& requestId, const char* data, size_t dataLength);
     void maybeDecodeDataToContent(const String& requestId);
     void addCachedResource(const String& requestId, CachedResource*);
     void addResourceSharedBuffer(const String& requestId, RefPtr<SharedBuffer>&&, const String& textEncodingName);
index b2889a3..3331078 100644 (file)
@@ -486,9 +486,13 @@ void InspectorNetworkAgent::didReceiveData(unsigned long identifier, const char*
     String requestId = IdentifiersFactory::requestId(identifier);
 
     if (data) {
-        NetworkResourcesData::ResourceData const* resourceData = m_resourcesData->data(requestId);
-        if (resourceData && !m_loadingXHRSynchronously)
-            m_resourcesData->maybeAddResourceData(requestId, data, dataLength);
+        NetworkResourcesData::ResourceData const* resourceData = m_resourcesData->maybeAddResourceData(requestId, data, dataLength);
+
+        // For a synchronous XHR, if we didn't add data then we can apply it here as base64 encoded content.
+        // Often the data is text and we would have a decoder, but for non-text we won't have a decoder.
+        // Sync XHRs may not have a cached resource, while non-sync XHRs usually transfer data over on completion.
+        if (m_loadingXHRSynchronously && resourceData && !resourceData->hasBufferedData() && !resourceData->cachedResource())
+            m_resourcesData->setResourceContent(requestId, base64Encode(data, dataLength), true);
     }
 
     m_frontendDispatcher->dataReceived(requestId, timestamp(), dataLength, encodedDataLength);