Bug 178034 - Add API to clean CacheStorage data
Summary: Add API to clean CacheStorage data
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2017-10-06 15:53 PDT by youenn fablet
Modified: 2017-10-19 13:15 PDT (History)
11 users (show)

See Also:


Attachments
Patch (33.73 KB, patch)
2017-10-06 16:01 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
GTK fix (34.74 KB, patch)
2017-10-06 16:52 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
GTK fix (35.22 KB, patch)
2017-10-06 17:05 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (35.07 KB, patch)
2017-10-06 17:08 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (35.26 KB, patch)
2017-10-09 11:37 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (37.27 KB, patch)
2017-10-09 13:57 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (37.94 KB, patch)
2017-10-09 16:05 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (43.74 KB, patch)
2017-10-10 10:54 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (44.15 KB, patch)
2017-10-11 14:55 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from webkit-cq-03 for mac-elcapitan (1.03 MB, application/zip)
2017-10-11 16:06 PDT, WebKit Commit Bot
no flags Details
Patch (2.39 KB, patch)
2017-10-11 17:29 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (1.19 KB, patch)
2017-10-12 16:39 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (1.53 KB, patch)
2017-10-19 12:00 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2017-10-06 15:53:22 PDT
Add API to clean CacheStorage data
Comment 1 youenn fablet 2017-10-06 16:01:27 PDT
Created attachment 323057 [details]
Patch
Comment 2 youenn fablet 2017-10-06 16:52:21 PDT
Created attachment 323067 [details]
GTK fix
Comment 3 youenn fablet 2017-10-06 17:05:40 PDT
Created attachment 323069 [details]
GTK fix
Comment 4 youenn fablet 2017-10-06 17:08:30 PDT
Created attachment 323071 [details]
Patch
Comment 5 youenn fablet 2017-10-06 18:26:36 PDT
rdar://problem/34396705
Comment 6 youenn fablet 2017-10-09 11:37:23 PDT
Created attachment 323193 [details]
Patch
Comment 7 youenn fablet 2017-10-09 13:57:00 PDT
Created attachment 323213 [details]
Patch
Comment 8 youenn fablet 2017-10-09 13:57:31 PDT
(In reply to youenn fablet from comment #7)
> Created attachment 323213 [details]
> Patch

Introducing a new WebsiteDatastore type for DOMCache
Comment 9 Chris Dumez 2017-10-09 14:53:47 PDT
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 10 Chris Dumez 2017-10-09 15:07:40 PDT
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 11 Chris Dumez 2017-10-09 15:20:10 PDT
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.
Comment 12 youenn fablet 2017-10-09 16:05:15 PDT
Created attachment 323237 [details]
Patch
Comment 13 youenn fablet 2017-10-09 16:09:24 PDT
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 14 Chris Dumez 2017-10-09 16:23:42 PDT
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.
Comment 15 youenn fablet 2017-10-09 20:08:37 PDT
> > 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.
Comment 16 Chris Dumez 2017-10-09 20:15:32 PDT
(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.
Comment 17 youenn fablet 2017-10-10 10:54:26 PDT
Created attachment 323321 [details]
Patch
Comment 18 youenn fablet 2017-10-10 11:48:18 PDT
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 19 Chris Dumez 2017-10-11 09:03:00 PDT
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.
Comment 20 youenn fablet 2017-10-11 14:55:09 PDT
Created attachment 323466 [details]
Patch for landing
Comment 21 WebKit Commit Bot 2017-10-11 16:06:44 PDT
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
Comment 22 WebKit Commit Bot 2017-10-11 16:06:46 PDT
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 23 WebKit Commit Bot 2017-10-11 16:55:15 PDT
Comment on attachment 323466 [details]
Patch for landing

Clearing flags on attachment: 323466

Committed r223213: <https://trac.webkit.org/changeset/223213>
Comment 24 WebKit Commit Bot 2017-10-11 16:55:17 PDT
All reviewed patches have been landed.  Closing bug.
Comment 25 youenn fablet 2017-10-11 17:29:55 PDT
Reopening to attach new patch.
Comment 26 youenn fablet 2017-10-11 17:29:56 PDT
Created attachment 323495 [details]
Patch
Comment 27 WebKit Commit Bot 2017-10-11 17:47:02 PDT
Comment on attachment 323495 [details]
Patch

Clearing flags on attachment: 323495

Committed r223220: <https://trac.webkit.org/changeset/223220>
Comment 28 WebKit Commit Bot 2017-10-11 17:47:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 29 Matt Lewis 2017-10-12 13:15:23 PDT
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
Comment 30 youenn fablet 2017-10-12 16:39:16 PDT
Reopening to attach new patch.
Comment 31 youenn fablet 2017-10-12 16:39:17 PDT
Created attachment 323599 [details]
Patch
Comment 32 WebKit Commit Bot 2017-10-12 17:19:51 PDT
Comment on attachment 323599 [details]
Patch

Clearing flags on attachment: 323599

Committed r223263: <https://trac.webkit.org/changeset/223263>
Comment 33 WebKit Commit Bot 2017-10-12 17:19:53 PDT
All reviewed patches have been landed.  Closing bug.
Comment 34 mitz 2017-10-17 22:20:53 PDT
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.
Comment 35 youenn fablet 2017-10-19 12:00:11 PDT
Reopening to attach new patch.
Comment 36 youenn fablet 2017-10-19 12:00:12 PDT
Created attachment 324257 [details]
Patch
Comment 37 youenn fablet 2017-10-19 12:01:19 PDT
(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 38 WebKit Commit Bot 2017-10-19 13:15:45 PDT
Comment on attachment 324257 [details]
Patch

Clearing flags on attachment: 324257

Committed r223702: <https://trac.webkit.org/changeset/223702>
Comment 39 WebKit Commit Bot 2017-10-19 13:15:48 PDT
All reviewed patches have been landed.  Closing bug.