Attachment 346744 Details for Bug 188385 – [PATCH] Proposed Fix

[patch] [PATCH] Proposed Fix

sync-xhr-content.patch (text/plain), 13.13 KB, created by Joseph Pecoraro on 2018-08-07 16:54:56 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Joseph Pecoraro

Created: 2018-08-07 16:54:56 PDT

Size: 13.13 KB

patch

obsolete

>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 518303a36a3..c62fda728f5 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,15 @@
>+2018-08-07  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 NOBODY (OOPS!).
>+
>+        * 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-04  Ryosuke Niwa  <rniwa@webkit.org>
> 
>         Add CEReactions=NotNeeded for reactions only needed for customized builtins
>diff --git a/LayoutTests/http/tests/inspector/network/xhr-response-body-expected.txt b/LayoutTests/http/tests/inspector/network/xhr-response-body-expected.txt
>index 4aeec294827..8a46b0e4841 100644
>--- a/LayoutTests/http/tests/inspector/network/xhr-response-body-expected.txt
>+++ b/LayoutTests/http/tests/inspector/network/xhr-response-body-expected.txt
>@@ -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.
>+
>diff --git a/LayoutTests/http/tests/inspector/network/xhr-response-body.html b/LayoutTests/http/tests/inspector/network/xhr-response-body.html
>index 4690f6be4d0..bb28d646ed1 100644
>--- a/LayoutTests/http/tests/inspector/network/xhr-response-body.html
>+++ b/LayoutTests/http/tests/inspector/network/xhr-response-body.html
>@@ -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.");
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 0c9ea71d811..e8ddfcf1235 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,25 @@
>+2018-08-07  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/22395159>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * 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-04  Ryosuke Niwa  <rniwa@webkit.org>
> 
>         Add CEReactions=NotNeeded for reactions only needed for customized builtins
>diff --git a/Source/WebCore/inspector/NetworkResourcesData.cpp b/Source/WebCore/inspector/NetworkResourcesData.cpp
>index b0113fa1862..b25968b2dc5 100644
>--- a/Source/WebCore/inspector/NetworkResourcesData.cpp
>+++ b/Source/WebCore/inspector/NetworkResourcesData.cpp
>@@ -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)
>diff --git a/Source/WebCore/inspector/NetworkResourcesData.h b/Source/WebCore/inspector/NetworkResourcesData.h
>index 37d8b7c01bd..782aa384ec7 100644
>--- a/Source/WebCore/inspector/NetworkResourcesData.h
>+++ b/Source/WebCore/inspector/NetworkResourcesData.h
>@@ -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);
>diff --git a/Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp b/Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp
>index b2889a36b5f..3c3dc83c4c9 100644
>--- a/Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp
>+++ b/Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp
>@@ -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.
>+        // For non-sync data would be transferred from a cached resource, but sync XHRs may not have those.
>+        if (m_loadingXHRSynchronously && resourceData && !resourceData->hasBufferedData() && !resourceData->cachedResource())
>+            m_resourcesData->setResourceContent(requestId, base64Encode(data, dataLength), true);
>     }
> 
>     m_frontendDispatcher->dataReceived(requestId, timestamp(), dataLength, encodedDataLength);

Flags:

hi: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 188385: 346744