[Webkit-unassigned] [Bug 64177] ApplicationCache update should not immediately fail when reaching per-origin quota

Fri Jul 8 13:55:00 PDT 2011

https://bugs.webkit.org/show_bug.cgi?id=64177

--- Comment #7 from Joseph Pecoraro <joepeck at webkit.org>  2011-07-08 13:55:00 PST ---
(In reply to comment #6)
> (From update of attachment 100138 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100138&action=review
> 
> Looks good for the most part.
> 
> Many of my comments are for discussion, although I expect at least some of them to lead to code changes. Not accounting for other caches with the same origin seems like a clear mistake.

You're correct. This was a mistake!

> > LayoutTests/ChangeLog:4
> > +        ApplicationCache update should not immediately fail when reaching per-origin quota
> > +        https://bugs.webkit.org/show_bug.cgi?id=64177
> 
> ChangeLog needs an overview of the new behavior. There are at least two ways to fix the bug, the other being that we wait for user decision right after exceeding the quota (in that case, the user could get multiple prompts).

Yes, I don't think multiple prompts is a nice solution. I'll include some high level
descriptions in the important ChangeLogs.

> > Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:812
> > +void ApplicationCacheGroup::didReachOriginQuota(PassRefPtr<Frame> frame, int64_t spaceNeeded)
> >  {
> >      // Inform the client the origin quota has been reached,
> >      // they may decide to increase the quota.
> > -    frame->page()->chrome()->client()->reachedApplicationCacheOriginQuota(m_origin.get());
> > +    frame->page()->chrome()->client()->reachedApplicationCacheOriginQuota(m_origin.get(), spaceNeeded);
> 
> This seems wrong. The quota is per origin, and there may be other caches using part of it. You need to add up other caches sizes when asking the client.

You're correct. The spaceNeeded was not taking the other caches into account.

> > Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:909
> > +        // If we encountered the origin quota while downloading we can request a quota
> > +        // increase now, before we attempt to store the cache.
> > +        if (m_originQuotaReachedWhileDownloading)
> 
> Why do we need to pre-compute m_originQuotaReachedWhileDownloading? It seems that with the proposed logic, we can just compare the final size to quota here.
> 
> You also have console logging when this happens, but it seems that it would be more helpful to log the full size at the end, not just tell the developer which resource brought us over the limit.

The pre-compute was to prevent running some SQL commands, but I agree
that is probably the preferred solution so I'll change this.

> > Source/WebCore/loader/appcache/ApplicationCacheGroup.h:122
> > -    void didReachOriginQuota(PassRefPtr<Frame> frame);
> > +    void didReachOriginQuota(PassRefPtr<Frame> frame, int64_t spaceNeeded);
> 
> I think that the pre-exising use of PassRefPtr is incorrect, and it would be good to fix it now. We had a long discussion about that in webkit-dev recently, and it seemed that Darin and myself had the last word :)

Sure, I'll remove the param and use m_frame, since this function uses m_origin anyways.

> > Source/WebCore/loader/appcache/ApplicationCacheGroup.h:205
> > +    bool m_originQuotaReachedWhileDownloading;
> 
> "DuringCurrentUpdate" perhaps? Or maybe the variable is not needed at all.

By just checking the quota at the end of the download I've removed this.

> > Source/WebCore/loader/appcache/ApplicationCacheGroup.h:206
> > +    bool m_originQuotaReachedPreviously;
> 
> Is this to prevent downloading and asking for quota again if the user reloads the page? I think that this idea should be considered on its own merits in a separate patch.
> 
> Any such per-process state that's not saved persistently could be confusing to users of platforms that don't expose application lifetime explicitly.

This is already the current behavior. I've just renamed the variable and moved it
to a better spot. I can rip this out, but if so I'd like to make that a separate patch,
with a test. That behavior is currently untested.

> > Source/WebKit/mac/WebView/WebUIDelegatePrivate.h:208
> > +- (void)webView:(WebView *)sender exceededApplicationCacheOriginQuotaForSecurityOrigin:(WebSecurityOrigin *)origin spaceNeeded:(NSNumber *)spaceNeeded;
> 
> I don't see any reason to use NSNumber here. It's only needed to put numbers in plists and other collections.
> 
> But I'm not absolutely sure what you want instead - likely NSUInteger, or maybe unsigned long long to match the type Foundation uses for file sizes.

That sounds good, I'll gladly switch to NSUInteger.

(In reply to comment #5)
> (From update of attachment 100138 [details])
> Attachment 100138 [details] did not pass qt-ews (qt):
> Output: http://queues.webkit.org/results/9003364

Looks like Qt has their own, separate, LayoutTestController implementation which
does not inherit from the default. I'll try to patch that up and see what the bot says!

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.