Bug 131757 – CachedResourceLoader should check redirections to reuse or not cached resources

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

[youenn fablet]

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.

[youenn fablet]

Created attachment 229482 [details]
Patch

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[Build Bot]

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

[youenn fablet]

Created attachment 229535 [details]
Trimmed down version

[Alexey Proskuryakov]

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?

[youenn fablet]

(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.

[youenn fablet]

Created attachment 230073 [details]
Fix without storage of redirection chain within CachedResource

[youenn fablet]

Created attachment 230075 [details]
Fixed mac compilation

[youenn fablet]

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.

[Darin Adler]

Alexey, Antti, could one of you review this?

[Antti Koivisto]

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.

[youenn fablet]

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.

[youenn fablet]

Created attachment 237478 [details]
updating according review

[Darin Adler]

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.

[WebKit Commit Bot]

Comment on attachment 237478 [details]
updating according review

Clearing flags on attachment: 237478

Committed r173173: <http://trac.webkit.org/changeset/173173>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.