CachedResourceLoader should reload when any redirection used when retrieving a cached resource is expired or should not be cached. Redirections cache control directives are only checked for CachedRawResource currently. This should be extended for all CachedResource.
Created attachment 229482 [details] Patch
Comment on attachment 229482 [details] Patch Attachment 229482 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5882214839484416 New failing tests: css3/masking/mask-repeat-space-padding.html http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag.html http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-parent-same-origin-deny.html css3/background/background-repeat-space-content.html webgl/1.0.1/conformance/more/functions/texImage2DHTML.html webgl/1.0.1/conformance/more/functions/texSubImage2DHTML.html
Created attachment 229492 [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
Comment on attachment 229482 [details] Patch Attachment 229482 [details] did not pass mac-ews (mac): Output: http://webkit-queues.appspot.com/results/5349477346967552 New failing tests: css3/masking/mask-repeat-space-padding.html http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag.html http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-parent-same-origin-deny.html css3/background/background-repeat-space-content.html webgl/1.0.1/conformance/more/functions/texImage2DHTML.html webgl/1.0.1/conformance/more/functions/texSubImage2DHTML.html
Created attachment 229504 [details] Archive of layout-test-results from webkit-ews-01 for mac-mountainlion The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: webkit-ews-01 Port: mac-mountainlion Platform: Mac OS X 10.8.5
Comment on attachment 229482 [details] Patch Attachment 229482 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5115896037113856 New failing tests: css3/background/background-repeat-space-content.html webgl/1.0.1/conformance/more/functions/texSubImage2DHTML.html http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag.html http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-parent-same-origin-deny.html webgl/1.0.1/conformance/more/functions/texImage2DHTML.html
Created attachment 229508 [details] Archive of layout-test-results from webkit-ews-16 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-16 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Comment on attachment 229482 [details] Patch Attachment 229482 [details] did not pass mac-wk2-ews (mac-wk2): Output: http://webkit-queues.appspot.com/results/5538108502179840 New failing tests: css3/background/background-repeat-space-content.html webgl/1.0.1/conformance/more/functions/texSubImage2DHTML.html http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag.html http/tests/security/XFrameOptions/x-frame-options-deny-meta-tag-parent-same-origin-deny.html webgl/1.0.1/conformance/more/functions/texImage2DHTML.html
Created attachment 229511 [details] Archive of layout-test-results from webkit-ews-10 for mac-mountainlion-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: webkit-ews-10 Port: mac-mountainlion-wk2 Platform: Mac OS X 10.8.5
Created attachment 229535 [details] Trimmed down version
I'm very concerned about memory use impact of this patch, responses can be very large, especially HTTPS ones. We likely do not have tests that readily demonstrate this, but that doesn't make it less real. Do we know what underlying networking implementations like CFNetwork do?
(In reply to comment #11) > I'm very concerned about memory use impact of this patch, responses can be very large, especially HTTPS ones. We likely do not have tests that readily demonstrate this, but that doesn't make it less real. > > Do we know what underlying networking implementations like CFNetwork do? If memory is a real issue, the min of all redirection freshness could be computed and stored instead.
Created attachment 230073 [details] Fix without storage of redirection chain within CachedResource
Created attachment 230075 [details] Fixed mac compilation
New patch does not store the redirection chain anymore in CachedResource. This should resolve the potential memory issue. We lose the ability to replay other redirection checks, CORS typically. I think this is OK for CORS: a simple approach would be to mandate reload of a cached resource when loading is done using CORS and the cached resource was retrieved with at least one redirection.
Alexey, Antti, could one of you review this?
Comment on attachment 230075 [details] Fixed mac compilation View in context: https://bugs.webkit.org/attachment.cgi?id=230075&action=review Looks good, some style comments. > Source/WebCore/loader/cache/CachedResource.cpp:393 > -double CachedResource::currentAge() const > +double CachedResource::currentAge(const ResourceResponse& response, double responseTimestamp) const You should turn this into static standalone helper function as it doesn't use the object fields anymore. > Source/WebCore/loader/cache/CachedResource.cpp:405 > -double CachedResource::freshnessLifetime() const > +double CachedResource::freshnessLifetime(const ResourceResponse& response) const This too. > Source/WebCore/loader/cache/CachedResource.cpp:437 > + m_requestedFromNetworkingLayer = true; > + if (!response.isNull()) { > + if (m_redirectChainCacheStatus != NotCachedRedirection) { Please use early return: if (response.isNull()) return; if (m_redirectChainCacheStatus == NotCachedRedirection) return; ... > Source/WebCore/loader/cache/CachedResource.cpp:814 > +bool CachedResource::redirectChainAllowsReuse() const > +{ > + if (m_redirectChainCacheStatus == CachedRedirection && currentTime() > m_redirectChainEndOfValidity) > + return false; > + return m_redirectChainCacheStatus != NotCachedRedirection; switch (m_redirectChainCacheStatus) would make this nicer.
Thanks for the review, I agree with most of your suggestions. > > Source/WebCore/loader/cache/CachedResource.cpp:393 > > -double CachedResource::currentAge() const > > +double CachedResource::currentAge(const ResourceResponse& response, double responseTimestamp) const > > You should turn this into static standalone helper function as it doesn't use the object fields anymore. OK > > Source/WebCore/loader/cache/CachedResource.cpp:405 > > -double CachedResource::freshnessLifetime() const > > +double CachedResource::freshnessLifetime(const ResourceResponse& response) const > > This too. The m_type field is still being used. To minimize the changes, I plan to keep it like this. > > Source/WebCore/loader/cache/CachedResource.cpp:437 > > + m_requestedFromNetworkingLayer = true; > > + if (!response.isNull()) { > > + if (m_redirectChainCacheStatus != NotCachedRedirection) { > > Please use early return: > > if (response.isNull()) > return; > if (m_redirectChainCacheStatus == NotCachedRedirection) > return; > ... OK > > Source/WebCore/loader/cache/CachedResource.cpp:814 > > +bool CachedResource::redirectChainAllowsReuse() const > > +{ > > + if (m_redirectChainCacheStatus == CachedRedirection && currentTime() > m_redirectChainEndOfValidity) > > + return false; > > + return m_redirectChainCacheStatus != NotCachedRedirection; > > switch (m_redirectChainCacheStatus) would make this nicer. Good point, I will update it accordingly.
Created attachment 237478 [details] updating according review
Comment on attachment 237478 [details] updating according review View in context: https://bugs.webkit.org/attachment.cgi?id=237478&action=review > Source/WebCore/loader/cache/CachedResource.cpp:818 > + return true; Should ASSERT_NOT_REACHED here.
Comment on attachment 237478 [details] updating according review Clearing flags on attachment: 237478 Committed r173173: <http://trac.webkit.org/changeset/173173>
All reviewed patches have been landed. Closing bug.