Summary: Some resources fetched via Fetch API do not have data Steps to Reproduce: 1. fetch("data.json") 2. Select "data.json" in Resources tab => No data Notes: - We have existing InspectorInstrumentation methods to stash XHR data: InspectorInstrumentation::didReceiveXHRResponse InspectorInstrumentation::didFinishXHRLoading - We likely need to do the same for Fetch - How does EventSource fit into this as well?
<rdar://problem/29449220>
> Notes: > - We have existing InspectorInstrumentation methods to stash XHR data: > InspectorInstrumentation::didReceiveXHRResponse > InspectorInstrumentation::didFinishXHRLoading > - We likely need to do the same for Fetch It would be great if we could remove those XHR specific APIs and use generic APIs at DocumentThreadableLoader level. Inspector should be able to do specific processing (XHR, fetch) if needed based on the initiator. > - How does EventSource fit into this as well? EventSource behaves a bit differently since data is long living. That might require being able to show received data before the response is finished. Is it already supported?
Created attachment 296338 [details] [PATCH] Proposed Fix
*** Bug 165228 has been marked as a duplicate of this bug. ***
Attachment 296338 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:163: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 1 in 37 files If any of these errors are false positives, please file a bug against check-webkit-style.
> EventSource behaves a bit differently since data is long living. > That might require being able to show received data before the response is > finished. > Is it already supported? Not supported yet. We should handle this when we start better handling streaming data (Web Sockets, Fetch, Media, Event Source). Bug 164890 tracks that.
I know the current patch is following xhr/inspector current pattern. I would prefer though that we hook response inspector callback in DocumentThreadableLoader::didReceiveResponse if that is not too complex. This could be limited to fetch requests using requester information to limit risk of breaking things.
(In reply to comment #7) > I know the current patch is following xhr/inspector current pattern. > > I would prefer though that we hook response inspector callback in > DocumentThreadableLoader::didReceiveResponse if that is not too complex. Yes, I should be able to do this. Looking at it now. But I still think we would want the new Requester Type on ResourceResponseBase so that we can know at willSendRequest time the type. As I learn more about how networking works we may be able to clean this up further.
Created attachment 296477 [details] [PATCH] Proposed Fix I think this should better match your expectations Youenn. Let me know if it doesn't.
Attachment 296477 [details] did not pass style-queue: ERROR: Source/WebCore/platform/network/ResourceRequestBase.h:163: enum members should use InterCaps with an initial capital letter or initial 'k' for C-style enums. [readability/enum_casing] [4] Total errors found: 1 in 34 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 296477 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=296477&action=review > Source/WebCore/inspector/InspectorInstrumentation.h:-863 > - if (cookie.isValid()) > - didReceiveResourceResponseImpl(cookie, identifier, loader, response, resourceLoader); This check was unnecessary because it is done immediately inside didReceiveResourceResponseImpl which it calls unconditionally. Technically we can eliminate these functions but I'll do other cleanup later, I'm investigating something related.
Comment on attachment 296477 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=296477&action=review > Source/WebCore/inspector/InspectorNetworkAgent.cpp:459 > + if (initiator == cachedResourceRequestInitiators().fetch) > + m_resourcesData->setResourceType(IdentifiersFactory::requestId(identifier), InspectorPageAgent::FetchResource); > + else if (initiator == cachedResourceRequestInitiators().xmlhttprequest) Should we do anything if both of these are false? ASSERT_NOT_REACHED()? > Source/WebCore/inspector/InspectorPageAgent.cpp:326 > case CachedResource::MainResource: > return InspectorPageAgent::DocumentResource; > + case CachedResource::MediaResource: > + case CachedResource::RawResource: { > + switch (cachedResource.resourceRequest().requester()) { > + case ResourceRequest::Requester::Fetch: > + return InspectorPageAgent::FetchResource; > + case ResourceRequest::Requester::Main: > + return InspectorPageAgent::DocumentResource; > + default: > + return InspectorPageAgent::XHRResource; > + } > + } I don't understand why media resources should enter the switch statement that only returns fetch, document, or xhr. Do we consider media resources to be xhr resources? Can either of them be a main resource? We already have a case for main resources in the parent switch statement.
Comment on attachment 296477 [details] [PATCH] Proposed Fix View in context: https://bugs.webkit.org/attachment.cgi?id=296477&action=review >> Source/WebCore/inspector/InspectorNetworkAgent.cpp:459 >> + else if (initiator == cachedResourceRequestInitiators().xmlhttprequest) > > Should we do anything if both of these are false? ASSERT_NOT_REACHED()? It is fine to not assert. We are only specializing ThreadableLoader loads for loads that have specific Inspector types (xhr, and fetch). We may have others in the future (such as event source). >> Source/WebCore/inspector/InspectorPageAgent.cpp:326 >> + } > > I don't understand why media resources should enter the switch statement that only returns fetch, document, or xhr. Do we consider media resources to be xhr resources? Can either of them be a main resource? We already have a case for main resources in the parent switch statement. The Media case is not changing here. Web Inspector still does not fully support media resources. Handling of CachedResource::Media was added by someone who did not understand Inspector code at a point where CachedResource::Media was split off of CachedResource::Raw so it was just made to copy CachedResource::Raw. Fixing Media resources is a separate issue.
Comment on attachment 296477 [details] [PATCH] Proposed Fix Ok, the rest of this code looks good to me.
This looks great to me as well! Would you be able to add some tests with opaque or filtered responses? For instance, fetch no-cors and cross origin URL. And another with filtered response headers? We should expect inspector to show the whole data although the web app should not see it.
(In reply to comment #15) > This looks great to me as well! > Would you be able to add some tests with opaque or filtered responses? > For instance, fetch no-cors and cross origin URL. And another with filtered > response headers? Gladly! I'll start writing a lot more tests over on bug 165683. Which I'll start on now.
<https://trac.webkit.org/changeset/209629>