Summary: | Add API to clean CacheStorage data | ||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | youenn fablet <youennf> | ||||||||||||||||||||||||||||
Component: | WebCore Misc. | Assignee: | youenn fablet <youennf> | ||||||||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||||||||
Severity: | Normal | CC: | achristensen, benjamin, buildbot, cdumez, cgarcia, cmarcelo, commit-queue, dbates, jlewis3, mitz, webkit-bug-importer | ||||||||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||||||||||||
Attachments: |
|
Description
youenn fablet
2017-10-06 15:53:22 PDT
Created attachment 323057 [details]
Patch
Created attachment 323067 [details]
GTK fix
Created attachment 323069 [details]
GTK fix
Created attachment 323071 [details]
Patch
Created attachment 323193 [details]
Patch
Created attachment 323213 [details]
Patch
(In reply to youenn fablet from comment #7) > Created attachment 323213 [details] > Patch Introducing a new WebsiteDatastore type for DOMCache Comment on attachment 323213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323213&action=review > Source/WebKit/NetworkProcess/NetworkProcess.cpp:441 > + CacheStorage::Engine::clearEngines([this, callbackID] { This seems wrong if websiteDataTypes does not contain WebsiteDataType::DOMCache. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:493 > + completionHandler = [this, origins, callbackID] { I am assuming origins is a container types, in which case we would want to avoid the copy here. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:501 > + clearDiskCacheEntries(originDatas, WTFMove(completionHandler)); If websiteDataTypes.contains(WebsiteDataType::DOMCache) is false, the completionHandler will be a no-op and this will fail to send the IPC back to the parent process. > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:83 > + // FIXME: Support fetching entries notImplemented(); ? > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:106 > + auto cleanTaskHandler = adoptRef(*new ClearTasksHandler(WTFMove(completionHandler))); We may want a create() factory to abstract this. > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:113 > + auto cleanTaskHandler = adoptRef(*new ClearTasksHandler(WTFMove(completionHandler))); ditto. > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:362 > +static void deleteFolder(const String& filename) Should this be in FileSystem.h / FileSystem.cpp so that it can be reused? It seems useful. Comment on attachment 323213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323213&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataRecord.h:35 > +WK_EXTERN NSString * const WKWebsiteDataTypeDOMCache WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_MAC_TBA)); I think we need to find a better name than DOM cache, especially for the public API. Comment on attachment 323213 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323213&action=review > Source/WebKit/NetworkProcess/NetworkProcess.cpp:491 > + return originData.isEmpty() ? String { } : originData.securityOrigin()->toString(); Why is it OK to use a null String here? I am not familiar with WTF::map() but do we end up with a Vector potentially containing null Strings? If so, this may be an issue and a null origin String has a special meaning at lower level. >> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:362 >> +static void deleteFolder(const String& filename) > > Should this be in FileSystem.h / FileSystem.cpp so that it can be reused? It seems useful. Or maybe this could reuse deleteDirectoryRecursively() currently in NetworkCacheFileSystem.h ? > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:373 > +void Engine::clearCaches(const String& origin, ClearTasksHandler& taskHandler) I think the second parameter should be a WTF::Function instead of a taskHandler. The taskHandler is a call site concept and I think it should stay at the call site (the call site would capture it in the lambda on its side). > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:378 > + return; It is not clear to me why we want to return early here and not delete things on this but I am not super familiar with this code. > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:381 > + for (auto& caches : m_caches.values()) So passing a null origin has a special meaning? I find the API a bit confusing. It may read better if we used: void Engine::clearCaches(completionHandler); void Engine::clearCachesForOrigin(origin, completionHandler); > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:106 > +void Caches::clear(WTF::Function<void()>&& completionHandler) if m_storage is null, the completionHandler is never called, this seems like a poor API contract. Created attachment 323237 [details]
Patch
Thanks for the review. I took almost it all. See below for more details. > > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:373 > > +void Engine::clearCaches(const String& origin, ClearTasksHandler& taskHandler) > > I think the second parameter should be a WTF::Function instead of a > taskHandler. The taskHandler is a call site concept and I think it should > stay at the call site (the call site would capture it in the lambda on its > side). This API is private. It is easier to use a ClearTaskHandler as it may be ref-incremented several time in the same function call. > > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:378 > > + return; > > It is not clear to me why we want to return early here and not delete things > on this but I am not super familiar with this code. If the engine is not persistent, it writes nothing to disk. > > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:381 > > + for (auto& caches : m_caches.values()) > > So passing a null origin has a special meaning? I find the API a bit > confusing. It may read better if we used: > void Engine::clearCaches(completionHandler); > void Engine::clearCachesForOrigin(origin, completionHandler); Right, I went with clearAllCaches/clearCachesForOrigin. They mirror the existing clearEngines and clearEnginesForOrigins. > > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:106 > > +void Caches::clear(WTF::Function<void()>&& completionHandler) > > if m_storage is null, the completionHandler is never called, this seems like > a poor API contract. That is a bug. > > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataRecord.h:35 > > +WK_EXTERN NSString * const WKWebsiteDataTypeDOMCache WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_MAC_TBA)); > > I think we need to find a better name than DOM cache, especially for the > public API. Changed to WKWebsiteDataTypeFetchCache, a tad better but not very satisfactory either. Comment on attachment 323237 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323237&action=review > Source/WebKit/NetworkProcess/NetworkProcess.cpp:445 > + completionHandler = [completionHandler = WTFMove(completionHandler)]() mutable { It seems unfortunate that we have to finish for the disk cache data to be removed before we start removing DOMCache data. Ideally, we would use a CallbackAggregator and do the tasks in parallel, like we do elsewhere. It would also be less fragile. > Source/WebKit/NetworkProcess/NetworkProcess.cpp:500 > + completionHandler = [origins = WTFMove(origins), completionHandler = WTFMove(completionHandler)]() mutable { Same comment as above about using a callback aggregator. > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:90 > +class ClearTasksHandler : public RefCounted<ClearTasksHandler> { Although it is probably OK right now, it seems risky for this object to not be thread safe refcounted (given that it is passed to background threads). > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:388 > + if (!shouldPersist()) Everything bellow here is duplicated with the method above and should probably be consolidated into a single function. > > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:90
> > +class ClearTasksHandler : public RefCounted<ClearTasksHandler> {
>
> Although it is probably OK right now, it seems risky for this object to not
> be thread safe refcounted (given that it is passed to background threads).
The main issue is the callback to be executed on the right thread.
We can make this ThreadSafeRefCounted in which case the destructor should make sure the callback is properly executed.
(In reply to youenn fablet from comment #15) > > > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:90 > > > +class ClearTasksHandler : public RefCounted<ClearTasksHandler> { > > > > Although it is probably OK right now, it seems risky for this object to not > > be thread safe refcounted (given that it is passed to background threads). > > The main issue is the callback to be executed on the right thread. > We can make this ThreadSafeRefCounted in which case the destructor should > make sure the callback is properly executed. You are passing this RefCounted object to a background I/O queue in at least one place. While I think it is currently safe because you WTFMove() carefully, this seems a bit fragile. The same code capturing the object by value would only work it was ThreadSafeRefCounted. Note that adding the assertion you suggest is probably a good idea either way. Just because your object is not ThreadSafeRefCounted does not guarantee it will get destroyed on the main thread. Created attachment 323321 [details]
Patch
Comment on attachment 323321 [details]
Patch
Starting to use CompletionHandler instead of WTF::Function.
There are a few places where we should probably use CompletionHandler.
I'll do that in another iteration/follow-up
Comment on attachment 323321 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=323321&action=review r=me with comments. > Source/WTF/wtf/CallbackAggregator.h:29 > +#include "RunLoop.h" Probably want MainThread.h instead. > Source/WTF/wtf/CallbackAggregator.h:46 > + RunLoop::main().dispatch([callback = WTFMove(m_callback)] () mutable { You cannot use RunLoop::main().dispatch() here because of WK1/iOS. callOnMainThread() is the way to go I believe. > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:123 > +void Caches::clear(WTF::Function<void()>&& completionHandler) We may want to use a WTF::CompletionHandler here instead. > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.h:73 > +WK_EXPORT void WKWebsiteDataStoreRemoveDOMCacheForOrigin(WKWebsiteDataStoreRef dataStoreRef, WKSecurityOriginRef origin); I do not think we should be use DOMCache in API. > Source/WebKit/UIProcess/API/C/WKWebsiteDataStoreRef.h:74 > +WK_EXPORT void WKWebsiteDataStoreRemoveAllDOMCaches(WKWebsiteDataStoreRef dataStoreRef); ditto. > Tools/WebKitTestRunner/TestController.cpp:2308 > + WKWebsiteDataStoreRemoveDOMCacheForOrigin(websiteDataStore, WKSecurityOriginCreateFromString(origin)); Looks like you're leaking the result of WKSecurityOriginCreateFromString() ? I guess you want to adopt the result and store it in a local variable. Created attachment 323466 [details]
Patch for landing
Comment on attachment 323466 [details] Patch for landing Rejecting attachment 323466 [details] from commit-queue. New failing tests: imported/w3c/web-platform-tests/XMLHttpRequest/open-url-worker-origin.htm Full output: http://webkit-queues.webkit.org/results/4828599 Created attachment 323478 [details]
Archive of layout-test-results from webkit-cq-03 for mac-elcapitan
The attached test failures were seen while running run-webkit-tests on the commit-queue.
Bot: webkit-cq-03 Port: mac-elcapitan Platform: Mac OS X 10.11.6
Comment on attachment 323466 [details] Patch for landing Clearing flags on attachment: 323466 Committed r223213: <https://trac.webkit.org/changeset/223213> All reviewed patches have been landed. Closing bug. Reopening to attach new patch. Created attachment 323495 [details]
Patch
Comment on attachment 323495 [details] Patch Clearing flags on attachment: 323495 Committed r223220: <https://trac.webkit.org/changeset/223220> All reviewed patches have been landed. Closing bug. The test added in the first patch (r223213) is an extremely flaky timeout: https://build.webkit.org/results/Apple%20Sierra%20Debug%20WK2%20(Tests)/r223239%20(3497)/results.html https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fcache-storage%2Fcache-clearing.https.html Diff: --- /Volumes/Data/slave/sierra-debug-tests-wk2/build/layout-test-results/http/tests/cache-storage/cache-clearing.https-expected.txt +++ /Volumes/Data/slave/sierra-debug-tests-wk2/build/layout-test-results/http/tests/cache-storage/cache-clearing.https-actual.txt @@ -1,5 +1,5 @@ +#PID UNRESPONSIVE - com.apple.WebKit.WebContent.Development (pid 75712) +FAIL: Timed out waiting for notifyDone to be called -PASS Cleaning existing caches -PASS Clearing disk cache of a given origin -PASS Clearing all disk cache - +#EOF +#EOF Reopening to attach new patch. Created attachment 323599 [details]
Patch
Comment on attachment 323599 [details] Patch Clearing flags on attachment: 323599 Committed r223263: <https://trac.webkit.org/changeset/223263> All reviewed patches have been landed. Closing bug. Comment on attachment 323466 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=323466&action=review > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataRecord.h:35 > +WK_EXTERN NSString * const WKWebsiteDataTypeFetchCache WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_MAC_TBA)); WK_MAC_TBA is not an appropriate value for iOS. Reopening to attach new patch. Created attachment 324257 [details]
Patch
(In reply to mitz from comment #34) > Comment on attachment 323466 [details] > Patch for landing > > View in context: > https://bugs.webkit.org/attachment.cgi?id=323466&action=review > > > Source/WebKit/UIProcess/API/Cocoa/WKWebsiteDataRecord.h:35 > > +WK_EXTERN NSString * const WKWebsiteDataTypeFetchCache WK_API_AVAILABLE(macosx(WK_MAC_TBA), ios(WK_MAC_TBA)); > > WK_MAC_TBA is not an appropriate value for iOS. Thanks, fixing it in latest patch. Comment on attachment 324257 [details] Patch Clearing flags on attachment: 324257 Committed r223702: <https://trac.webkit.org/changeset/223702> All reviewed patches have been landed. Closing bug. |