Bug 215852 – Web Inspector fails to preview response from XHR requests

• Component: Web Inspector
• Status: RESOLVED FIXED

[Patrick Angle]

XHR requests fail to preview in the Network tab of the inspector when the server responds with `304 Not Modified`, even if the resource already exists in the local cache.

[Patrick Angle]

<rdar://66571823>

[Patrick Angle]

Created attachment 407304 [details]
Initial pass at tests to reproduce the issue consistantly.

[Blaze Burg]

Comment on attachment 407304 [details]
Initial pass at tests to reproduce the issue consistantly.

Clearing r? as this is a work in progress.

[Patrick Angle]

Created attachment 407506 [details]
Works around issue; Tests still fail

[Patrick Angle]

Comment on attachment 407506 [details]
Works around issue; Tests still fail

Clearing r?, still a work in progress.

[Patrick Angle]

Created attachment 407704 [details]
Patch

[Patrick Angle]

Comment on attachment 407704 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407704&action=review

> Source/JavaScriptCore/inspector/IdentifiersFactory.cpp:56
> +        return identifier.substring(2).toUInt64();

I realize now that this is a bad assumption and needs to be changed. `long` and `uint64` are not the same... This will fail on some platforms.

[Devin Rousso]

Comment on attachment 407704 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407704&action=review

Nice work!  I have some comments/concerns, but I think this is mostly good :)

> Source/WebCore/ChangeLog:10
> +        Tests are implemented as LayoutTests for this change.

NIT: we usually list the test that is changed rather than saying that there are tests
```
    Test: http/tests/inspector/network/fetch-response-body.html
```

> Source/WebCore/inspector/NetworkResourcesData.cpp:335
> +    Vector<NetworkResourcesData::ResourceData*> allResources = resources();

NIT: you can have a local variable have the same name as a member variable/method if you prefix that member variable/method with `this->`
```
    auto resources = this->resources();
```

> Source/WebCore/inspector/NetworkResourcesData.cpp:338
> +        if (!resourceDataURL.isNull() && resourceDataURL == url && resourceData->httpStatusCode() != 304 && (!mostRecentResourceData || (IdentifiersFactory::identifier(resourceData->requestId()) > IdentifiersFactory::identifier(mostRecentResourceData->requestId()))))

NIT: it shouldn't be necessary to check `!resourceDataURL.isNull()` here as you're already checking for equality with `url` and we know that `url` is NOT null

This definitely needs a comment here explaining why you're comparing the identifiers.  I'm guessing that you're assuming that since identifiers are sequential, the largest identifier must be the newest resource?  I think that's valid right now, but that seems very fragile.  Perhaps we could save a `WallTime` to `NetworkResourcesData::ResourceData` (just like `CachedResource::m_responseTimestamp`) and compare that instead?

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:544
> +        NetworkResourcesData::ResourceData const* previousResourceData = m_resourcesData->dataWithout304ForURL(response.url().string());

`auto`

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:550
> +                RefPtr<SharedBuffer> previousBuffer = previousResourceData->buffer();

`auto`

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:563
> +            m_resourcesData->setResourceContent(requestId, emptyString(), false);
> +            resourceResponse->setString("mimeType"_s, emptyString());

Why are we forcibly removing the content and MIME?  Is this needed/wanted?

> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:161
> +        if (this._resource.mimeType === "" && this._resource.statusCode === 304) {

NIT: we normally use `!this._resource.mimeType` or `!this._resource.mimeType.length` instead of directly comparing to the empty string

> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:162
> +            this._responseContentView = new WI.TextResourceContentView(this._resource);

Are we sure that every 304 response is text?

Also, how is it possible that we have a 304 response which basically means "you already have the content" and not have a MIME type?  Do we not get anything about what is trying to request this 304 response and can't at least infer the `Page.ResourceType` from that, or at the very least look at the `NetworkResourcesData::ResourceData` and know whether or not it's `base64Encoded` to show text or non-text?

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:35
> +function createFetchForPhpHttp200() {

NIT: PHP should be capitalized

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:39
> +function createFetchForPhpHttp304() {

NIT: PHP should be capitalized

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:69
> +            name, description, setup,

Style: these really should all be on separate lines :/

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:171
> +        setup(resolve) { InspectorTest.evaluateInPage(`internals.clearMemoryCache()`, resolve); },

Style: please put non-inline-arrow-function bodies on separate lines
```
    async setup() {
        return InspectorTest.evaluateInPage(`interals.clearMemoryCache()`);
    },
```

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:187
> +            InspectorTest.expectEqual(sourceCode.responseHeaders["Cache-Control"], "public, max-age=31556926", "Should see caching header `public, max-age=31556926`.");

NIT: I think just "Should see caching header." is enough of a description :)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:225
> +        setup(resolve) { InspectorTest.evaluateInPage(`internals.clearMemoryCache()`, resolve); },

Ditto (:171)

> LayoutTests/http/tests/inspector/network/resources/fetch-cachable.php:13
> +echo "Plain text resource.\n";

NIT: could we drop the `\n` so that the test doesn't need to include `\n` in the `expectEqual`?

[Patrick Angle]

(In reply to Devin Rousso from comment #8)
> Comment on attachment 407704 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=407704&action=review
> 
> > Source/WebCore/inspector/NetworkResourcesData.cpp:338
> > +        if (!resourceDataURL.isNull() && resourceDataURL == url && resourceData->httpStatusCode() != 304 && (!mostRecentResourceData || (IdentifiersFactory::identifier(resourceData->requestId()) > IdentifiersFactory::identifier(mostRecentResourceData->requestId()))))
> 
> This definitely needs a comment here explaining why you're comparing the
> identifiers.  I'm guessing that you're assuming that since identifiers are
> sequential, the largest identifier must be the newest resource?  I think
> that's valid right now, but that seems very fragile.  Perhaps we could save
> a `WallTime` to `NetworkResourcesData::ResourceData` (just like
> `CachedResource::m_responseTimestamp`) and compare that instead?

Good idea! Also saves us from the fragile conversion of `resourceId`s back into numbers.

> > Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:563
> > +            m_resourcesData->setResourceContent(requestId, emptyString(), false);
> > +            resourceResponse->setString("mimeType"_s, emptyString());
> 
> Why are we forcibly removing the content and MIME?  Is this needed/wanted?

Just double checked, the MIME type removal is not needed, it will already be empty if we end up here.

A 304 shouldn't have content, but content needs to have been set otherwise in `InspectorNetworkAgent::getResponseBody` we fall through unable to determine that the content was empty because we never assigned the "content" to the ResourceData. Normally the content would be set on the resource data inside `InspectorNetworkAgent::didReceiveData` or it would be part of the cached response (which we don't have if we haven't gotten a non-304 yet), but without a cached response we never call `InspectorNetworkAgent::didReceiveData` because we don't have a cached copy yet.

Open to suggestions on better ways to handle this.

> > Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:162
> > +            this._responseContentView = new WI.TextResourceContentView(this._resource);
> 
> Are we sure that every 304 response is text?
> 
> Also, how is it possible that we have a 304 response which basically means
> "you already have the content" and not have a MIME type?  Do we not get
> anything about what is trying to request this 304 response and can't at
> least infer the `Page.ResourceType` from that, or at the very least look at
> the `NetworkResourcesData::ResourceData` and know whether or not it's
> `base64Encoded` to show text or non-text?

An actual 304 response won't have any body content (unless we had cached content for it and are providing that and a proper MIME type). I guess really the correct thing to do would be show some sort of message to that effect (e.g. "Response had no body.") instead of an empty editor...

And lastly thank you for the style/nit notes! Much appreciated!

[Devin Rousso]

Comment on attachment 407704 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407704&action=review

>>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:563
>>> +            resourceResponse->setString("mimeType"_s, emptyString());
>> 
>> Why are we forcibly removing the content and MIME?  Is this needed/wanted?
> 
> Just double checked, the MIME type removal is not needed, it will already be empty if we end up here.
> 
> A 304 shouldn't have content, but content needs to have been set otherwise in `InspectorNetworkAgent::getResponseBody` we fall through unable to determine that the content was empty because we never assigned the "content" to the ResourceData. Normally the content would be set on the resource data inside `InspectorNetworkAgent::didReceiveData` or it would be part of the cached response (which we don't have if we haven't gotten a non-304 yet), but without a cached response we never call `InspectorNetworkAgent::didReceiveData` because we don't have a cached copy yet.
> 
> Open to suggestions on better ways to handle this.

I think it may be better to have the frontend "allow" an error returned by `Network.getResponseBody` if the related `WI.Resource` is 304.  This way, there is also some legacy support when inspecting older backends.

My reason for concern here is that now there's no way of determining whether there was actually content or not (e.g. we could have an existing `CachedResource` matching the 304 which legitimately had empty content (which is not the same as no/missing content), such as for headers or `<img>` "pings").

>>> Source/WebInspectorUI/UserInterface/Views/ResourceClusterContentView.js:162
>>> +            this._responseContentView = new WI.TextResourceContentView(this._resource);
>> 
>> Are we sure that every 304 response is text?
>> 
>> Also, how is it possible that we have a 304 response which basically means "you already have the content" and not have a MIME type?  Do we not get anything about what is trying to request this 304 response and can't at least infer the `Page.ResourceType` from that, or at the very least look at the `NetworkResourcesData::ResourceData` and know whether or not it's `base64Encoded` to show text or non-text?
> 
> An actual 304 response won't have any body content (unless we had cached content for it and are providing that and a proper MIME type). I guess really the correct thing to do would be show some sort of message to that effect (e.g. "Response had no body.") instead of an empty editor...
> 
> And lastly thank you for the style/nit notes! Much appreciated!

Yeah I think some sort of message is better, although I'm not really sure what to say because 304 implies that we have the content already, so I feel like we definitely should be able to show it 😅

Happy to help!  Really good job :)

[Blaze Burg]

Comment on attachment 407704 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407704&action=review

Great work!

> Source/WebCore/ChangeLog:5
> +

Please hard line wrap at ~120 characters.

> Source/WebCore/ChangeLog:13
> +        (WebCore::NetworkResourcesData::responseReceived): Store `mimeType` from response

Nit: end Changelog sentences with period. :)

> Source/WebCore/inspector/NetworkResourcesData.cpp:285
> +    return resourceDataWithout304ForURL(url);

Nit: I don't think the two-level approach is needed.

> Source/WebCore/inspector/NetworkResourcesData.cpp:328
> +NetworkResourcesData::ResourceData* NetworkResourcesData::resourceDataWithout304ForURL(const String& url)

I would drop the 'Without304' part of the method name. It seems unnecessary to mention for its callers.

[Joseph Pecoraro]

Comment on attachment 407704 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407704&action=review

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:185
> +        expression: "createFetchForPhpHttp200()",

Style: Generally for code strings in our tests we use template strings (e.g. `foo`) instead of double quoted strings. I like that practice.

[Patrick Angle]

Created attachment 407809 [details]
Patch

[Devin Rousso]

Comment on attachment 407809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407809&action=review

I think this is mostly good, but I have a few remaining questions (and some Style/NIT comments).

> Source/WebCore/inspector/NetworkResourcesData.h:131
> +        WallTime m_responseTimestamp;

please add `#include <wtf/WallTime.h>`

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:542
> +    // FIXME: 304 Not Modified responses for XHR/Fetch do not have all their information from the cache.

please make/link a bug for this

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:543
> +    if (isNotModified && (type == InspectorPageAgent::XHRResource || type == InspectorPageAgent::FetchResource) && (!cachedResource || !cachedResource->encodedSize())) {

I just noticed this, but why do we have the `isNotModified` check earlier in this function?  I would think that if we have a `CachedResource` from a `SubresourceLoader` it would have some content, even if 304.  Is that not correct?  I keep thinking in my mind that the content _must_ exist somewhere, as otherwise 304 responses for JavaScript/CSS files would break (unless I'm fundamentally misunderstanding 304).

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:563
> +            // This will not be set otherwise, as the cachedResource's body was not initialized.
> +            m_resourcesData->setResourceContent(requestId, emptyString(), false);

Do we still need this now that the frontend handles content-missing 304?

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:231
> +        if (parameters.sourceCode.statusCode == 304 && !parameters.sourceCode.content) {
> +            this.showGenericNoContentMessage();
> +            return;
> +        }

nice :)

> LayoutTests/ChangeLog:22
> +        * platform/mac-wk1/http/tests/inspector/network/fetch-response-body-expected.txt: Added. Test does not receive
> +                cache hit information for WK1.

rather than create a separate expected file, it'd probably be better to create a new file for your new test cases (e.g. `http/tests/inspector/network/fetch-response-body-caching.html`) and skip it on WK1

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:35
> +function createFetchForPHPHttp200() {

NIT: you can drop the "Http"

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:39
> +function createFetchForPHPHttp304() {

ditto (:35)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:43
> +function createFetchForFileHttp200() {

ditto (:35)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:47
> +function createFetchForFileHttp304() {

ditto (:35)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:171
> +        name: "Network.getResponseBody.Fetch.PhpHttp304NotCached",

ditto (:35)

NIT: should capitalize `PHP`

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:175
> +        setup(resolve) {
> +            InspectorTest.evaluateInPage(`internals.clearMemoryCache()`, resolve);
> +        },

NIT: you should be able to make this into an `async` function so that any errors in the `InspectorTest.evaluateInPage` call are properly logged (ideally this won't happen, but it can help diagnose issues on the bots if it does)
```
    async setup() {
        return InspectorTest.evaluateInPage(`internals.clearMemoryCache()`);
    },
```

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:176
> +        expression: `createFetchForPHPHttp304()`,

ditto (:35)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:178
> +            InspectorTest.expectEqual(sourceCode.mimeType, "", "MIMEType should be empty.");

NIT: we normally try to write the message as a regular english sentence, so please use "MIME type"

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:187
> +        name: "Network.getResponseBody.Fetch.PhpHttp200NotCached",

ditto (:35)
ditto (:187)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:189
> +        expression: `createFetchForPHPHttp200()`,

ditto (:35)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:192
> +            InspectorTest.expectEqual(sourceCode.mimeType, "text/plain", "MIMEType should be 'text/plain'.");

ditto (:178)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:201
> +        name: "Network.getResponseBody.Fetch.PhpHttp200Cached",

ditto (:35)
ditto (:187)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:203
> +        expression: `createFetchForPHPHttp200()`,

ditto (:35)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:205
> +            InspectorTest.expectEqual(sourceCode.mimeType, "text/plain", "MIMEType should be 'text/plain'.");

ditto (:178)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:214
> +        name: "Network.getResponseBody.Fetch.PhpHttp304Cached",

ditto (:35)
ditto (:187)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:216
> +        expression: `createFetchForPHPHttp304()`,

ditto (:35)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:218
> +            InspectorTest.expectEqual(sourceCode.mimeType, "text/plain", "MIMEType should be 'text/plain'.");

ditto (:178)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:227
> +        name: "Network.getResponseBody.Fetch.FileHttp304NotCached",

ditto (:35)
ditto (:187)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:231
> +        setup(resolve) {
> +            InspectorTest.evaluateInPage(`internals.clearMemoryCache()`, resolve);
> +        },

ditto (:173)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:232
> +        expression: `createFetchForFileHttp304()`,

ditto (:35)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:234
> +            InspectorTest.expectEqual(sourceCode.mimeType, "", "MIMEType should be empty.");

ditto (:178)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:243
> +        name: "Network.getResponseBody.Fetch.FileHttp200NotCached",

ditto (:35)
ditto (:187)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:245
> +        expression: `createFetchForFileHttp200()`,

ditto (:35)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:248
> +            InspectorTest.expectEqual(sourceCode.mimeType, "text/plain", "MIMEType should be 'text/plain'.");

ditto (:178)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:257
> +        name: "Network.getResponseBody.Fetch.FileHttp200Cached",

ditto (:35)
ditto (:187)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:259
> +        expression: `createFetchForFileHttp200()`,

ditto (:35)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:261
> +            InspectorTest.expectEqual(sourceCode.mimeType, "text/plain", "MIMEType should be 'text/plain'.");

ditto (:178)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:270
> +        name: "Network.getResponseBody.Fetch.FileHttp304Cached",

ditto (:35)
ditto (:187)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:272
> +        expression: `createFetchForFileHttp304()`,

ditto (:35)

> LayoutTests/http/tests/inspector/network/fetch-response-body.html:274
> +            InspectorTest.expectEqual(sourceCode.mimeType, "text/plain", "MIMEType should be 'text/plain'.");

ditto (:178)

[Patrick Angle]

Comment on attachment 407809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407809&action=review

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:543
>> +    if (isNotModified && (type == InspectorPageAgent::XHRResource || type == InspectorPageAgent::FetchResource) && (!cachedResource || !cachedResource->encodedSize())) {
> 
> I just noticed this, but why do we have the `isNotModified` check earlier in this function?  I would think that if we have a `CachedResource` from a `SubresourceLoader` it would have some content, even if 304.  Is that not correct?  I keep thinking in my mind that the content _must_ exist somewhere, as otherwise 304 responses for JavaScript/CSS files would break (unless I'm fundamentally misunderstanding 304).

The check earlier is related to a fix multiple async xhr requests being in flight at once, in which case

On paper if we get a 304, we should have it cached somewhere because a header we sent would have resulted in the 304. The `CachedResource` from the `SubresourceLoader` as well as from the `InspectorPageAgent::cachedResource` both report an encoded length of -1 for our requests, which I don't believe should be possible. The crux of the issue is that even if we handle caching just fine, when someone makes a fetch request with a `If-Modified-Since` request header, it is possible we didn't have it cached to begin with.

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:563
>> +            m_resourcesData->setResourceContent(requestId, emptyString(), false);
> 
> Do we still need this now that the frontend handles content-missing 304?

In the cases where our cachedResource has an encoded length of -1, the content/data property will never be set on our ResourceResponse. If we were unable to resolve the lack of cached content from our resourcesData cache, we still need to set a value for either content or data on the current resourcesData.

[Patrick Angle]

Comment on attachment 407809 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407809&action=review

>>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:543
>>> +    if (isNotModified && (type == InspectorPageAgent::XHRResource || type == InspectorPageAgent::FetchResource) && (!cachedResource || !cachedResource->encodedSize())) {
>> 
>> I just noticed this, but why do we have the `isNotModified` check earlier in this function?  I would think that if we have a `CachedResource` from a `SubresourceLoader` it would have some content, even if 304.  Is that not correct?  I keep thinking in my mind that the content _must_ exist somewhere, as otherwise 304 responses for JavaScript/CSS files would break (unless I'm fundamentally misunderstanding 304).
> 
> The check earlier is related to a fix multiple async xhr requests being in flight at once, in which case
> 
> On paper if we get a 304, we should have it cached somewhere because a header we sent would have resulted in the 304. The `CachedResource` from the `SubresourceLoader` as well as from the `InspectorPageAgent::cachedResource` both report an encoded length of -1 for our requests, which I don't believe should be possible. The crux of the issue is that even if we handle caching just fine, when someone makes a fetch request with a `If-Modified-Since` request header, it is possible we didn't have it cached to begin with.

Ignore the first sentence. It's from a long gone train of thought.

[Patrick Angle]

Created attachment 407840 [details]
Patch

[Devin Rousso]

Comment on attachment 407840 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407840&action=review

nice work!  one last question that I messaged you about offline :P

> Source/WebCore/ChangeLog:7
> +        Added workaround for 304 XHR/Fetch requests so that the mimeType and content are correctly set, otherwise the
> +        frontend does not have the information it needs to preview the response.

this should go below the `Reviewed by ...`

> Source/WebInspectorUI/ChangeLog:7
> +        Responses with no content and a status code of 304 now show a `Resource has no cached content` message instead
> +            of a generic error.

this should go below the `Reviewed by ...`

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:542
> +    // FIXME: <rdar://68241104> 304 Not Modified responses for XHR/Fetch do not have all their information from the cache.

NIT: I normally like to link to bugzilla (I create one if it doesn't exist) because then potential open source contributors can know about it and/or fix it :)

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:563
> +            // This will not be set otherwise, as the cachedResource's body was not initialized.
> +            m_resourcesData->setResourceContent(requestId, emptyString(), false);

@Patrick Angle i messaged you about this offline for a more in-depth discussion

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:167
> +        this.showMessage(WI.UIString("Resource has no cached content.", "No Cached Content @ Resource Preview", "An error message when there is no cached content for a Not Modified resource."));

NIT: we normally re-use the format string in the key before the `@`, so "Resource has no cached content. @ Resource Preview"

> LayoutTests/ChangeLog:8
> +        Added new test cases and associated resources to test handling of `304 Not Modified` responses to XHR requests.
> +        We test both XHR for a PHP page as well as for a text file, as they behaved differently (incorrectly) before
> +        this patch.

this should go below the `Reviewed by ...`

> LayoutTests/platform/mac-wk1/TestExpectations:410
> +http/tests/inspector/network/fetch-response-body-304.html [ Failure ]

we should just `[ Skip ]` the test :)

[Patrick Angle]

Created attachment 407888 [details]
Patch

[Devin Rousso]

Comment on attachment 407888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407888&action=review

r=me, I have some remaining comments and there are a few more issues with the newly added code, but they're easy enough to resolve that I don't think it necessitates another review pingpong :)

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:546
> +        auto previousResourceData = m_resourcesData->dataForURL(response.url().string());
> +        
> +        if (previousResourceData) {

NIT: you can combine these into a single like
```
    if (auto previousResourceData = m_resourcesData->dataForURL(response.url().string()))
```

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:553
> +            } else
> +                m_resourcesData->setResourceContent(requestId, emptyString(), false);

Do we need this as well?  If the `previousResourceData` doesn't have any content or buffered data, we shouldn't show the `requestId` as having any either.

> Source/WebInspectorUI/UserInterface/Models/Resource.js:1033
> +            return this.requestContentFailure.bind(this)();

`bind` isn't necessary when the function is being invoked

> Source/WebInspectorUI/UserInterface/Models/Resource.js:1039
> +            }).then(WI.SourceCode.prototype.requestContent.bind(this))

Drive-by: while you're at it I think we can change this to be `super.requestContent.bind(this)`

> Source/WebInspectorUI/UserInterface/Models/Resource.js:1046
> +    requestContentFailure(error)

since this isn't used outside this class, we should make it "private" by adding a leading `_` and moving it below after the `// Private`

> Source/WebInspectorUI/UserInterface/Models/Resource.js:1051
> +        return Promise.resolve({error: WI.UIString("An error occurred trying to load the resource."), message: this._failureReasonText}).then(this._processContent.bind(this));

Do we actually need the `_processContent` call?  I don't think it would really do anything since `parameters` doesn't have `content`/`body`/`text`/`scriptSource`/`base64Encoded`/etc..

NIT: `_processContent` is a "private" method of the parent class `WI.SourceCode`, so it's a layering violation to use it here.

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:160
> -        this.showMessage(WI.UIString("Resource has no content"));
> +        this.showMessage(WI.UIString("Resource has no content."));

ooo nice catch!

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:167
> +        this.showMessage(WI.UIString("Resource has no cached content.", "Resource has no cached content. @ Resource Preview", "An error message when there is no cached content for a Not Modified resource."));

Grammar: "error message shown when there"

you might also want to say "HTTP 304 Not Modified" so that it's explicitly clear what you're talking about :)

> Source/WebInspectorUI/UserInterface/Views/ResourceContentView.js:224
> +            if (parameters.sourceCode.statusCode == 304 && parameters.message === "Missing content of resource for given requestId")
> +                this.showNoCachedContentMessage();
> +            else

NIT: i'd make this into an `if (...) { ... return; }` instead of having the `else`

[Patrick Angle]

Comment on attachment 407888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407888&action=review

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:553
>> +                m_resourcesData->setResourceContent(requestId, emptyString(), false);
> 
> Do we need this as well?  If the `previousResourceData` doesn't have any content or buffered data, we shouldn't show the `requestId` as having any either.

Good catch; don't need this one any more either.

>> Source/WebInspectorUI/UserInterface/Models/Resource.js:1051
>> +        return Promise.resolve({error: WI.UIString("An error occurred trying to load the resource."), message: this._failureReasonText}).then(this._processContent.bind(this));
> 
> Do we actually need the `_processContent` call?  I don't think it would really do anything since `parameters` doesn't have `content`/`body`/`text`/`scriptSource`/`base64Encoded`/etc..
> 
> NIT: `_processContent` is a "private" method of the parent class `WI.SourceCode`, so it's a layering violation to use it here.

`_processContent` is taking the error and message and bundling it with other information about the response. While we don't have any content, `_processContent` is also responsible for providing the `sourceCode` property which has general information about the response. This is what lets us have access to things like the status code when we handle the error in `ResourceContentView`'s `_contentAvailable(...)`.

I see three possible solution here solutions here:
1. Move this error handling go one level deeper in `SourceCode` instead - although in SourceCode we can't set the `_failure` and `_failureReasonText` properties that are unique to `Resource`. The only issue it looks like that could cause is in Script.js `requestScriptSyntaxTree(...)` is handling uncaught exceptions, but we could just check and make sure there is no `error` in `parameters` and do the same thing there. Not sure this is a great answer; I may have missed subtle assumptions that have been made about error handling in `SourceCode` over the years.
2. Mark `_processContent` as protected (and drop the `_` from its name)? Less bad than #1.
3. In `requestContentFailure(...)` we just return a value for `sourceCode` instead of letting `_processContent` do it for us. This kind of seems like the best answer given that we won't have any content.

[Devin Rousso]

Comment on attachment 407888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407888&action=review

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:555
> +            resourceResponse->setString("mimeType"_s, previousResourceData->mimeType());

I just realized that you should use `setMimeType` for this as it has extra checks to make sure that it's not called twice

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:557
> +            resourceResponse->setInteger("status"_s, previousResourceData->httpStatusCode());

ditto (:555) with `setStatus`

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:558
> +            resourceResponse->setString("statusText"_s, previousResourceData->httpStatusText());

ditto (:555) with `setStatusText`

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:560
> +            resourceResponse->setString("source"_s, Inspector::Protocol::InspectorHelpers::getEnumConstantValue(Inspector::Protocol::Network::Response::Source::DiskCache));

ditto (:555) with `setSource`

[Devin Rousso]

Comment on attachment 407888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407888&action=review

> Source/WebInspectorUI/UserInterface/Models/Resource.js:1040
> +            .catch(this.requestContentFailure.bind(this));

Actually, this will likely have issues if `WI.Resource.Event.LoadingDidFail` because the argument in that case will be a `WI.Event`, which doesn't have a `message` property.  I don't think it'll throw any errors, but it's one of those subtle things that can cause bugs in the future.  I think a better solution to this would be to instead just have this simply be `.then(this.requestContent.bind(this)))` and avoid the `.catch` altogether.

[Patrick Angle]

Comment on attachment 407888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407888&action=review

>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:555
>> +            resourceResponse->setString("mimeType"_s, previousResourceData->mimeType());
> 
> I just realized that you should use `setMimeType` for this as it has extra checks to make sure that it's not called twice

There actually isn't a `setMimeType`, `setStatus`, `setStatusText`, or `setSource` for the resource response, the ones you see are builder methods. This pattern is borrowed from line 530 where someone needed to make sure the MIME type was set under specific conditions. What I will say is that looking at it, I should use `Inspector::Protocol::Network::Response::MimeType` and add properties in the protocol for the other strings, instead of hard-coding the strings here.

[Patrick Angle]

Comment on attachment 407888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407888&action=review

>>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:555
>>> +            resourceResponse->setString("mimeType"_s, previousResourceData->mimeType());
>> 
>> I just realized that you should use `setMimeType` for this as it has extra checks to make sure that it's not called twice
> 
> There actually isn't a `setMimeType`, `setStatus`, `setStatusText`, or `setSource` for the resource response, the ones you see are builder methods. This pattern is borrowed from line 530 where someone needed to make sure the MIME type was set under specific conditions. What I will say is that looking at it, I should use `Inspector::Protocol::Network::Response::MimeType` and add properties in the protocol for the other strings, instead of hard-coding the strings here.

It actually won't be trivial to expose a `Inspector::Protocol::Network::Response::Source` property because that name is already in use to expose the enum representing the possible values.

[Devin Rousso]

Comment on attachment 407888 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407888&action=review

>>>> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:555
>>>> +            resourceResponse->setString("mimeType"_s, previousResourceData->mimeType());
>>> 
>>> I just realized that you should use `setMimeType` for this as it has extra checks to make sure that it's not called twice
>> 
>> There actually isn't a `setMimeType`, `setStatus`, `setStatusText`, or `setSource` for the resource response, the ones you see are builder methods. This pattern is borrowed from line 530 where someone needed to make sure the MIME type was set under specific conditions. What I will say is that looking at it, I should use `Inspector::Protocol::Network::Response::MimeType` and add properties in the protocol for the other strings, instead of hard-coding the strings here.
> 
> It actually won't be trivial to expose a `Inspector::Protocol::Network::Response::Source` property because that name is already in use to expose the enum representing the possible values.

aaahhh this isn't the builder.  Yeah i guess this is ... "fine" ¯\_(ツ)_/¯

Yeah `Source` is a bad name and probably should be made more specific, like `ResponseSource`.  I wouldn't worry about creating things like `Inspector::Protocol::Network::Response::MimeType` (which really should be `MIMEType`) as it's so rare that it's probably not worth it (not to mention I personally think we never even should do this in the first place).

[Patrick Angle]

Created attachment 407915 [details]
Patch

[Devin Rousso]

Comment on attachment 407915 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=407915&action=review

r=me, nice work!

> Source/WebInspectorUI/ChangeLog:13
> +        * UserInterface/Models/Resource.js:  More consistant error handling model - previously some paths rejected on

NIT: double space

> Source/WebCore/inspector/agents/InspectorNetworkAgent.cpp:552
> +            resourceResponse->setString("mimeType", previousResourceData->mimeType());

missing `_s`

> Source/WebInspectorUI/UserInterface/Models/Resource.js:1196
> +            sourceCode: this

Style: please include a trailing comma for any multi-line objects/arrays

[Patrick Angle]

Created attachment 407936 [details]
Patch

[EWS]

[e-mail] does not have committer permissions according to https://svn.webkit.org/repository/webkit/trunk/Tools/Scripts/webkitpy/common/config/contributors.json.

Rejecting attachment 407936 [details] from commit queue.

[EWS]

Committed r266568: <https://trac.webkit.org/changeset/266568>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 407936 [details].