Add quota to cache API
Created attachment 321982 [details] Patch
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.
Created attachment 321987 [details] Patch
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 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
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
Created attachment 322025 [details] Patch
Created attachment 322026 [details] Patch
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.
Created attachment 322045 [details] Patch
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 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.
> > 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.
> 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?
(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 :(
Created attachment 322111 [details] Patch
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.
Created attachment 322113 [details] Patch
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 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
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 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
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
Created attachment 322189 [details] Patch
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 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.
Created attachment 322648 [details] Patch
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.
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 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
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 on attachment 322648 [details] Patch Ping review? EWS failure is unrelated
rdar://problem/34865866
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.
Created attachment 323212 [details] Patch for landing
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 on attachment 323212 [details] Patch for landing Clearing flags on attachment: 323212 Committed r223073: <http://trac.webkit.org/changeset/223073>
All reviewed patches have been landed. Closing bug.