|Summary:||Prevent Redundant AppCache Downloads of Disallowed AppCache|
|Product:||WebKit||Reporter:||Joseph Pecoraro <joepeck>|
|Component:||WebCore Misc.||Assignee:||Nobody <webkit-unassigned>|
|Severity:||Normal||CC:||ap, joepeck, michaeln|
|Version:||528+ (Nightly build)|
Description Joseph Pecoraro 2011-07-22 10:57:57 PDT
This only affects platforms that have set an ApplicationCache per-origin quota. If a per-origin quota is 20MB and a site has a manifest listing 30MB of resources WebKit will perform: CHECKING -> DOWNLOADING -> ERROR on each page load. Potentially downloading resources if the WebCore memory cache doesn't have them. Ideally we will save the fact that the cache on this site has been disallowed (origin quota has not been increased enough) and break earlier if the manifest is byte for byte identical to the time it was disallowed. Resulting in: CHECKING -> ERROR, much like the CHECKING -> NOUPDATE behavior of a non-changing-manifest for a successful cache. NOTE: Because this sequence of events is not actually specified, maybe it would make sense to still trigger a "Downloading" event anyways.
Comment 1 Joseph Pecoraro 2011-07-23 16:36:11 PDT
Created attachment 101820 [details] [PATCH] Part 1: Remove the old implementation
Comment 2 Joseph Pecoraro 2011-07-23 16:37:56 PDT
Created attachment 101821 [details] [PATCH] Part 2: Implement "Disallowed Cache" This is the bulk of the work. ChangeLog explains the details.
Comment 3 Joseph Pecoraro 2011-07-23 16:39:39 PDT
Created attachment 101822 [details] [PATCH] Part 3: When Disallowed still Fallback to Newest Cache I think this is the controversial patch. ChangeLog explains it a bit.
Comment 4 Joseph Pecoraro 2011-07-23 16:41:32 PDT
Created attachment 101823 [details] [PATCH] Part 4: Move Existing Quota Tests to http/tests/appcache/quotas So other ports Skip list don't need to be adjusted every time a new "origin quota" test is added.
Comment 5 Joseph Pecoraro 2011-07-23 16:44:10 PDT
Created attachment 101824 [details] [PATCH] Part 5: Add Tests for Disallowed Quota Behavior I think this does a good job of displaying the expected behavior. Please let me know if you think there is an area where I am missing obvious test coverage.
Comment 6 Joseph Pecoraro 2011-07-23 16:47:52 PDT
These patches are very nearly complete. I hope the tests show that. • I still have some questions as FIXMEs in Part 2 • I think discussion would be required on whether or not we perform the policy in Part 3. • The tests in Part 5 should hopefully display clearly what is implemented. • I have a minor open issue about including the disallowed cache in quota calculation. I can include a follow-up patch for that, but I'd first like to have the discussion about the approach in the existing patches so I don't do unnecessary extra work if things change. Thanks!
Comment 7 Joseph Pecoraro 2011-07-23 16:52:17 PDT
Comment on attachment 101824 [details] [PATCH] Part 5: Add Tests for Disallowed Quota Behavior View in context: https://bugs.webkit.org/attachment.cgi?id=101824&action=review > LayoutTests/http/tests/appcache/quotas/origin-quota-disallowed-without-preexisting.html:64 > + // Expected to fail. Will be ~15kb out of the 20kb limit. This comment was stale. It should just be "Expected to fail" because it won't fit into the 1kb limit.
Comment 8 Michael Nordman 2011-08-02 11:40:25 PDT
Part 3 - When Disallowed still Fallback to Newest Cache 10 This handles the case where: 11 12 - user has a successfully downloads a "newest" cache 13 - server updates manifest 14 - user disallows the update (has a "disallowed" cache) 15 - user reloads the page. it is the same as the disallowed 16 cache so we issue an "error" but we fallback to the 17 old, successful cache. If an update is "disallowed", continuing to use an old version questionable. That old version may or may not be compatible with the current server-side of the application. From the apps point of view, this client is wedged at old version. It may be better to delete the old version when the user disallows an update, so the user is disallowing the manifest and not just a particular version of the manifest.
Comment 9 Joseph Pecoraro 2011-08-02 11:57:16 PDT
(In reply to comment #8) > If an update is "disallowed", continuing to use an old version questionable. That old version may or may not be compatible with the current server-side of the application. From the apps point of view, this client is wedged at old version. It may be better to delete the old version when the user disallows an update, so the user is disallowing the manifest and not just a particular version of the manifest. Correct, that is one of the reasons I mentioned it was controversial and would need discussion. I guess deleting the previous manifests would be safest.