Bug 159358 – Web Inspector: Sending XHR with UTF8 encoded data shows garbled data in Resource sidebar

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Inspector

[Johan K. Jensen]

Summary:
Sending an XHR with UTF8 encoded data shows garbled data in Resource sidebar of the Resource/Network tab.

Steps to reproduce:
1. Open a page and run:
    var request = new XMLHttpRequest();
    request.open("POST", "/utf8", true);
    request.setRequestHeader("Content-Type", "application/x-www-form-urlencoded");
    request.send("utf8=√");
2. Open the sidebar for the (possibly failed) request in the Network tab or Resources tab.
3. See in the "Request Data" section that "utf8" had the value "√"
 => Expected "√" or "%u221a"

[Radar WebKit Bug Importer]

<rdar://problem/27133625>

[Joseph Pecoraro]

Looks like the bad data comes from the backend:

event: {
    "method": "Network.requestWillBeSent",
    "params": {
        "type": "XHR",
        "request": {
            "url": "https://bugs.webkit.org/utf8",
            "method": "POST",
            "headers": {
                "Content-Type": "application/x-www-form-urlencoded",
                "Referer": "https://bugs.webkit.org/show_bug.cgi?id=159358",
                "Origin": "https://bugs.webkit.org",
                "User-Agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_6) AppleWebKit/602.1.37 (KHTML, like Gecko) Version/9.1.2 Safari/601.7.4"
            },
            "postData": "utf8=√"
        },
        ...
    }
}

[Joseph Pecoraro]

Which comes from: Source/WebCore/inspector/InspectorNetworkAgent.cpp

> static Ref<Inspector::Protocol::Network::Request> buildObjectForResourceRequest(const ResourceRequest& request)
> {
>     ...
>     if (request.httpBody() && !request.httpBody()->isEmpty())
>         requestObject->setPostData(request.httpBody()->flattenToString());
>     ...
> }

[Joseph Pecoraro]

Seems very similar to:
<https://chromium.googlesource.com/chromium/src/+/bcbb663864624ab38b36731eb2edc839a90f9e65%5E%21/#F2>

[Johan K. Jensen]

Created attachment 282607 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 282607 [details]
Patch

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

Fantastic patch! r=me, but please put up another addressing the comments and I can cq+ it.

> Source/WebCore/ChangeLog:8
> +        Test: http/tests/inspector/network/xhr-request-data-encoded-correctly.html

We should link to the Chromium patch, given we looked at it.

Based on Chromium patch:
<https://chromium.googlesource.com/chromium/src/+/bcbb663864624ab38b36731eb2edc839a90f9e65%5E%21/#F2>

There should be examples of other such links in the ChangeLog.

> LayoutTests/http/tests/inspector/network/xhr-request-data-encoded-correctly.html:18
> +    InspectorTest.dumpActivityToSystemConsole = true;
> +    InspectorBackend.dumpInspectorProtocolMessages = true;

Nit: Remove these lines, they are only for development. (Also, now you can use InspectorTest.debug()!)

> LayoutTests/http/tests/inspector/network/xhr-request-data-encoded-correctly.html:28
> +                InspectorTest.expectThat(resource.requestData === "utf8=ð", "Request data has expected content");

Style: We want our expectThat() and assert() sentences to end in a period.

[Blaze Burg]

(In reply to comment #6)
> Comment on attachment 282607 [details]
> > LayoutTests/http/tests/inspector/network/xhr-request-data-encoded-correctly.html:28
> > +                InspectorTest.expectThat(resource.requestData === "utf8=ð", "Request data has expected content");
> 
> Style: We want our expectThat() and assert() sentences to end in a period.

Not only that, but they should rephrase the assertion as an expectation that could pass or fail. Here, I would write "Request data should have expected content."

[Johan K. Jensen]

Created attachment 282611 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 282611 [details]
Patch

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

> LayoutTests/http/tests/inspector/network/xhr-request-data-encoded-correctly-expected.txt:7
> +PASS: Request data has expected content

This needs an update for the new phrasing.

[Johan K. Jensen]

Created attachment 282612 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 282612 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 282612 [details]
Patch

Clearing flags on attachment: 282612

Committed r202766: <http://trac.webkit.org/changeset/202766>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This new test asserts every time. EWS was going to tell you that, please wait for its results next time.

Will roll out.

https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r202768%20(6283)/http/tests/inspector/network/xhr-request-data-encoded-correctly-crash-log.txt

Thread 0 Crashed:: Dispatch queue: com.apple.main-thread
0   com.apple.JavaScriptCore      	0x00000001122f9857 WTFCrash + 39 (Assertions.cpp:317)
1   com.apple.WebCore             	0x0000000116039649 WebCore::NetworkResourcesData::ResourceData::setContent(WTF::String const&, bool) + 89 (NetworkResourcesData.cpp:63)
2   com.apple.WebCore             	0x000000011603a554 WebCore::NetworkResourcesData::setResourceContent(WTF::String const&, WTF::String const&, bool) + 260 (NetworkResourcesData.cpp:197)
3   com.apple.WebCore             	0x0000000115620fc5 WebCore::InspectorNetworkAgent::didFinishXHRLoading(WebCore::ThreadableLoaderClient*, unsigned long, WTF::String const&) + 101 (InspectorNetworkAgent.cpp:453)
4   com.apple.WebCore             	0x0000000115607a96 WebCore::InspectorInstrumentation::didFinishXHRLoadingImpl(WebCore::InstrumentingAgents&, WebCore::ThreadableLoaderClient*, unsigned long, WTF::String const&, WTF::String const&, WTF::String const&, unsigned int, unsigned int) + 150 (InspectorInstrumentation.cpp:644)
5   com.apple.WebCore             	0x0000000116d5a5b5 WebCore::InspectorInstrumentation::didFinishXHRLoading(WebCore::ScriptExecutionContext*, WebCore::ThreadableLoaderClient*, unsigned long, WTF::String const&, WTF::String const&, WTF::String const&, unsigned int, unsigned int) + 117 (InspectorInstrumentation.h:923)
6   com.apple.WebCore             	0x0000000116d59120 WebCore::XMLHttpRequest::didFinishLoading(unsigned long, double) + 368 (XMLHttpRequest.cpp:1055)
...

[WebKit Commit Bot]

Re-opened since this is blocked by bug 159382

[Johan K. Jensen]

Created attachment 282820 [details]
Patch

[Joseph Pecoraro]

Comment on attachment 282820 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 282820 [details]
Patch

Clearing flags on attachment: 282820

Committed r202843: <http://trac.webkit.org/changeset/202843>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.