Bug 178034 – Add API to clean CacheStorage data

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

[youenn fablet]

Add API to clean CacheStorage data

[youenn fablet]

Created attachment 323057 [details]
Patch

[youenn fablet]

Created attachment 323067 [details]
GTK fix

[youenn fablet]

Created attachment 323069 [details]
GTK fix

[youenn fablet]

Created attachment 323071 [details]
Patch

[youenn fablet]

rdar://problem/34396705

[youenn fablet]

Created attachment 323193 [details]
Patch

[youenn fablet]

Created attachment 323213 [details]
Patch

[youenn fablet]

(In reply to youenn fablet from comment #7)
> Created attachment 323213 [details]
> Patch

Introducing a new WebsiteDatastore type for DOMCache

[Chris Dumez]

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.

[Chris Dumez]

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.

[Chris Dumez]

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.

[youenn fablet]

Created attachment 323237 [details]
Patch

[youenn fablet]

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.

[Chris Dumez]

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.

[youenn fablet]

> > 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.

[Chris Dumez]

(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.

[youenn fablet]

Created attachment 323321 [details]
Patch

[youenn fablet]

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

[Chris Dumez]

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.

[youenn fablet]

Created attachment 323466 [details]
Patch for landing

[WebKit Commit Bot]

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

[WebKit Commit Bot]

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

[WebKit Commit Bot]

Comment on attachment 323466 [details]
Patch for landing

Clearing flags on attachment: 323466

Committed r223213: <https://trac.webkit.org/changeset/223213>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 323495 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 323495 [details]
Patch

Clearing flags on attachment: 323495

Committed r223220: <https://trac.webkit.org/changeset/223220>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Matt Lewis]

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

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 323599 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 323599 [details]
Patch

Clearing flags on attachment: 323599

Committed r223263: <https://trac.webkit.org/changeset/223263>

[WebKit Commit Bot]

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.

[youenn fablet]

Reopening to attach new patch.

[youenn fablet]

Created attachment 324257 [details]
Patch

[youenn fablet]

(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.

[WebKit Commit Bot]

Comment on attachment 324257 [details]
Patch

Clearing flags on attachment: 324257

Committed r223702: <https://trac.webkit.org/changeset/223702>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.