Bug 226359 – POST requests should not be memory-cached

• Component: New Bugs
• Status: RESOLVED FIXED

[Ben Nham]

POST requests should not be memory-cached

[Ben Nham]

Created attachment 429942 [details]
Patch

[Ben Nham]

<rdar://problem/78533759>

[Geoffrey Garen]

Comment on attachment 429942 [details]
Patch

r=me

[Alexey Proskuryakov]

Comment on attachment 429942 [details]
Patch

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

> LayoutTests/ChangeLog:3
> +        Only cache GET requests in the memory cache

But not HEAD or OPTIONS?

[Ben Nham]

We don't cache HEAD or OPTIONS in disk cache either:

```
    if (originalRequest.httpMethod() != "GET")
        return StoreDecision::NoDueToHTTPMethod;
```

I actually did write a patch to cache HEAD requests, but it didn't seem to be a win for anything, so I never ended up committing it: https://bugs.webkit.org/show_bug.cgi?id=203058.

[Ben Nham]

As for OPTIONS, it is not listed as a cacheable method in RFC7231 4.2.3.

[Ben Nham]

The reason why the ping/beacon test failures occur is because InspectorNetworkAgent::willSendRequest tries to fetch the ping/beacon CachedResource (which stores the fact that the request is a ping or beacon) from the memory cache. But as of this patch, that lookups, so the inspector temporarily marks the request as having the type "other" rather than "ping" or "beacon".

The state mismatch is only temporary because InspectorNetworkAgent::didReceiveResponse successfully fetches the CachedResource from the resource loader instead of the memory cache.

Discussing with some inspector folks on the way they want to resolve this.

[Ben Nham]

Created attachment 430445 [details]
Patch

[Antti Koivisto]

Comment on attachment 430445 [details]
Patch

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

> Source/WebCore/loader/cache/MemoryCache.cpp:119
> +    if (resource.resourceRequest().httpMethod() != "GET")
> +        return false;
> +

It might be cleaner to make CachedResource::allowsCaching return false so we never get here.

This could be achieved by adding GET check there, or maybe earlier during CachedResourceRequest setup.

[Antti Koivisto]

Do we cache HEAD in memory cache right now?

[Ben Nham]

(In reply to Antti Koivisto from comment #10)
> Do we cache HEAD in memory cache right now?

Yes, if you go to a website that makes HEAD requests (the Amazon homepage is probably the easiest one for this task), you'll see that we call MemoryCache::add with HEAD resources.

[Ben Nham]

(In reply to Antti Koivisto from comment #9)
> Comment on attachment 430445 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=430445&action=review
> 
> > Source/WebCore/loader/cache/MemoryCache.cpp:119
> > +    if (resource.resourceRequest().httpMethod() != "GET")
> > +        return false;
> > +
> 
> It might be cleaner to make CachedResource::allowsCaching return false so we
> never get here.
> 
> This could be achieved by adding GET check there, or maybe earlier during
> CachedResourceRequest setup.

I was trying to avoid putting the strcmp in CachedResource::allowsCaching() since it seemed to be called in more places than MemoryCache::add. Perhaps a premature optimization. I could also do this by changing the CachedResource constructor to set the DisallowCaching option if this isn't a GET request.

Doing the GET check in CachedResourceRequest was a bit uglier because that class has two setters (setCachingPolicy and setOptions) that allow you to mess with the caching policy after the object is created, and also CachedResource doesn't call CachedResourceRequests::allowsCaching, it just blindly copies the whole options struct.

[Radar WebKit Bug Importer]

<rdar://problem/78840141>

[Ben Nham]

Created attachment 431007 [details]
Patch

[EWS]

Committed r278717 (238685@main): <https://commits.webkit.org/238685@main>

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