Bug 52044 – REGRESSION(r74807): No-store is ignored within a document

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

[Alexey Proskuryakov]

Starting with r74807 ("Move loading related code from MemoryCache to CachedResourceLoader"), requests for the same resource URL always return cached data, even if the resource is served as no-store.

This doesn't match Firefox, and is just wrong. I expect this to break sites, even though I do not have any examples (this change isn't even in nightlies yet).

[Alexey Proskuryakov]

Created attachment 78207 [details]
proposed fix

I'm not sure if this patch is technically correct. We also had some URL checks prior to r74807, which moved them, and changed their meaning. But all HTTP regression tests pass.

Also, I don't really understand what changed in this revision to make similar code so different. It's possible that there are other regressions I don't know about. I only learned about this particular behavior change because Antti mentioned it in bug 43704 comment 23.

[Antti Koivisto]

Comment on attachment 78207 [details]
proposed fix

r74807 did not introduce that test (the hash was called m_reloadedURLs before it). This is a long standing webkit behavior. You will need to provide a much better justification for changing this.

[Antti Koivisto]

Ah, there is an actual regression with no-store (that's what you are testing, the current bug title says no-cache).

The old code would never use no-store resources from cache (except when CachePolicy was CachePolicyHistoryBuffer)  due to a check in MemoryCache::requestResource(). The correct fix is to add that case to CachedResourceLoader::determineRevalidationPolicy() just before the m_validatedURLs check.

[Alexey Proskuryakov]

> This is a long standing webkit behavior.

What exactly is the behavior? It's untested, and I can't even tell what to test for.

> The correct fix is to add that case to CachedResourceLoader::determineRevalidationPolicy() just before the m_validatedURLs check.

Would you be willing to make the fix?

[Alexey Proskuryakov]

You are right that I was confused about no-store vs. no-cache here.

However, no-cache seems to be in need of fixing, too. Firefox always revalidates these. Perhaps this should be tracked in a separate bug, but my patch takes care of both.

[Alexey Proskuryakov]

The Firefox behavior is of course to re-fetch no-store resources with unconditional requests, and to revalidate no-cache ones.

Do we want to match that exactly? I think that we do. This is the same observed behavior that's achieved with my patch. It needs more extensive tests, but what do you think about the code change?

[Antti Koivisto]

(In reply to comment #5)
> You are right that I was confused about no-store vs. no-cache here.
> 
> However, no-cache seems to be in need of fixing, too. Firefox always revalidates these. Perhaps this should be tracked in a separate bug, but my patch takes care of both.

Yeah, I can patch this for no-store. Surprised we didn't have a test for that.

Since we have managed with not revalidating no-cache resources so far I think we should stick with that behavior. If people want their loads to reach server they can use no-store. It would be good to get test cases though. Tests should also differentiate between regular page load and what happens afterwards with timer initiated loads.

[Alexey Proskuryakov]

There are two reasons why I would like to fix our no-cache behavior, too:
1) Practical Firefox compatibility (well, I don't have actual examples of broken sites, but matching Firefox is what we usually do for cache behavior).
2) I don't wan't to keep confusing authors about the meaning of no-cache. This is difficult for them, so sticking to the spec and to Firefox will be helpful, and may make a site or two more robust.
3) A reason why I want to eliminate this check altogether is that URL is never a sufficient resource identifier, so all code that's using it alone is wrong. A resource is identified by it URL plus request headers, plus it's considered unique per request if it's no-store, plus the server can always decide to unique it if it's no-cache. My change obviously doesn't get us there, but keeping this check just adds to general confusion.

[Alexey Proskuryakov]

> There are two reasons

I made that three! :)

[Alexey Proskuryakov]

> If people want their loads to reach server they can use no-store.

I don't think that's a great option. With no-store, the request would be non-conditional, which is horrible for performance. So, reason #4:

4) We should fix no-cache, because that's pretty common and desirable behavior. Always get fresh data, but don't waste bandwidth if it hasn't changed.

[Antti Koivisto]

Created attachment 78393 [details]
patch

[Antti Koivisto]

(In reply to comment #10)
> I don't think that's a great option. With no-store, the request would be non-conditional, which is horrible for performance. So, reason #4:
> 
> 4) We should fix no-cache, because that's pretty common and desirable behavior. Always get fresh data, but don't waste bandwidth if it hasn't changed.

You are probably right (though all expiration should be treated uniformly, no-cache should not be a special case) for what should happen post-load. However  we should never load a resource more than once during page loading (with this patch we may in certain cases).

[Alexey Proskuryakov]

Comment on attachment 78393 [details]
patch

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

> However  we should never load a resource more than once during page loading

Definitely agreed.

Would you be willing to file a bug about no-cache?

> Source/WebCore/ChangeLog:6
> +        REGRESSION(r74807): No-cache is ignored within a document

No-cache?

> LayoutTests/ChangeLog:10
> +        * http/tests/misc/resources/random.php: Added.

I'd have renamed this random-no-store.php. Maybe the .html file, too.

[Antti Koivisto]

http://trac.webkit.org/changeset/75384

[Antti Koivisto]

(In reply to comment #13)
> Would you be willing to file a bug about no-cache?

Bug 52153.

> 
> > Source/WebCore/ChangeLog:6
> > +        REGRESSION(r74807): No-cache is ignored within a document
> 
> No-cache?
> 
> > LayoutTests/ChangeLog:10
> > +        * http/tests/misc/resources/random.php: Added.
> 
> I'd have renamed this random-no-store.php. Maybe the .html file, too.

Fixed these.