ResourceHandle::willLoadFromCache is evil

• Component: Page Loading
• Status: RESOLVED FIXED

[Alexey Proskuryakov]

One can't be certain when using ResourceHandle::willLoadFromCache(), because the resource may be pruned as soon as this function returns. We should try to load from cache unconditionally, and retry if that fails.

<rdar://problem/12948053>

[Alexey Proskuryakov]

Created attachment 181407 [details]
proposed patch

Beyond the usual FrameLoader flakiness, I'm worried about my use of stopAllLoaders() in retryAfterFailedCacheOnlyMainResourceLoad(). It seems to work, but I don't know which method of canceling the cache-only request would be fully correct.

[Brady Eidson]

Comment on attachment 181407 [details]
proposed patch

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

A somewhat scary change.  Would like an answer to my question and assurance existing layouttests pass.

> Source/WebCore/loader/FrameLoader.cpp:1466
> +    // FIXME: If the resource is a result of form submission and is not cached, the form will be silently resubmitted.
> +    // We should ask the user for confirmation in this case.
>      request.setCachePolicy(ReturnCacheDataElseLoad);

This FIXME is concerning.  Should we even allow this to be a remote possibility?

[Alexey Proskuryakov]

> This FIXME is concerning.  Should we even allow this to be a remote possibility?

This is not a change from existing behavior. Just something that I noticed while working on this patch, and decided to make clear in code.

http tests passed for me locally (I did not run the whole suite).

[Build Bot]

Comment on attachment 181407 [details]
proposed patch

Attachment 181407 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/15706610

New failing tests:
http/tests/cache/history-only-cached-subresource-loads-max-age-https.html

[Brady Eidson]

(In reply to comment #4)
> (From update of attachment 181407 [details])
> Attachment 181407 [details] did not pass mac-ews (mac):
> Output: http://queues.webkit.org/results/15706610
> 
> New failing tests:
> http/tests/cache/history-only-cached-subresource-loads-max-age-https.html

Guess we need to figure this one out first :(

[Alexey Proskuryakov]

Created attachment 181514 [details]
proposed patch

Indeed.

I've been running tests locally with one more fix that I removed as unrelated prior to submitting the patch. Turns out it's not unrelated!

The additional fix is for non-form case FrameLoader::loadDifferentDocumentItem() - we've been explicitly special casing https subresources to not use stale data on b/f navigation. The reason this worked before was that addExtraFieldsToRequest() was resetting cache policy for b/f navigations after policy delegate.

[Alexey Proskuryakov]

Committed <http://trac.webkit.org/changeset/138962>.

[Florin Malita]

Apparently this caused https://code.google.com/p/chromium/issues/detail?id=172721

Any ideas about what might be going on?

[Alexey Proskuryakov]

Chromium used to hack this functionality in a custom way, this patch changed all ports to a cross-platform implementation. Perhaps there are some vestiges of the old hack remaining and breaking things.

[Darin Adler]

[e-mail] rolled this out in http://trac.webkit.org/changeset/141118

[Darin Adler]

Oops, my mistake, only on a Chromium branch.

[Adam Barth]

I filed bug 108214 about the regression.  Please either fix the regression or roll out your change.

[Alexey Proskuryakov]

This is not a fair request.

The chromium port had a long-standing technical debt to the WebKit project due to hacking its implementation in a way that only benefitted their platform at the expense of others. I invested the time to properly implement this in a cross-platform manner. 

This is what chromium developers should have done years ago. The fact that you chose to do an obvious hack instead does not make it others' responsibility to support it.