Bug 188385

Summary: Web Inspector: XHR content sometimes shows as error even though load succeeded
Product: WebKit Reporter: Joseph Pecoraro <joepeck>
Component: Web InspectorAssignee: Joseph Pecoraro <joepeck>
Status: RESOLVED FIXED    
Severity: Normal CC: hi, inspector-bugzilla-changes, joepeck, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: All   
OS: All   
Attachments:
Description Flags
[PATCH] Proposed Fix hi: review+

Joseph Pecoraro
Reported 2018-08-07 11:54:44 PDT
XHR content sometimes shows as error even though load succeeded Steps to Reproduce: 1. Inspect <https://3medu.com/> 2. Reload 3. Show XHRs in Network Tab 4. Select an XHR 5. Show Preview => Error message instead of content Compare to loading the content directly, the content is just some JSON: https://3medu.com/rs/navigator/find?code=MAP
Attachments
[PATCH] Proposed Fix (13.13 KB, patch)
2018-08-07 16:54 PDT, Joseph Pecoraro
hi: review+
Joseph Pecoraro
Comment 1 2018-08-07 11:54:57 PDT Comment hidden (obsolete)
Joseph Pecoraro
Comment 2 2018-08-07 12:00:54 PDT
So I'm seeing the load happen successfully: [Log] event: { "method": "Network.requestWillBeSent", "params": { "requestId": "0.119", "frameId": "0.2", "loaderId": "0.10", "documentURL": "https://3medu.com/", "type": "XHR", "request": { "url": "https://3medu.com/rs/navigator/find?code=NAVBAR", "method": "GET", "headers": {...} }, ... } } [Log] event: { "method": "Network.responseReceived", "params": { "requestId": "0.119", "frameId": "0.2", "loaderId": "0.10", "timestamp": 475.15031187294517, "type": "XHR", "response": { "url": "https://3medu.com/rs/navigator/find?code=NAVBAR", "status": 200, "statusText": "OK", "headers": {...}, "mimeType": "application/json", "source": "network" } } } [Log] event: { "method": "Network.loadingFinished", "params": { "requestId": "0.119", "timestamp": 475.15040089702234, "metrics": { "protocol": "http/1.1", "priority": "low", "remoteAddress": "120.55.57.48:443", "connectionIdentifier": "86A10B88-2F3E-4433-AA5E-4BC5E3316DD0", "requestHeaders": {...}, "requestHeaderBytesSent": 550, "requestBodyBytesSent": 0, "responseHeaderBytesReceived": 286, "responseBodyBytesReceived": 664, "responseBodyDecodedSize": 664 } } } But when the frontend tries to request the data for this request (0.119) we get a bad response: [Log] request: { "id": 163, "method": "Network.getResponseBody", "params": { "requestId": "0.119" } } [Log] response: { "error": { "code": -32000, "message": "No data found for resource with given identifier", "data": [{ "code": -32000, "message": "No data found for resource with given identifier" }] }, "id": 163 } So now I have to step through InspectorNetworkAgent::getResponseBody and see what's up. I'd expect there to be ResourceData that can be decoded to text. The response was `"Content-Type": "application/json"` so it should be decoded as text pretty easily.
Joseph Pecoraro
Comment 3 2018-08-07 13:53:17 PDT
There is a NetworkResourceData, but it has no content: (lldb) p *this (WebCore::NetworkResourcesData::ResourceData) $2 = { ... m_url = { length = 47, contents = 'https://3medu.com/rs/navigator/find?code=NAVBAR' } { m_impl = { m_ptr = 0x000000044c0ce168 { length = 47, is8bit = 1, contents = 'https://3medu.com/rs/navigator/find?code=NAVBAR' } } } m_content = { length = 0, contents = '' } { m_impl = { m_ptr = 0x0000000000000000 } } m_textEncodingName = { length = 0, contents = '' } { m_impl = { m_ptr = 0x0000000000000000 } } Something should have set this earlier.
Joseph Pecoraro
Comment 4 2018-08-07 14:00:26 PDT
XMLHttpRequest::didFinishLoading used to call through to InspectorInstrumentation::didFinishXHRLoading. I removed that in r225546 expecting to go down the normal InspectorNetworkAgent::didReceiveData addResourceData path. Interesting that path gets avoided if the XHR is synchronous (which may be a problem for some of the missing XHRs).
Joseph Pecoraro
Comment 5 2018-08-07 14:08:47 PDT
Heh, in this case we are loading synchronously.
Joseph Pecoraro
Comment 6 2018-08-07 14:19:12 PDT
Allowing synchronous XHR to behave the same as async here looks to be correct behavior now that `didLoadXHRSynchronously` doesn't exist to force setting the content. It also corrects the behavior of the https://3medu.com/ XHRs.
Joseph Pecoraro
Comment 7 2018-08-07 15:15:26 PDT
Joseph Pecoraro
Comment 8 2018-08-07 16:54:56 PDT
Created attachment 346744 [details] [PATCH] Proposed Fix
Devin Rousso
Comment 9 2018-08-07 23:36:43 PDT
Comment on attachment 346744 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=346744&action=review r=me Just out of curiosity, does this do anything for that longstanding bug about base64 content from XHRs not being previewable? > LayoutTests/http/tests/inspector/network/xhr-response-body.html:7 > +function xhrGet(url, asyncFlag=true) { Style: do we prefer to add spaces around the `=` for optionals? > Source/WebCore/inspector/NetworkResourcesData.h:127 > + ResourceData const* maybeAddResourceData(const String& requestId, const char* data, size_t dataLength); Style: Is this a new style "rule"? IIRC, this is the same as `const ResourceData*` since `const` applies to the left 🤔 > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:493 > + // For non-sync data would be transferred from a cached resource, but sync XHRs may not have those. This reads awkwardly. "Sync XHRs might not have a cached resource, while non-sync XHRs usually transfer the data over on completion." or something like that.
Joseph Pecoraro
Comment 10 2018-08-08 10:39:31 PDT
Comment on attachment 346744 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=346744&action=review >> Source/WebCore/inspector/NetworkResourcesData.h:127 >> + ResourceData const* maybeAddResourceData(const String& requestId, const char* data, size_t dataLength); > > Style: Is this a new style "rule"? IIRC, this is the same as `const ResourceData*` since `const` applies to the left 🤔 I was matching another function in this file. I think here the "pointer" is const, but the ResourceData is not. So the pointer is not allowed to change. Probably not necessary.
Joseph Pecoraro
Comment 11 2018-08-08 10:42:59 PDT
(In reply to Devin Rousso from comment #9) > Comment on attachment 346744 [details] > [PATCH] Proposed Fix > > Just out of curiosity, does this do anything for that longstanding bug about > base64 content from XHRs not being previewable? I think what you are describing was addressed a while ago when reworking the Network Tab. Turns out I broke Synchronous XHR for text content, and here I'm fixing Synchronous XHR for text and binary content. Namely: https://trac.webkit.org/r225546 <https://webkit.org/b/141389> Web Inspector: content views for resources loaded through XHR do not reflect declared mime-type You can probably view the history of the test file for more history: LayoutTests/http/tests/inspector/network/xhr-response-body.html
Joseph Pecoraro
Comment 12 2018-08-08 11:00:46 PDT
Note You need to log in before you can comment on or make changes to this bug.