Bug 65034 – Prevent Redundant AppCache Downloads of Disallowed AppCache

• Status: NEW
• Component: WebCore Misc.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

Created attachment 101820 [details]
[PATCH] Part 1: Remove the old implementation

[Joseph Pecoraro]

Created attachment 101821 [details]
[PATCH] Part 2: Implement "Disallowed Cache"

This is the bulk of the work. ChangeLog explains the details.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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.

[Joseph Pecoraro]

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!

[Joseph Pecoraro]

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.

[Michael Nordman]

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.

[Joseph Pecoraro]

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

[Joseph Pecoraro]

Comment on attachment 101820 [details]
[PATCH] Part 1: Remove the old implementation

Clearing flags on these. We can hold off on this and, as Michael Nordman pointed out, we probably would want to implement the safest behavior.