Bug 177552 – Add quota to cache API

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit Misc.

[youenn fablet]

Add quota to cache API

[youenn fablet]

Created attachment 321982 [details]
Patch

[Build Bot]

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


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:408:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 39 files


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

[youenn fablet]

Created attachment 321987 [details]
Patch

[Build Bot]

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


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:408:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 1 in 39 files


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

[Build Bot]

Comment on attachment 321987 [details]
Patch

Attachment 321987 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4679038

New failing tests:
http/wpt/cache-storage/cache-quota.https.worker.html
http/wpt/cache-storage/cache-quota.https.any.html

[Build Bot]

Created attachment 322008 [details]
Archive of layout-test-results from ews104 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews104  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[youenn fablet]

Created attachment 322025 [details]
Patch

[youenn fablet]

Created attachment 322026 [details]
Patch

[Build Bot]

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


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:392:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:419:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 36 files


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

[youenn fablet]

Created attachment 322045 [details]
Patch

[Build Bot]

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


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:392:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:419:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 36 files


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

[Alex Christensen]

Comment on attachment 322045 [details]
Patch

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

> Source/WebKit/ChangeLog:13
> +        This size is fuzzed at WebCore for opaque responses.

What does "fuzzed" mean and why are we doing it?

> Source/WebCore/platform/FileSystem.h:107
> -bool fileIsDirectory(const String&, ShouldFollowSymbolicLinks);
> +WEBCORE_EXPORT bool fileIsDirectory(const String&, ShouldFollowSymbolicLinks);

This isn't used in this patch.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:318
> +    uint64_t responseBodyFuzzedSize;
> +    if (!decoder.decode(responseBodyFuzzedSize))
> +        return std::nullopt;

std::optional<uint64_t> responseBodyFuzzedSize;
decoder >> responseBodyFuzzedSize;
if (!responseBodyFuzzedSize)
    return std::nullopt;


Also, I don't think responseBodyFuzzedSize is a good name.  I have no idea what it is used for based on the name.

> Source/WebKit/UIProcess/API/APIProcessPoolConfiguration.h:157
> +    uint64_t m_cacheStoragePerOriginQuota { 20000 * 1024 };

20 * 1024 * 1024?

> Tools/WebKitTestRunner/TestController.cpp:35
> +#include <WebCore/FileSystem.h>

This doesn't look needed.

> LayoutTests/ChangeLog:10
> +        * http/wpt/cache-storage/cache-quota.https.any.js: Added.

I don't think you actually added the test.

[youenn fablet]

> > Source/WebKit/ChangeLog:13
> > +        This size is fuzzed at WebCore for opaque responses.
> 
> What does "fuzzed" mean and why are we doing it?

I'll add a link to https://github.com/whatwg/storage/issues/31.

> > Source/WebCore/platform/FileSystem.h:107
> > -bool fileIsDirectory(const String&, ShouldFollowSymbolicLinks);
> > +WEBCORE_EXPORT bool fileIsDirectory(const String&, ShouldFollowSymbolicLinks);
> 
> This isn't used in this patch.

Right, plan is to delete the cache directory between each run for improved stability.
I splitted it from this patch to do it as a follow-up.
 
> > Source/WebKit/Shared/WebCoreArgumentCoders.cpp:318
> > +    uint64_t responseBodyFuzzedSize;
> > +    if (!decoder.decode(responseBodyFuzzedSize))
> > +        return std::nullopt;
> 
> std::optional<uint64_t> responseBodyFuzzedSize;
> decoder >> responseBodyFuzzedSize;
> if (!responseBodyFuzzedSize)
>     return std::nullopt;

It is now available for WebCoreArgumetnCoders, cool.
I would prefer though to update the whole routine at once.

> Also, I don't think responseBodyFuzzedSize is a good name.  I have no idea
> what it is used for based on the name.

We take the response body size and change it randomly when the response is opaque.
This ensures that an attacker cannot compute the body size of an opaque response through the cache API.

Maybe responseBodySize is good enough though.

> > Source/WebKit/UIProcess/API/APIProcessPoolConfiguration.h:157
> > +    uint64_t m_cacheStoragePerOriginQuota { 20000 * 1024 };
> 
> 20 * 1024 * 1024?

OK

 
> > Tools/WebKitTestRunner/TestController.cpp:35
> > +#include <WebCore/FileSystem.h>
> 
> This doesn't look needed.

Will remove it and add it back when it will be needed.
> > LayoutTests/ChangeLog:10
> > +        * http/wpt/cache-storage/cache-quota.https.any.js: Added.
> 
> I don't think you actually added the test.

I moved it to http/wpt/cache-storage/cache-quota.any.js.

[youenn fablet]

> It is now available for WebCoreArgumetnCoders, cool.
> I would prefer though to update the whole routine at once.

Of course it is...
is it available to persistent coder as well?

[Alex Christensen]

(In reply to youenn fablet from comment #14)
> > It is now available for WebCoreArgumetnCoders, cool.
> > I would prefer though to update the whole routine at once.
> 
> Of course it is...
> is it available to persistent coder as well?

not yet :(

[youenn fablet]

Created attachment 322111 [details]
Patch

[Build Bot]

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


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:392:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:419:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 38 files


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

[youenn fablet]

Created attachment 322113 [details]
Patch

[Build Bot]

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


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:392:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:419:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 38 files


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

[Build Bot]

Comment on attachment 322113 [details]
Patch

Attachment 322113 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/4690969

New failing tests:
http/wpt/cache-storage/cache-quota.any.html

[Build Bot]

Created attachment 322125 [details]
Archive of layout-test-results from ews105 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6

[Build Bot]

Comment on attachment 322113 [details]
Patch

Attachment 322113 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/4691006

New failing tests:
http/wpt/cache-storage/cache-quota.any.html

[Build Bot]

Created attachment 322128 [details]
Archive of layout-test-results from ews123 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews123  Port: ios-simulator-wk2  Platform: Mac OS X 10.12.6

[youenn fablet]

Created attachment 322189 [details]
Patch

[Build Bot]

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


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:392:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:419:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 38 files


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

[Alex Christensen]

Comment on attachment 322189 [details]
Patch

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

> Source/WebCore/Modules/cache/CacheStorageConnection.cpp:101
> +        // Padding the size as per https://github.com/whatwg/storage/issues/31.
> +        uint64_t sizeWithPadding = realSize + static_cast<uint64_t>(randomNumber() * 128000);
> +        sizeWithPadding -= (sizeWithPadding % 32000);

There's a chance sizeWithPadding will be less than realSize.  The GitHub issue says to round up, and I think we should.
We should also test that the visible size is always a multiple of 32k.

> Source/WebCore/Modules/cache/DOMCache.cpp:559
>              auto response = FetchResponse::create(*scriptExecutionContext(), std::nullopt, WTFMove(responseHeaders), WTFMove(record.response));
>              response->setBodyData(WTFMove(record.responseBody));
> +            response->setBodySizeWithPadding(record.responseBodySize);

Could we include these in the constructor so we don't accidentally forget to call additional setters every time we make a FetchResponse?

> Source/WebKit/UIProcess/API/C/WKContextConfigurationRef.h:47
>  WK_EXPORT WKStringRef WKContextConfigurationCopyCacheStorageDirectory(WKContextConfigurationRef configuration);
>  WK_EXPORT void WKContextConfigurationSetCacheStorageDirectory(WKContextConfigurationRef configuration, WKStringRef cacheStorageDirectory);
>  
> +WK_EXPORT void WKContextConfigurationSetCacheStoragePerOriginQuota(WKContextConfigurationRef configuration, uint64_t quota);

I have two problems with this:
1) There's no corresponding ObjC SPI.  Doing this makes it even harder to adopt the ObjC SPI.
2) This shouldn't be on WKContext.  It should be on WKWebsiteDataStore/WKWebsiteDataStoreRef.  These functions should be removed.  Everything else here having to do with storage is on its way to being moved there.  Putting more things here makes that transition harder.

[youenn fablet]

Created attachment 322648 [details]
Patch

[Build Bot]

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


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:392:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:419:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 34 files


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

[youenn fablet]

Thanks for the review.

(In reply to Alex Christensen from comment #26)
> Comment on attachment 322189 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322189&action=review
> 
> > Source/WebCore/Modules/cache/CacheStorageConnection.cpp:101
> > +        // Padding the size as per https://github.com/whatwg/storage/issues/31.
> > +        uint64_t sizeWithPadding = realSize + static_cast<uint64_t>(randomNumber() * 128000);
> > +        sizeWithPadding -= (sizeWithPadding % 32000);
> 
> There's a chance sizeWithPadding will be less than realSize.  The GitHub
> issue says to round up, and I think we should.
> We should also test that the visible size is always a multiple of 32k.

OK. I thought it might be good to sometimes having 0ko responses being counted as 0ko but that is probably fine as the quota size will be large enough anyway.

> > Source/WebCore/Modules/cache/DOMCache.cpp:559
> >              auto response = FetchResponse::create(*scriptExecutionContext(), std::nullopt, WTFMove(responseHeaders), WTFMove(record.response));
> >              response->setBodyData(WTFMove(record.responseBody));
> > +            response->setBodySizeWithPadding(record.responseBodySize);
> 
> Could we include these in the constructor so we don't accidentally forget to
> call additional setters every time we make a FetchResponse?

I changed setBodyData to pass the size with padding.
I don't think it makes a lot of sense to pass it in the constructor since the usual value will be zero and would probably become a default value.

> 
> > Source/WebKit/UIProcess/API/C/WKContextConfigurationRef.h:47
> >  WK_EXPORT WKStringRef WKContextConfigurationCopyCacheStorageDirectory(WKContextConfigurationRef configuration);
> >  WK_EXPORT void WKContextConfigurationSetCacheStorageDirectory(WKContextConfigurationRef configuration, WKStringRef cacheStorageDirectory);
> >  
> > +WK_EXPORT void WKContextConfigurationSetCacheStoragePerOriginQuota(WKContextConfigurationRef configuration, uint64_t quota);
> 
> I have two problems with this:
> 1) There's no corresponding ObjC SPI.  Doing this makes it even harder to
> adopt the ObjC SPI.
> 2) This shouldn't be on WKContext.  It should be on
> WKWebsiteDataStore/WKWebsiteDataStoreRef.  These functions should be
> removed.  Everything else here having to do with storage is on its way to
> being moved there.  Putting more things here makes that transition harder.

Agreed on moving it to WebsiteDataStore but I think it needs some unrelated work to happen before this can be made possible.
I updated the patch to have a default value of 400ko so that WTR does not have to configure this value.

[Build Bot]

Comment on attachment 322648 [details]
Patch

Attachment 322648 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/4755377

New failing tests:
workers/wasm-long-compile.html

[Build Bot]

Created attachment 322689 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6

[youenn fablet]

Comment on attachment 322648 [details]
Patch

Ping review?
EWS failure is unrelated

[youenn fablet]

rdar://problem/34865866

[Alex Christensen]

Comment on attachment 322648 [details]
Patch

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

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.h:85
> +        double insertionTime;
> +        uint64_t size;

These could use initializer lists to be safe.

[youenn fablet]

Created attachment 323212 [details]
Patch for landing

[Build Bot]

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


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:392:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCache.cpp:419:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 2 in 34 files


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

[WebKit Commit Bot]

Comment on attachment 323212 [details]
Patch for landing

Clearing flags on attachment: 323212

Committed r223073: <http://trac.webkit.org/changeset/223073>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.