ApplicationCache update should not immediately fail when reaching per-origin quota

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Joseph Pecoraro]

If the user intends to cache a page (despite the default quota), then the forced manual
reload of a page is annoying and wasteful. The update should only fail at the end,
if the user disallows a storage increase.

[Joseph Pecoraro]

Created attachment 100137 [details]
[PATCH] Patch without Boilerplate

This is the patch of the test + changes, without boilerplate crowding the patch.
A patch with ChangeLogs and other boilerplate will follow.

[Joseph Pecoraro]

Created attachment 100138 [details]
[PATCH] Proposed Patch for Landing

[WebKit Review Bot]

Attachment 100138 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Source/WebCore/page/ChromeClient.h:196:  The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebCore/loader/appcache/ApplicationCacheGroup.h:122:  The parameter name "frame" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 52 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Joseph Pecoraro]

Comment on attachment 100137 [details]
[PATCH] Patch without Boilerplate

View in context: https://bugs.webkit.org/attachment.cgi?id=100137&action=review

> LayoutTests/http/tests/appcache/resources/quota-origin-continued-download.manifest:5
> +# will fire at the end, allowing an increase, and then a successful
> +# cache with the new quota.

I will change this to: "... and then successfully cache with the new quota."

[Early Warning System Bot]

Comment on attachment 100138 [details]
[PATCH] Proposed Patch for Landing

Attachment 100138 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9003364

[Alexey Proskuryakov]

Comment on attachment 100138 [details]
[PATCH] Proposed Patch for Landing

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.

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

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

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

> 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 :)

> Source/WebCore/loader/appcache/ApplicationCacheGroup.h:205
> +    bool m_originQuotaReachedWhileDownloading;

"DuringCurrentUpdate" perhaps? Or maybe the variable is not needed at all.

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

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

[Joseph Pecoraro]

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

[Joseph Pecoraro]

Created attachment 100181 [details]
[PATCH] Address Comments

• Attempted to address Qt build issues.
• Addressed ap's Review comments:
  - fix the spaceNeeded parameter to account for other caches/manifests in the domain
    Added a test for this as well, which required changes the delegate dump'd output.
• Implemented spaceNeeded for Qt to attempt to keep their tests from failing.

[WebKit Review Bot]

Attachment 100181 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Tools/DumpRenderTree/qt/DumpRenderTreeQt.h:122:  The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/qt/Api/qwebpage.h:397:  The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Early Warning System Bot]

Comment on attachment 100181 [details]
[PATCH] Address Comments

Attachment 100181 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/9001604

[Alexey Proskuryakov]

Comment on attachment 100181 [details]
[PATCH] Address Comments

View in context: https://bugs.webkit.org/attachment.cgi?id=100181&action=review

I haven't been able to follow the new logic in ApplicationCacheGroup::checkIfLoadIsComplete(). I'd like to fully understand it, and will get back to it on Monday.

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:615
>          cacheUpdateFailedDueToOriginQuota();
> +        cacheUpdateFailed();

It's super confusing to have two "failed" calls, which do completely different things.

In fact, we should log different messages when the function is called for early abort here, and when it's called after full update.

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:937
> +                // We ran out of space for this origin. Fall down to the normal error handling
> +                // after recording this state.
>                  cacheUpdateFailedDueToOriginQuota();

We should log cache size and quota size in this case (and possibly the size of other caches for this origin).

> Source/WebCore/page/ChromeClient.h:196
> +        virtual void reachedApplicationCacheOriginQuota(SecurityOrigin*, int64_t spaceNeeded) = 0;

It's not 100% clear if spaceNeeded is total space needed, or how much more space we need in addition to what we have.

[Joseph Pecoraro]

(In reply to comment #11)
> (From update of attachment 100181 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=100181&action=review
> 
> I haven't been able to follow the new logic in ApplicationCacheGroup::checkIfLoadIsComplete(). I'd like to fully understand it, and will get back to it on Monday.
> 
> > Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:615
> >          cacheUpdateFailedDueToOriginQuota();
> > +        cacheUpdateFailed();
> 
> It's super confusing to have two "failed" calls, which do completely different things.
> 
> In fact, we should log different messages when the function is called for early abort here, and when it's called after full update.

I can remove the cacheUpdateFailedDueToOriginQuota function and
instead pull its 1 or 2 lines out to the call sites.


> > Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:937
> > +                // We ran out of space for this origin. Fall down to the normal error handling
> > +                // after recording this state.
> >                  cacheUpdateFailedDueToOriginQuota();
> 
> We should log cache size and quota size in this case (and possibly the size of other caches for this origin).

Can we leave that for a follow-up bug or would you like me to tackle that now?


> > Source/WebCore/page/ChromeClient.h:196
> > +        virtual void reachedApplicationCacheOriginQuota(SecurityOrigin*, int64_t spaceNeeded) = 0;
> 
> It's not 100% clear if spaceNeeded is total space needed, or how much more space we need in addition to what we have.

It is total space needed. I could improve the function's documentation and
rename the parameter to totalSpaceNeeded or something else.

[Joseph Pecoraro]

Created attachment 100196 [details]
[PATCH] Attempt to Fix Qt Build

[WebKit Review Bot]

Attachment 100196 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/http..." exit_code: 1

Tools/DumpRenderTree/qt/DumpRenderTreeQt.h:122:  The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]
Source/WebKit/qt/Api/qwebpage.h:397:  The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 67 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alexey Proskuryakov]

Comment on attachment 100196 [details]
[PATCH] Attempt to Fix Qt Build

View in context: https://bugs.webkit.org/attachment.cgi?id=100196&action=review

> It is total space needed. I could improve the function's documentation and
> rename the parameter to totalSpaceNeeded or something else.

Yes, please do.

Looks good to me.

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:75
> -    , m_originQuotaReached(false)
> +    , m_originQuotaReachedPreviously(false)

Maybe s/reached/exceeded/? It doesn't seem confusing in practice though.

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:604
> +    if (m_availableSpaceInQuota == ApplicationCacheStorage::unknownQuota())
> +        recalculateAvailableSpaceInQuota();

Even if it's known, it could have changed due to another appcache in the same domain. That needs a FIXME comment.

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:615
>          cacheUpdateFailedDueToOriginQuota();
> +        cacheUpdateFailed();

I'm still very unhappy to see two "cacheUpdateFailed" calls in a row.

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:803
>      // Inform the client the origin quota has been reached,
>      // they may decide to increase the quota.

This comment doesn't need to be split into two short lines.

Perhaps it's worth mentioning that we expect quota to be increased synchronously while waiting for the call to return. At least, I was confused about that despite having learned that while reviewing the original quota code.

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:827
> +    if (!cacheStorage().remainingSizeForOriginExcludingCache(m_origin.get(), m_newestCache.get(), m_availableSpaceInQuota)) {
> +        // Failed to determine what is left in the quota. Fallback to allowing anything.

It's a bad coding style violation that a function called remainingSizeForOriginExcludingCache() returns a boolean, not the remaining size. This is a pre-existing issue, and not something you did in this patch.

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:913
> +        // If we encountered the origin quota while downloading we can request a quota
> +        // increase now, before we attempt to store the cache.

s/encountered/exceeded/

[Alexey Proskuryakov]

Comment on attachment 100196 [details]
[PATCH] Attempt to Fix Qt Build

View in context: https://bugs.webkit.org/attachment.cgi?id=100196&action=review

> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:940
>              if (failureReason == ApplicationCacheStorage::TotalQuotaReached && !m_calledReachedMaxAppCacheSize) {

It's a pity that logic for total quota reached is so different from origin quota (I don't really understand what happens for global quota). Perhaps that's worth a FIXME too.

[Joseph Pecoraro]

Comment on attachment 100196 [details]
[PATCH] Attempt to Fix Qt Build

View in context: https://bugs.webkit.org/attachment.cgi?id=100196&action=review

>> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:75
>> +    , m_originQuotaReachedPreviously(false)
> 
> Maybe s/reached/exceeded/? It doesn't seem confusing in practice though.

Sure, I'll make this rename. I think its clearer.

>> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:604
>> +        recalculateAvailableSpaceInQuota();
> 
> Even if it's known, it could have changed due to another appcache in the same domain. That needs a FIXME comment.

Done.

>> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:615
>> +        cacheUpdateFailed();
> 
> I'm still very unhappy to see two "cacheUpdateFailed" calls in a row.

I removed this. I took the approach I mentioned in a previous comment and pulled the
log statement out to the call sites, and set the exceeded previously variable the only place
it was needed.

>> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:803
>>      // they may decide to increase the quota.
> 
> This comment doesn't need to be split into two short lines.
> 
> Perhaps it's worth mentioning that we expect quota to be increased synchronously while waiting for the call to return. At least, I was confused about that despite having learned that while reviewing the original quota code.

Done.

>> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:827
>> +        // Failed to determine what is left in the quota. Fallback to allowing anything.
> 
> It's a bad coding style violation that a function called remainingSizeForOriginExcludingCache() returns a boolean, not the remaining size. This is a pre-existing issue, and not something you did in this patch.

But it is something I did in a previous patch =/. It uses an out parameter with a return bool
for succeeded/failed. Should I switch instead to returning a number, and having a bool
out param for success/failure (like parseDouble from String)?

>> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:913

> 
> s/encountered/exceeded/

Done.

>> Source/WebCore/loader/appcache/ApplicationCacheGroup.cpp:940
>>              if (failureReason == ApplicationCacheStorage::TotalQuotaReached && !m_calledReachedMaxAppCacheSize) {
> 
> It's a pity that logic for total quota reached is so different from origin quota (I don't really understand what happens for global quota). Perhaps that's worth a FIXME too.

Will do, I also don't know exactly what happens here. I also don't think it should return here without
reseting the state. I added:

    // FIXME: Should this be handled more like Origin Quotas? Does this fail properly?

>> Tools/DumpRenderTree/qt/DumpRenderTreeQt.h:122
>> +    void dumpApplicationCacheQuota(QWebSecurityOrigin* origin, quint64 defaultOriginQuota, quint64 spaceNeeded);
> 
> The parameter name "origin" adds no information, so it should be removed.  [readability/parameter_name] [5]

Old code, I'll leave it since the rest of this file has similar style warnings.

[Joseph Pecoraro]

Landed in r90856 <http://trac.webkit.org/changeset/90856>.
Watching the bots.

[Alexey Proskuryakov]

> But it is something I did in a previous patch =/. It uses an out parameter with a return bool
> for succeeded/failed. Should I switch instead to returning a number, and having a bool
> out param for success/failure (like parseDouble from String)?

Just renaming it from remainingSizeForOriginExcludingCache() to calculateRemainingSizeForOriginExcludingCache() would do the trick. Your suggestion is fine too.