Summary: | Allow no-store resources to be used for back navigation | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Joseph Pecoraro <joepeck> | ||||||||||
Component: | Page Loading | Assignee: | Nobody <webkit-unassigned> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, ap, beidson, darin, ddkilzer, eric, joepeck, koivisto, simon.fraser, skyul, webkit.review.bot | ||||||||||
Priority: | P2 | ||||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | PC | ||||||||||||
OS: | OS X 10.5 | ||||||||||||
Attachments: |
|
Description
Joseph Pecoraro
2010-11-17 10:08:44 PST
Created attachment 74132 [details]
[PATCH] Allow no-store for history
The test can be tested manually when the patch is applied. Just start the webkit-tests http server: shell> ./WebKitTools/Scripts/run-webkit-httpd Then load the test page: http://127.0.0.1:8000/cache/history-only-cached-subresource-loads.html I thought about adding a setTimeout between navigations when not in DRT, so when run manually you can see the navigations but I decided it would make the code look more complicated. When run before the patch, the first part of the test will FAIL, because the back navigation will refetch the resource. Created attachment 74133 [details]
[PATCH] Fixed Typos + Improved Comments
Does this work in Firefox? I personally think that this is a good idea, but there is some related controversy, see bug 26777. Comment on attachment 74133 [details] [PATCH] Fixed Typos + Improved Comments View in context: https://bugs.webkit.org/attachment.cgi?id=74133&action=review r=me, but you should change some of the names > WebCore/loader/cache/CachedResource.h:124 > + bool isHistoryOnly() const { return m_historyOnly; } > + void setHistoryOnly(bool b) { m_historyOnly = b; } Name these isStale() and setStale(). > WebCore/loader/cache/CachedResource.h:260 > + bool m_historyOnly : 1; Name this m_isStale. (In reply to comment #5) > (From update of attachment 74133 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=74133&action=review > > r=me, but you should change some of the names > > > WebCore/loader/cache/CachedResource.h:124 > > + bool isHistoryOnly() const { return m_historyOnly; } > > + void setHistoryOnly(bool b) { m_historyOnly = b; } > > Name these isStale() and setStale(). > > > WebCore/loader/cache/CachedResource.h:260 > > + bool m_historyOnly : 1; > > Name this m_isStale. I originally had that exact naming scheme, but decided against it. My reasoning for switching to "history only" was that I thought "stale" was too similar to isExpired(). This comes into play when a resource is received with other headers, such as "Expires", or "Cache-Control: max-age=..." and it outlives its freshness. I wanted to clearly indicate this new state of a resource, which is why I chose the longer name. With this in mind, would "stale" still be a better choice? It does match more closely to the CachePolicy name (AllowsStale). If not, should I change the "allowsStale" variable name to MemoryCache::requestResource be changed instead? (In reply to comment #6) > I originally had that exact naming scheme, but decided against it. My reasoning > for switching to "history only" was that I thought "stale" was too similar to isExpired(). > This comes into play when a resource is received with other headers, such as "Expires", > or "Cache-Control: max-age=..." and it outlives its freshness. I wanted to clearly > indicate this new state of a resource, which is why I chose the longer name. > > With this in mind, would "stale" still be a better choice? It does match more closely > to the CachePolicy name (AllowsStale). If not, should I change the "allowsStale" > variable name to MemoryCache::requestResource be changed instead? I see that you took your term from the specification text, which refers to this as a “history buffer”. Sadly, the word “history” is ambiguous, because back/forward is called history in the Window object API, but and global history is called history in browser user interface. So I think a name involving “history” is confusing. And also a bit of a layering violation because the fact that this data is used for a “history buffer” is not really something that belongs down at this cache level. If we want to use the terminology “for history only” for this, then yes I think we need to change the name of the cache policy too. Comment on attachment 74133 [details] [PATCH] Fixed Typos + Improved Comments View in context: https://bugs.webkit.org/attachment.cgi?id=74133&action=review >>> WebCore/loader/cache/CachedResource.h:124 >>> + void setHistoryOnly(bool b) { m_historyOnly = b; } >> >> Name these isStale() and setStale(). > > I originally had that exact naming scheme, but decided against it. My reasoning > for switching to "history only" was that I thought "stale" was too similar to isExpired(). > This comes into play when a resource is received with other headers, such as "Expires", > or "Cache-Control: max-age=..." and it outlives its freshness. I wanted to clearly > indicate this new state of a resource, which is why I chose the longer name. > > With this in mind, would "stale" still be a better choice? It does match more closely > to the CachePolicy name (AllowsStale). If not, should I change the "allowsStale" > variable name to MemoryCache::requestResource be changed instead? The names should probably then be isForHistoryOnly, m_isForHistoryOnly, and setForHistoryOnly. > WebCore/loader/cache/CachedResourceLoader.cpp:266 > + bool allowStaleResources = cachePolicy() == CachePolicyAllowStale; The CachePolicy value should probably be changed to something like CachePolicyHistoryBuffer or something along those lines. > WebCore/loader/cache/MemoryCache.cpp:97 > +CachedResource* MemoryCache::requestResource(CachedResourceLoader* cachedResourceLoader, CachedResource::Type type, const KURL& url, const String& charset, bool requestIsPreload, bool allowStale) This argument name should be something like forHistory. (In reply to comment #8) > • The names should probably then be isForHistoryOnly, m_isForHistoryOnly, and setForHistoryOnly. > > • The CachePolicy value should probably be changed to something like CachePolicyHistoryBuffer or something along those lines. > > • This argument name should be something like forHistory. Done, Thanks. (In reply to comment #4) > Does this work in Firefox? Good question. Yes, it looks like Firefox already does this. I manually tested Firefox/4.0b6 loading the page containing a no-store subresource, did a google search, and came back, and the page must have used the stale subresource because the random number was the same. Refreshing was different. Likewise, it does not cache the no-store for a main resource. # Manually tested the following, as they couldn't handle the window.open automation for some reason. http://127.0.0.1:8000/cache/resources/no-store-resource.html > Good question. Yes, it looks like Firefox already does this.
Actually, I take that back. Its likely that for the simple page they were using their back/forward
cache, the equivalent of our PageCache. I'm going to take a moment to more accurately check.
(In reply to comment #10) > > Good question. Yes, it looks like Firefox already does this. > > Actually, I take that back. Its likely that for the simple page they were using their back/forward > cache, the equivalent of our PageCache. I'm going to take a moment to more accurately check. Firefox does not cache "no-store" resources. What I saw before was their back/forward cache. I disabling their back/forward cache by (1) going to "about:config" and (2) setting "browser.sessionhistory.max_total_viewers" to 0. After this change, they did decide to reload the "no-store" resource. > What I saw before was their back/forward cache.
That's surprising, because it contradicts what was said in 26777. Could you compare to what Firefox 3.6 does?
And as Joe himself mentioned on IRC, this needs to be carefully considered for https resources. I don't see any practical harm from caching these for history, but perhaps others will. (In reply to comment #12) > > What I saw before was their back/forward cache. > > That's surprising, because it contradicts what was said in 26777. > Could you compare to what Firefox 3.6 does? I don't think it contradicts the comments. I think those comments were specific to https pages, or handling of "no-store" on the main resource. Here, "no-store" was for a subresource. The original documentation Firefox provided for the "bfcache" feature does not mention its handling of subresources, only the loose term "pages". https://developer.mozilla.org/En/Using_Firefox_1.5_caching For what it is worth, the manual test of Safari 5.0.2 with the PageCache enabled acts the same as Firefox 4. The main resource still uses the PageCache even with a "no-store" subresource. This should be fine, as no new requests are made, we are just jumping back to where we were. But, since (currently) we won't be using the PageCache in https sites, I think we should not allow "forHistory" resources. Created attachment 74193 [details]
[PATCH] Do No Mark "https" Resources as Allowed For History
• Addressed Darin's comments and made renames.
• Do not mark https resources for history, just remove them like we used to.
Correct me if I am wrong, but I don't need to check if the main resource is https, just
the "no-cache" subresource. I thought about this for a while, and I figured that if a
sub-resource is non-secure inside a secure main resource, then the subresource is
not particularly important.
I have "Reviewed by Darin" in the patch, but I will replace that after another review.
I just forgot to change it back to OOPS.
How about just directly testing for resource->response().cacheControlContainsNoStore()? I think that would be simpler and less confusing than yet another field in CachedResource. Response caches the cache control parse result so there is no performance penalty for calling it repeatedly. Attachment 74193 [details] did not build on win: Build output: http://queues.webkit.org/results/6142036 Comment on attachment 74193 [details] [PATCH] Do No Mark "https" Resources as Allowed For History View in context: https://bugs.webkit.org/attachment.cgi?id=74193&action=review r=me, but please fix that function to remove the unnecessary conversion to KURL, and try to find out why the Windows EWS build is failing. > WebCore/loader/cache/CachedResource.cpp:259 > + if (KURL(ParsedURLString, url()).protocolIs("https")) There is no need to parse a URL just to check its protocol. The KURL.h header has a protocolIs function that takes a string for this reason. Antti’s comment also makes sense. I missed that when reviewing. Comment on attachment 74133 [details] [PATCH] Fixed Typos + Improved Comments Cleared Darin Adler's review+ from obsolete attachment 74133 [details] so that this bug does not appear in http://webkit.org/pending-commit. Created attachment 74245 [details]
[PATCH] Patch that Will Land
Just attaching the patch I'll land.
• addressed Antti's comment, remove "isForHistory" and just use the response. cacheControlContainsNoStore
• addressed Darin's comment, use the static "protocolIs" function.
• added some comments to clarify this in MemoryCache::requestResource
• switched from remove() to evict() because all remove does is call evict =)
Rebasing and landing now.
Committed r72303 M WebCore/ChangeLog M WebCore/loader/FrameLoader.cpp M WebCore/loader/cache/MemoryCache.h M WebCore/loader/cache/CachedResourceLoader.cpp M WebCore/loader/cache/CachedResource.cpp M WebCore/loader/cache/CachePolicy.h M WebCore/loader/cache/MemoryCache.cpp A LayoutTests/http/tests/cache/history-only-cached-subresource-loads-expected.txt A LayoutTests/http/tests/cache/history-only-cached-subresource-loads.html A LayoutTests/http/tests/cache/resources/no-store-resource-forward.html A LayoutTests/http/tests/cache/resources/random.cgi A LayoutTests/http/tests/cache/resources/no-store-resource-next.html A LayoutTests/http/tests/cache/resources/no-store-resource.html M LayoutTests/ChangeLog r72303 = a40cebe316b06142cfcc85e9aa988f2364189d69 http://trac.webkit.org/changeset/72303 http://trac.webkit.org/changeset/72303 might have broken Qt Linux Release |