Bug 159358 - Web Inspector: Sending XHR with UTF8 encoded data shows garbled data in Resource sidebar
Summary: Web Inspector: Sending XHR with UTF8 encoded data shows garbled data in Resou...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Inspector (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Johan K. Jensen
URL:
Keywords: InRadar
Depends on: 159382
Blocks:
  Show dependency treegraph
 
Reported: 2016-07-01 10:13 PDT by Johan K. Jensen
Modified: 2016-07-05 17:57 PDT (History)
7 users (show)

See Also:


Attachments
Patch (5.21 KB, patch)
2016-07-01 16:31 PDT, Johan K. Jensen
no flags Details | Formatted Diff | Diff
Patch (5.25 KB, patch)
2016-07-01 17:20 PDT, Johan K. Jensen
no flags Details | Formatted Diff | Diff
Patch (5.25 KB, patch)
2016-07-01 17:25 PDT, Johan K. Jensen
no flags Details | Formatted Diff | Diff
Patch (6.12 KB, patch)
2016-07-05 15:53 PDT, Johan K. Jensen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Johan K. Jensen 2016-07-01 10:13:03 PDT
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"
Comment 1 Radar WebKit Bug Importer 2016-07-01 10:13:18 PDT
<rdar://problem/27133625>
Comment 2 Joseph Pecoraro 2016-07-01 11:19:22 PDT
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=√"
        },
        ...
    }
}
Comment 3 Joseph Pecoraro 2016-07-01 11:20:24 PDT
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());
>     ...
> }
Comment 5 Johan K. Jensen 2016-07-01 16:31:23 PDT
Created attachment 282607 [details]
Patch
Comment 6 Joseph Pecoraro 2016-07-01 16:44:28 PDT
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.
Comment 7 Brian Burg 2016-07-01 16:50:52 PDT
(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."
Comment 8 Johan K. Jensen 2016-07-01 17:20:57 PDT
Created attachment 282611 [details]
Patch
Comment 9 Joseph Pecoraro 2016-07-01 17:23:12 PDT
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.
Comment 10 Johan K. Jensen 2016-07-01 17:25:28 PDT
Created attachment 282612 [details]
Patch
Comment 11 Joseph Pecoraro 2016-07-01 17:26:46 PDT
Comment on attachment 282612 [details]
Patch

r=me
Comment 12 WebKit Commit Bot 2016-07-01 17:56:11 PDT
Comment on attachment 282612 [details]
Patch

Clearing flags on attachment: 282612

Committed r202766: <http://trac.webkit.org/changeset/202766>
Comment 13 WebKit Commit Bot 2016-07-01 17:56:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 14 Alexey Proskuryakov 2016-07-01 19:14:49 PDT
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)
...
Comment 15 WebKit Commit Bot 2016-07-01 19:17:37 PDT
Re-opened since this is blocked by bug 159382
Comment 16 Johan K. Jensen 2016-07-05 15:53:08 PDT
Created attachment 282820 [details]
Patch
Comment 17 Joseph Pecoraro 2016-07-05 17:36:00 PDT
Comment on attachment 282820 [details]
Patch

r=me
Comment 18 WebKit Commit Bot 2016-07-05 17:57:24 PDT
Comment on attachment 282820 [details]
Patch

Clearing flags on attachment: 282820

Committed r202843: <http://trac.webkit.org/changeset/202843>
Comment 19 WebKit Commit Bot 2016-07-05 17:57:28 PDT
All reviewed patches have been landed.  Closing bug.