Bug 52044

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.

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.

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.

> 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?

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.

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?

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

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.

> There are two reasons I made that three! :)

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

Created attachment 78393 [details] patch

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

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.

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