Bug 175995 – [Cache API] Support cache names persistency

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

[youenn fablet]

Add support for cache names persistency

[youenn fablet]

Created attachment 319106 [details]
Patch

[Build Bot]

Attachment 319106 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:95:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:101:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:125:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 22 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 319107 [details]
Patch

[Build Bot]

Attachment 319107 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:95:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:101:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:125:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 3 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 319114 [details]
Patch

[Build Bot]

Attachment 319114 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:95:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:101:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:125:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:123:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 23 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 319119 [details]
Patch

[Build Bot]

Attachment 319119 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:95:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:101:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:125:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:123:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 25 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 319119 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319119&action=review

> Source/WebCore/Modules/cache/CacheStorageConnection.h:49
> +    // Used only for testing purposes.
> +    virtual void setPersistency(const String& /* origin */, bool /* isPersistent */, DOMCache::CompletionCallback&& callback) { callback(DOMCache::Error::NotImplemented); }

Maybe we should call this setPersistencyForTesting, and I think we should do something similar to setPrivateBrowsingEnabled instead and use SessionID to determine whether a cache should touch the disk or not.

[youenn fablet]

Created attachment 319201 [details]
Patch

[Build Bot]

Attachment 319201 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:95:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:101:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:125:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:122:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 27 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

(In reply to Alex Christensen from comment #9)
> Comment on attachment 319119 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319119&action=review
> 
> > Source/WebCore/Modules/cache/CacheStorageConnection.h:49
> > +    // Used only for testing purposes.
> > +    virtual void setPersistency(const String& /* origin */, bool /* isPersistent */, DOMCache::CompletionCallback&& callback) { callback(DOMCache::Error::NotImplemented); }
> 
> Maybe we should call this setPersistencyForTesting, and I think we should do
> something similar to setPrivateBrowsingEnabled instead and use SessionID to
> determine whether a cache should touch the disk or not.

I was not sure whether we wanted to implement cache API in private browsing with in-memory-only caches. Let's do it then.
This will probably end up having a much smaller quota for private browsing than for regular sessions.

I removed setPersistencyForTesting and used setPrivateBrowsingEnabled in the test.

[Sam Weinig]

Comment on attachment 319201 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319201&action=review

> Source/WebCore/Modules/cache/DOMCache.cpp:44
> +        return Exception { TypeError, ASCIILiteral("Failed reading data from disk") };

I'm not sure 'disk' is the best name here, as many machines don't have hard-disks anymore. I like 'FileSystem' as a modifier, but I am sure we could come up with others.

[Alex Christensen]

Comment on attachment 319201 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319201&action=review

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:47
> +    String sessionSubdirectory = ASCIILiteral("session") + String::number(sessionID.sessionID());
> +    return WebCore::pathByAppendingComponent(NetworkProcess::singleton().cacheStorageDirectory(), sessionSubdirectory);

This should be a String from the WebsiteDataStore.

[youenn fablet]

Created attachment 319219 [details]
Patch

[Build Bot]

Attachment 319219 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:95:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:101:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:125:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:117:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 38 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

(In reply to Sam Weinig from comment #13)
> Comment on attachment 319201 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319201&action=review
> 
> > Source/WebCore/Modules/cache/DOMCache.cpp:44
> > +        return Exception { TypeError, ASCIILiteral("Failed reading data from disk") };
> 
> I'm not sure 'disk' is the best name here, as many machines don't have
> hard-disks anymore. I like 'FileSystem' as a modifier, but I am sure we
> could come up with others.

Changed to "file system".

(In reply to Alex Christensen from comment #14)
> Comment on attachment 319201 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319201&action=review
> 
> > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:47
> > +    String sessionSubdirectory = ASCIILiteral("session") + String::number(sessionID.sessionID());
> > +    return WebCore::pathByAppendingComponent(NetworkProcess::singleton().cacheStorageDirectory(), sessionSubdirectory);
> 
> This should be a String from the WebsiteDataStore.

I added a new parameter to WebsiteDataStoreParameters to pass the sub folder name.
This enforces one main folder for cache storage and one subfolder for each cache storage engine.

[youenn fablet]

Created attachment 319221 [details]
Patch

[Build Bot]

Attachment 319221 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:95:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:101:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:125:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:118:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 4 in 39 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

Created attachment 319303 [details]
Patch

[Build Bot]

Attachment 319303 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:95:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:101:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:105:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:129:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:111:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Alex Christensen]

Comment on attachment 319303 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=319303&action=review

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:246
> +    if (!shouldPersist()) {
> +        m_salt = makeSalt();

I don't think we should need  a salt of it's not persistent.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:361
> +            callback(std::nullopt);

Let's put an ASSERT(RunLoop::isMain()); here.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:384
> +            callback(data, error);

Same here.  It might be worth having a bunch of such assertions.

> Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:163
> +    Vector<String> names;
> +    names.reserveInitialCapacity(count);

We should use the VectorCoders and put the reserveInitialCapacity optimization in them.

[youenn fablet]

Created attachment 319409 [details]
Patch for landing

[Build Bot]

Attachment 319409 [details] did not pass style-queue:


ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:95:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:101:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:105:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:128:  More than one command on the same line  [whitespace/newline] [4]
ERROR: Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:111:  More than one command on the same line  [whitespace/newline] [4]
Total errors found: 5 in 28 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[youenn fablet]

(In reply to Alex Christensen from comment #22)
> Comment on attachment 319303 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=319303&action=review
> 
> > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:246
> > +    if (!shouldPersist()) {
> > +        m_salt = makeSalt();
> 
> I don't think we should need  a salt of it's not persistent.

Will do as a follow-up

> > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:361
> > +            callback(std::nullopt);
> 
> Let's put an ASSERT(RunLoop::isMain()); here.

OK

> 
> > Source/WebKit/NetworkProcess/cache/CacheStorageEngine.cpp:384
> > +            callback(data, error);
> 
> Same here.  It might be worth having a bunch of such assertions.

OK

> 
> > Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp:163
> > +    Vector<String> names;
> > +    names.reserveInitialCapacity(count);
> 
> We should use the VectorCoders and put the reserveInitialCapacity
> optimization in them.

I sticked with the current decoder since the encoder side is not using the Vector encoder either as it would need to create a vector of string for that.

[WebKit Commit Bot]

Comment on attachment 319409 [details]
Patch for landing

Clearing flags on attachment: 319409

Committed r221403: <http://trac.webkit.org/changeset/221403>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/34174534>

[Ryan Haddad]

The LayoutTest added with this change (http/tests/cache-storage/cache-persistency.https.html) is failing an assertion on El Capitan:

ASSERTION FAILED: position != notFound
/Volumes/Data/slave/elcapitan-debug/build/Source/WebKit/NetworkProcess/cache/CacheStorageEngineCaches.cpp(130) : void WebKit::CacheStorage::Caches::remove(uint64_t, CompletionCallback &&)

https://build.webkit.org/results/Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r221417%20(2811)/results.html

[youenn fablet]

(In reply to Ryan Haddad from comment #29)
> The LayoutTest added with this change
> (http/tests/cache-storage/cache-persistency.https.html) is failing an
> assertion on El Capitan:
> 
> ASSERTION FAILED: position != notFound
> /Volumes/Data/slave/elcapitan-debug/build/Source/WebKit/NetworkProcess/cache/
> CacheStorageEngineCaches.cpp(130) : void
> WebKit::CacheStorage::Caches::remove(uint64_t, CompletionCallback &&)
> 
> https://build.webkit.org/results/
> Apple%20El%20Capitan%20Debug%20WK2%20(Tests)/r221417%20(2811)/results.html

Will have a look at it.
Thanks!

[Matt Lewis]

This change also caused several imported tests to fail intermittently and some to start timing out intermittently:

Text diff:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fcache-storage%2Fwindow%2Fcache-storage-keys.https.html
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fcache-storage%2Fworker%2Fcache-storage-keys.https.html

Timeout:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=imported%2Fw3c%2Fweb-platform-tests%2Fservice-workers%2Fcache-storage%2Fcommon.https.html