Bug 165230 – Web Inspector: Some resources fetched via Fetch API do not have data

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

[Joseph Pecoraro]

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?

[Radar WebKit Bug Importer]

<rdar://problem/29449220>

[youenn fablet]

> 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?

[Joseph Pecoraro]

Created attachment 296338 [details]
[PATCH] Proposed Fix

[Joseph Pecoraro]

*** Bug 165228 has been marked as a duplicate of this bug. ***

[WebKit Commit Bot]

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.

[Joseph Pecoraro]

> 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.

[youenn fablet]

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.

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

Created attachment 296477 [details]
[PATCH] Proposed Fix

I think this should better match your expectations Youenn. Let me know if it doesn't.

[WebKit Commit Bot]

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.

[Joseph Pecoraro]

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.

[Alex Christensen]

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.

[Joseph Pecoraro]

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.

[Alex Christensen]

Comment on attachment 296477 [details]
[PATCH] Proposed Fix

Ok, the rest of this code looks good to me.

[youenn fablet]

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.

[Joseph Pecoraro]

(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.

[Joseph Pecoraro]

<https://trac.webkit.org/changeset/209629>