Bug 226359 - POST requests should not be memory-cached
Summary: POST requests should not be memory-cached
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ben Nham
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-05-27 15:32 PDT by Ben Nham
Modified: 2021-06-10 12:03 PDT (History)
10 users (show)

See Also:


Attachments
Patch (4.87 KB, patch)
2021-05-27 15:42 PDT, Ben Nham
no flags Details | Formatted Diff | Diff
Patch (25.08 KB, patch)
2021-06-02 23:23 PDT, Ben Nham
no flags Details | Formatted Diff | Diff
Patch (28.49 KB, patch)
2021-06-09 13:49 PDT, Ben Nham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Ben Nham 2021-05-27 15:32:53 PDT
POST requests should not be memory-cached
Comment 1 Ben Nham 2021-05-27 15:42:16 PDT
Created attachment 429942 [details]
Patch
Comment 2 Ben Nham 2021-05-27 15:43:22 PDT
<rdar://problem/78533759>
Comment 3 Geoffrey Garen 2021-05-27 16:04:43 PDT
Comment on attachment 429942 [details]
Patch

r=me
Comment 4 Alexey Proskuryakov 2021-05-27 18:44:39 PDT
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?
Comment 5 Ben Nham 2021-05-27 20:30:11 PDT
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.
Comment 6 Ben Nham 2021-05-27 20:38:11 PDT
As for OPTIONS, it is not listed as a cacheable method in RFC7231 4.2.3.
Comment 7 Ben Nham 2021-06-02 17:00:11 PDT
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.
Comment 8 Ben Nham 2021-06-02 23:23:31 PDT
Created attachment 430445 [details]
Patch
Comment 9 Antti Koivisto 2021-06-03 04:41:15 PDT
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.
Comment 10 Antti Koivisto 2021-06-03 04:42:11 PDT
Do we cache HEAD in memory cache right now?
Comment 11 Ben Nham 2021-06-03 13:29:21 PDT
(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.
Comment 12 Ben Nham 2021-06-03 14:20:44 PDT
(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.
Comment 13 Radar WebKit Bug Importer 2021-06-03 15:33:19 PDT
<rdar://problem/78840141>
Comment 14 Ben Nham 2021-06-09 13:49:16 PDT
Created attachment 431007 [details]
Patch
Comment 15 EWS 2021-06-10 12:02:58 PDT
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].