Bug 143125 – [WK2][NetworkCache] We only cache responses with status codes that are cacheable by default

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Chris Dumez]

We currently only cache responses with status codes that are cacheable by default [1]. However, RFC 7234 [2] allows us to cache responses with other status codes, as long as they have explicit headers allowing caching:
- 'Expires' header field
- 'max-age' response directive

[1] http://tools.ietf.org/html/rfc7231#page-48
[2] http://tools.ietf.org/html/rfc7234#section-4.3.2

[Radar WebKit Bug Importer]

<rdar://problem/20321172>

[Chris Dumez]

Created attachment 249550 [details]
Patch

[Chris Dumez]

Created attachment 249551 [details]
Patch

[Antti Koivisto]

Comment on attachment 249551 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:286
> +    // Check for expiration headers allowing us to cache.
> +    // http://tools.ietf.org/html/rfc7234#section-4.3.2
> +    if (std::isfinite(response.expires()) || std::isfinite(response.cacheControlMaxAge()))
> +        return StoreDecision::Yes;

http://tools.ietf.org/html/rfc7234#section-3

A cache MUST NOT store a response to any request, unless:
- the response status code is understood by the cache, and
- ...

To me this sounds like we might still want to have a list of status codes that we "understand" and not just cache everything with >0 cache lifetime specified in headers. Maybe just a list of all valid status codes.

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:294
>      switch (response.httpStatusCode()) {
> +    // These status codes are cacheable by default:
> +    // http://tools.ietf.org/html/rfc7231#page-48
>      case 200: // OK
>      case 203: // Non-Authoritative Information
>      case 204: // No Content
>      case 300: // Multiple Choices

Would be nice to factor this into a function: isResponseCacheableByDefault(). Then you don't need to write a comment explaining what these are about.

[Chris Dumez]

Created attachment 249579 [details]
Patch

[Antti Koivisto]

Comment on attachment 249579 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:345
> +    LOG(NetworkCache, "(NetworkProcess) status code %d is not cacheable by default", response.httpStatusCode());

The message is now wrong, you should drop "by default"

[Chris Dumez]

Created attachment 249580 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 249580 [details]
Patch

Clearing flags on attachment: 249580

Committed r182071: <http://trac.webkit.org/changeset/182071>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.