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
Created attachment 242611 [details] Patch
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.
(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.
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().
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.
Created attachment 242648 [details] Patch
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?
Doesn't this mean that we no longer use page cache there? This doesn't sound right.
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 ]
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
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
(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.
Created attachment 242669 [details] Patch
> > 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.
(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.
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
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
Created attachment 242731 [details] Patch
Created attachment 242733 [details] Patch
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.
Created attachment 242829 [details] WIP Patch
(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.
Created attachment 244016 [details] Patch
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
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
Created attachment 244073 [details] Patch
Comment on attachment 244073 [details] Patch Clearing flags on attachment: 244073 Committed r178012: <http://trac.webkit.org/changeset/178012>
All reviewed patches have been landed. Closing bug.