Bug 139263 – Do not attempt to revalidate cached main resource on back/forward navigation

• Component: History
• Status: RESOLVED FIXED

[Chris Dumez]

I believe we should not attempt to revalidate cached resources on back/forward navigation. We currently do, and this is causing unnecessary network traffic.

Both RFC2616 [1] & newer RFC7234 [2] distinguish history mechanisms and caches, and state
"""
   The freshness model (Section 4.2) does not necessarily apply to
   history mechanisms.  That is, a history mechanism can display a
   previous representation even if it has expired.
"""

IE10+ and Chrome seem to behave this way already (verified with 'no-cache' page of https://www.alain.knaff.lu/bug-reports/mozillaNoStore/).

[1] http://www.w3.org/Protocols/rfc2616/rfc2616-sec13.html#sec13.13
[2] http://tools.ietf.org/html/rfc7234#section-6

[Chris Dumez]

Created attachment 242611 [details]
Patch

[Antti Koivisto]

Comment on attachment 242611 [details]
Patch

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

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:661
> +    if (request.cachePolicy() != ReturnCacheDataElseLoad && existingResource->mustRevalidateDueToCacheHeaders(cachePolicy(type))) {

The change is somewhat in a wrong place. The ResourceRequest cache policy is meant to be what is passed to the network stack (and maps directly to CFNetwork API) and shouldn't really be used as input in WebCore code.

I think the correct fix is along the lines of making CachedResourceLoader::cachePolicy() (and so FrameLoader::subresourceCachePolicy()) always return CachePolicyHistoryBuffer (which needs a better name) for back-forward navigation.

Are you changing no-store behavior? That could use a test too.

[Chris Dumez]

(In reply to comment #2)
> Comment on attachment 242611 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=242611&action=review
> 
> > Source/WebCore/loader/cache/CachedResourceLoader.cpp:661
> > +    if (request.cachePolicy() != ReturnCacheDataElseLoad && existingResource->mustRevalidateDueToCacheHeaders(cachePolicy(type))) {
> 
> The change is somewhat in a wrong place. The ResourceRequest cache policy is
> meant to be what is passed to the network stack (and maps directly to
> CFNetwork API) and shouldn't really be used as input in WebCore code.
> 
> I think the correct fix is along the lines of making
> CachedResourceLoader::cachePolicy() (and so
> FrameLoader::subresourceCachePolicy()) always return
> CachePolicyHistoryBuffer (which needs a better name) for back-forward
> navigation.
> 
> Are you changing no-store behavior? That could use a test too.

Your question seems to hint that we are caching no-store pages in the memory (or disk) cache, and using this cached content for back/forward navigation? If so, then yes, my patch is causing those cached resources to not be revalidated as well.

However, if this is the case, I am a bit worried. I don't think we should cache no-store resources at all. no-store is used by banks for sensitive pages for example and I don't think we should be caching those (even in memory). Imagine the case where you do online banking on a computer, then log out. After, someone you use this computer and navigate back in history to view your banking information.

Also note that I have verified that IE10+ / Chrome do not revalidate cached resources on back/forward navigation. However, both do not seem to cache no-store pages. Try clicking no-store link in https://www.alain.knaff.lu/bug-reports/mozillaNoStore/ with Chrome for e.g.

[Chris Dumez]

Comment on attachment 242611 [details]
Patch

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

>>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:661
>>> +    if (request.cachePolicy() != ReturnCacheDataElseLoad && existingResource->mustRevalidateDueToCacheHeaders(cachePolicy(type))) {
>> 
>> The change is somewhat in a wrong place. The ResourceRequest cache policy is meant to be what is passed to the network stack (and maps directly to CFNetwork API) and shouldn't really be used as input in WebCore code.
>> 
>> I think the correct fix is along the lines of making CachedResourceLoader::cachePolicy() (and so FrameLoader::subresourceCachePolicy()) always return CachePolicyHistoryBuffer (which needs a better name) for back-forward navigation.
>> 
>> Are you changing no-store behavior? That could use a test too.
> 
> Your question seems to hint that we are caching no-store pages in the memory (or disk) cache, and using this cached content for back/forward navigation? If so, then yes, my patch is causing those cached resources to not be revalidated as well.
> 
> However, if this is the case, I am a bit worried. I don't think we should cache no-store resources at all. no-store is used by banks for sensitive pages for example and I don't think we should be caching those (even in memory). Imagine the case where you do online banking on a computer, then log out. After, someone you use this computer and navigate back in history to view your banking information.
> 
> Also note that I have verified that IE10+ / Chrome do not revalidate cached resources on back/forward navigation. However, both do not seem to cache no-store pages. Try clicking no-store link in https://www.alain.knaff.lu/bug-reports/mozillaNoStore/ with Chrome for e.g.

Also note that FrameLoader::subresourceCachePolicy() is doing "if (request.cachePolicy() == ReturnCacheDataElseLoad)" exactly as I do. We might want to fix this also to use loadType instead, as you suggest, for consistency. Also what this means is that the current code only does revalidation on back/forward for the main resource. Apparently, we already don't revalidate sub-resources according to FrameLoader::subresourceCachePolicy().

[Chris Dumez]

Comment on attachment 242611 [details]
Patch

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

>>>> Source/WebCore/loader/cache/CachedResourceLoader.cpp:661
>>>> +    if (request.cachePolicy() != ReturnCacheDataElseLoad && existingResource->mustRevalidateDueToCacheHeaders(cachePolicy(type))) {
>>> 
>>> The change is somewhat in a wrong place. The ResourceRequest cache policy is meant to be what is passed to the network stack (and maps directly to CFNetwork API) and shouldn't really be used as input in WebCore code.
>>> 
>>> I think the correct fix is along the lines of making CachedResourceLoader::cachePolicy() (and so FrameLoader::subresourceCachePolicy()) always return CachePolicyHistoryBuffer (which needs a better name) for back-forward navigation.
>>> 
>>> Are you changing no-store behavior? That could use a test too.
>> 
>> Your question seems to hint that we are caching no-store pages in the memory (or disk) cache, and using this cached content for back/forward navigation? If so, then yes, my patch is causing those cached resources to not be revalidated as well.
>> 
>> However, if this is the case, I am a bit worried. I don't think we should cache no-store resources at all. no-store is used by banks for sensitive pages for example and I don't think we should be caching those (even in memory). Imagine the case where you do online banking on a computer, then log out. After, someone you use this computer and navigate back in history to view your banking information.
>> 
>> Also note that I have verified that IE10+ / Chrome do not revalidate cached resources on back/forward navigation. However, both do not seem to cache no-store pages. Try clicking no-store link in https://www.alain.knaff.lu/bug-reports/mozillaNoStore/ with Chrome for e.g.
> 
> Also note that FrameLoader::subresourceCachePolicy() is doing "if (request.cachePolicy() == ReturnCacheDataElseLoad)" exactly as I do. We might want to fix this also to use loadType instead, as you suggest, for consistency. Also what this means is that the current code only does revalidation on back/forward for the main resource. Apparently, we already don't revalidate sub-resources according to FrameLoader::subresourceCachePolicy().

FYI, I have just tried https://www.alain.knaff.lu/bug-reports/mozillaNoStore/no-store.cgi with my patch and we always reload the resource on back forward navigation, even with my patch. This is what I would expect since it is no-store. This is also consistent with Chrome and IE10+. I will add a layout test to cover this case though.

[Chris Dumez]

Created attachment 242648 [details]
Patch

[Chris Dumez]

Comment on attachment 242648 [details]
Patch

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

> LayoutTests/http/tests/navigation/post-frames-goback1-uncached-expected.txt:9
> +This page was requested with the HTTP method POST.

Alexey, it seems you added this test in http://trac.webkit.org/changeset/154306. As explained in the Changelog, I think this is OK but could you please confirm?

[Alexey Proskuryakov]

Doesn't this mean that we no longer use page cache there? This doesn't sound right.

[Alexey Proskuryakov]

EWS is unhappy:

Regressions: Unexpected text-only failures (3)
  http/tests/navigation/post-frames-goback1-uncached.html [ Failure ]
  http/tests/navigation/redirect-on-back-updates-history-item.html [ Failure ]
  http/tests/navigation/redirect-on-reload-updates-history-item.html [ Failure ]

[Build Bot]

Comment on attachment 242648 [details]
Patch

Attachment 242648 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5610514797297664

New failing tests:
http/tests/navigation/redirect-on-reload-updates-history-item.html
http/tests/navigation/post-frames-goback1-uncached.html
http/tests/navigation/redirect-on-back-updates-history-item.html

[Build Bot]

Created attachment 242660 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Chris Dumez]

(In reply to comment #9)
> EWS is unhappy:
> 
> Regressions: Unexpected text-only failures (3)
>   http/tests/navigation/post-frames-goback1-uncached.html [ Failure ]
>   http/tests/navigation/redirect-on-back-updates-history-item.html [ Failure
> ]
>   http/tests/navigation/redirect-on-reload-updates-history-item.html [
> Failure ]

Yes, only for WK1. I will try and figure out why the behavior differs between WK1 and WK2 here.

[Chris Dumez]

Created attachment 242669 [details]
Patch

[Antti Koivisto]

> > Are you changing no-store behavior? That could use a test too.
> 
> Your question seems to hint that we are caching no-store pages in the memory
> (or disk) cache, and using this cached content for back/forward navigation?
> If so, then yes, my patch is causing those cached resources to not be
> revalidated as well.

I wasn't really trying to hint anything, I just wasn't sure if this patch changes no-store behavior (and if we have tests for it) or not. Just looking at this code it seems it might.

I don't think it actually does though because we don't currently leave no-store resources to memory cache in the first place.

[Chris Dumez]

(In reply to comment #14)
> > > Are you changing no-store behavior? That could use a test too.
> > 
> > Your question seems to hint that we are caching no-store pages in the memory
> > (or disk) cache, and using this cached content for back/forward navigation?
> > If so, then yes, my patch is causing those cached resources to not be
> > revalidated as well.
> 
> I wasn't really trying to hint anything, I just wasn't sure if this patch
> changes no-store behavior (and if we have tests for it) or not. Just looking
> at this code it seems it might.
> 
> I don't think it actually does though because we don't currently leave
> no-store resources to memory cache in the first place.

Actually, it seems we do keep some no-store resources in memory cache. See the following layout test for e.g.:
LayoutTests/http/tests/cache/history-only-cached-subresource-loads.html

"""
This test checks that loading a subresource with "Cache-Control: no-store" is
cached and reused in back navigation when the page is not in the page cache.
"""

However, we don't load "Cache-Control: no-store" *main* resources from the cache. My patch only impacts the *main* resource loading so it doesn't change no-store behavior. In any case, I added a layout test to cover the history navigation for the no-store main resource.

[Build Bot]

Comment on attachment 242669 [details]
Patch

Attachment 242669 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/6017992840183808

New failing tests:
http/tests/navigation/redirect-on-reload-updates-history-item.html

[Build Bot]

Created attachment 242677 [details]
Archive of layout-test-results from ews100 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Chris Dumez]

Created attachment 242731 [details]
Patch

[Chris Dumez]

Created attachment 242733 [details]
Patch

[Chris Dumez]

Comment on attachment 242733 [details]
Patch

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

> LayoutTests/ChangeLog:28
> +        Use "Cache-control: no-store" header to make sure it is not cached. redirect-on-reload-updates-history-item.php already has

I am looking into this. This looks like a bug I should address first before disabling revalidation on back/forward. Considering that a new history entry is created on redirect, I don't think we should re-use the same CachedResource for the redirect.
Adding "Cache-control: no-store" header here merely works around the bug.

[Chris Dumez]

Created attachment 242829 [details]
WIP Patch

[Chris Dumez]

(In reply to comment #8)
> Doesn't this mean that we no longer use page cache there? This doesn't sound
> right.

My understanding is that page cache is disabled by default for layout tests and needs to be enabled explicitly by layout tests using:
testRunner.overridePreference("WebKitUsesPageCachePreferenceKey", 1);

The reason the test output changed is because no-store main resources are now loaded from the cache on back/forward navigation. Previously, we were only doing so for sub-resources.

[Chris Dumez]

Created attachment 244016 [details]
Patch

[Build Bot]

Comment on attachment 244016 [details]
Patch

Attachment 244016 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/5587127358193664

New failing tests:
http/tests/navigation/post-frames-goback1-uncached.html
http/tests/navigation/redirect-on-back-updates-history-item.html

[Build Bot]

Created attachment 244021 [details]
Archive of layout-test-results from ews103 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-mountainlion  Platform: Mac OS X 10.8.5

[Chris Dumez]

Created attachment 244073 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 244073 [details]
Patch

Clearing flags on attachment: 244073

Committed r178012: <http://trac.webkit.org/changeset/178012>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.