Bug 152424 – Cache redirects as separate entries

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Antti Koivisto]

We are currently caching redirect chains. This has correctness issues and can be inefficient in cases where multiple URLs redirect to the same destination.

[Antti Koivisto]

Created attachment 267630 [details]
patch

[WebKit Commit Bot]

Attachment 267630 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:346:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:423:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 19 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 267630 [details]
patch

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

r=me one these questions are addressed.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:185
> -    if (NetworkCache::singleton().isEnabled())
> +    if (canUseCache(request))

Did you change all uses of NetworkCache::singleton().isEnabled to canUseCache?  This seems like a very important change that we should've done long ago.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:-336
> -            // Make sure we don't keep a stale entry in the cache.

Why don't we need to remove any cache entries here any more?

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:402
> +#if ENABLE(NETWORK_CACHE)

You might need an #else UNUSED_PARAM(request);
And maybe it should be called something like originalRequest.

> Source/WebKit2/NetworkProcess/NetworkResourceLoader.cpp:531
> +void NetworkResourceLoader::dispatchWillSendRequestForCacheEntry(std::unique_ptr<NetworkCache::Entry> entry)

This is only called from one place and might not need its own function unless you plan to use it from other places in the future.

> LayoutTests/http/tests/cache/disk-cache/disk-cache-redirect-expected.txt:10
> +response source: Disk cache

How is the first response from the Disk cache?

> LayoutTests/http/tests/cache/disk-cache/disk-cache-redirect.html:17
> +  { responseHeaders: {'Cache-control': 'max-age=0' } },
> +  { responseHeaders: {'Cache-control': 'max-age=100' } },

If I'm correct, the first cache entry expires immediately, and the second cache entry does not expire immediately, so we test making a request for a cache entry that has expired.  Why don't we make another request after the second one to test making a request for a cache entry that has not expired?

[Antti Koivisto]

(In reply to comment #3)
> Did you change all uses of NetworkCache::singleton().isEnabled to
> canUseCache?  This seems like a very important change that we should've done
> long ago.

Yeah, everywhere in NetworkResourceLoader.

> Why don't we need to remove any cache entries here any more?

I don't think so. That case was there for expired redirect chains. Now redirects expire like other cache entries.

> This is only called from one place and might not need its own function
> unless you plan to use it from other places in the future.

It looked nicer like this in the call site.

> > LayoutTests/http/tests/cache/disk-cache/disk-cache-redirect-expected.txt:10
> > +response source: Disk cache
> 
> How is the first response from the Disk cache?

301 Permanent Redirect is cacheable by default unless other headers say otherwise. 302, 303 and 303 are not cached by default.
> 
> > LayoutTests/http/tests/cache/disk-cache/disk-cache-redirect.html:17
> > +  { responseHeaders: {'Cache-control': 'max-age=0' } },
> > +  { responseHeaders: {'Cache-control': 'max-age=100' } },
> 
> If I'm correct, the first cache entry expires immediately, and the second
> cache entry does not expire immediately, so we test making a request for a
> cache entry that has expired.  Why don't we make another request after the
> second one to test making a request for a cache entry that has not expired?

Thase are independent test cases (or actually used to generate individual test cases by generating all header permutations). Each test is loaded multiple times to see if it gets faced. You see the actual cases in the test output (the first warmup load doesn't generate output).

Thanks for review.

[Antti Koivisto]

https://trac.webkit.org/r194313