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"
<rdar://problem/27133625>
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=√" }, ... } }
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()); > ... > }
Seems very similar to: <https://chromium.googlesource.com/chromium/src/+/bcbb663864624ab38b36731eb2edc839a90f9e65%5E%21/#F2>
Created attachment 282607 [details] Patch
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.
(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."
Created attachment 282611 [details] Patch
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.
Created attachment 282612 [details] Patch
Comment on attachment 282612 [details] Patch r=me
Comment on attachment 282612 [details] Patch Clearing flags on attachment: 282612 Committed r202766: <http://trac.webkit.org/changeset/202766>
All reviewed patches have been landed. Closing bug.
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) ...
Re-opened since this is blocked by bug 159382
Created attachment 282820 [details] Patch
Comment on attachment 282820 [details] Patch r=me
Comment on attachment 282820 [details] Patch Clearing flags on attachment: 282820 Committed r202843: <http://trac.webkit.org/changeset/202843>