Bug 177552

Summary: Add quota to cache API
Product: WebKit Reporter: youenn fablet <youennf>
Component: WebKit Misc.Assignee: youenn fablet <youennf>
Status: RESOLVED FIXED    
Severity: Normal CC: achristensen, beidson, buildbot, cdumez, cgarcia, commit-queue, esprehn+autocc, kondapallykalyan, rniwa, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
buildbot: commit-queue-
Archive of layout-test-results from ews104 for mac-elcapitan-wk2
none
Patch
none
Patch
none
Patch
none
Patch
none
Patch
none
Archive of layout-test-results from ews105 for mac-elcapitan-wk2
none
Archive of layout-test-results from ews123 for ios-simulator-wk2
none
Patch
none
Patch
none
Archive of layout-test-results from ews116 for mac-elcapitan
none
Patch for landing none

Description youenn fablet 2017-09-27 11:00:52 PDT
Add quota to cache API
Comment 1 youenn fablet 2017-09-27 11:33:25 PDT
Created attachment 321982 [details]
Patch
Comment 2 Build Bot 2017-09-27 11:34:58 PDT
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.
Comment 3 youenn fablet 2017-09-27 11:58:02 PDT
Created attachment 321987 [details]
Patch
Comment 4 Build Bot 2017-09-27 12:01:50 PDT
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.
Comment 5 Build Bot 2017-09-27 13:04:52 PDT
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
Comment 6 Build Bot 2017-09-27 13:04:57 PDT
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
Comment 7 youenn fablet 2017-09-27 15:03:47 PDT
Created attachment 322025 [details]
Patch
Comment 8 youenn fablet 2017-09-27 15:06:34 PDT
Created attachment 322026 [details]
Patch
Comment 9 Build Bot 2017-09-27 15:46:44 PDT
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.
Comment 10 youenn fablet 2017-09-27 17:10:12 PDT
Created attachment 322045 [details]
Patch
Comment 11 Build Bot 2017-09-27 17:11:18 PDT
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.
Comment 12 Alex Christensen 2017-09-28 10:01:51 PDT
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.
Comment 13 youenn fablet 2017-09-28 10:15:31 PDT
> > 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.
Comment 14 youenn fablet 2017-09-28 10:19:27 PDT
> 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?
Comment 15 Alex Christensen 2017-09-28 10:25:31 PDT
(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 :(
Comment 16 youenn fablet 2017-09-28 13:43:08 PDT
Created attachment 322111 [details]
Patch
Comment 17 Build Bot 2017-09-28 13:44:23 PDT
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.
Comment 18 youenn fablet 2017-09-28 14:00:02 PDT
Created attachment 322113 [details]
Patch
Comment 19 Build Bot 2017-09-28 14:02:41 PDT
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.
Comment 20 Build Bot 2017-09-28 15:14:17 PDT
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
Comment 21 Build Bot 2017-09-28 15:14:19 PDT
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
Comment 22 Build Bot 2017-09-28 15:34:25 PDT
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
Comment 23 Build Bot 2017-09-28 15:34:27 PDT
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
Comment 24 youenn fablet 2017-09-29 09:37:45 PDT
Created attachment 322189 [details]
Patch
Comment 25 Build Bot 2017-09-29 09:40:16 PDT
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.
Comment 26 Alex Christensen 2017-10-03 18:13:54 PDT
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.
Comment 27 youenn fablet 2017-10-04 03:28:53 PDT
Created attachment 322648 [details]
Patch
Comment 28 Build Bot 2017-10-04 03:30:51 PDT
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.
Comment 29 youenn fablet 2017-10-04 03:34:17 PDT
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.
Comment 30 Build Bot 2017-10-04 10:53:37 PDT
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
Comment 31 Build Bot 2017-10-04 10:53:39 PDT
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
Comment 32 youenn fablet 2017-10-06 16:46:23 PDT
Comment on attachment 322648 [details]
Patch

Ping review?
EWS failure is unrelated
Comment 33 youenn fablet 2017-10-06 18:25:24 PDT
rdar://problem/34865866
Comment 34 Alex Christensen 2017-10-09 13:21:27 PDT
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.
Comment 35 youenn fablet 2017-10-09 13:42:46 PDT
Created attachment 323212 [details]
Patch for landing
Comment 36 Build Bot 2017-10-09 13:45:12 PDT
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.
Comment 37 WebKit Commit Bot 2017-10-09 15:17:09 PDT
Comment on attachment 323212 [details]
Patch for landing

Clearing flags on attachment: 323212

Committed r223073: <http://trac.webkit.org/changeset/223073>
Comment 38 WebKit Commit Bot 2017-10-09 15:17:11 PDT
All reviewed patches have been landed.  Closing bug.