Bug 195707 – Cache API and IDB space usages should be initialized on first quota check

• Component: Service Workers
• Status: RESOLVED FIXED

[youenn fablet]

Cache API and IDB space usages should be initialized on first quota check

[youenn fablet]

Created attachment 364831 [details]
Patch

[youenn fablet]

Created attachment 365489 [details]
Patch

[EWS Watchlist]

Comment on attachment 365489 [details]
Patch

Attachment 365489 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/11593464

New failing tests:
http/tests/IndexedDB/storage-limit-2.https.html
http/tests/IndexedDB/storage-limit-1.https.html

[EWS Watchlist]

Created attachment 365503 [details]
Archive of layout-test-results from ews102 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews102  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365489 [details]
Patch

Attachment 365489 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/11593619

New failing tests:
http/tests/IndexedDB/storage-limit-2.https.html
http/tests/IndexedDB/storage-limit-1.https.html

[EWS Watchlist]

Created attachment 365509 [details]
Archive of layout-test-results from ews113 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews113  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 365489 [details]
Patch

Attachment 365489 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/11594299

New failing tests:
http/tests/IndexedDB/storage-limit-2.https.html
http/tests/IndexedDB/storage-limit-1.https.html

[EWS Watchlist]

Created attachment 365513 [details]
Archive of layout-test-results from ews206 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews206  Port: win-future  Platform: CYGWIN_NT-6.1-2.9.0-0.318-5-3-x86_64-64bit

[youenn fablet]

Created attachment 365544 [details]
Patch

[youenn fablet]

Created attachment 365556 [details]
Patch

[Chris Dumez]

Comment on attachment 365556 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:128
> +    void initializeQuotaUser(const ClientOrigin& origin) { quotaUser(origin); }

Please rename quotaUser() to ensureQuotaUser() for clarity, it looks really weird to call a getter and expect a side effect otherwise.

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:180
> +    WEBCORE_EXPORT QuotaUser& quotaUser(const ClientOrigin&);

Please rename.

> Source/WebCore/storage/StorageQuotaManager.cpp:90
> +    if (m_pendingInitializationUsers.isEmpty())

Do we want to do this only if we actually removed the user from m_pendingInitializationUsers?

> Source/WebCore/storage/StorageQuotaManager.cpp:122
> +void StorageQuotaManager::processPendingRequests(Optional<uint64_t> newQuota, ShouldProcessFirst shouldProcessFirst)

It is hard for me to understand what ShouldProcessFirst means here? First? Before what? I am calling process function after all.

> Source/WebCore/storage/StorageQuotaManager.h:68
> +    void processPendingRequests(Optional<uint64_t>, ShouldProcessFirst = ShouldProcessFirst::No);

I do not think we should omit the name of the first parameter.

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:2397
> +class QuotaUserInitializer : public WebCore::StorageQuotaUser {

final

[youenn fablet]

> > Source/WebCore/Modules/indexeddb/server/IDBServer.h:128
> > +    void initializeQuotaUser(const ClientOrigin& origin) { quotaUser(origin); }
> 
> Please rename quotaUser() to ensureQuotaUser() for clarity, it looks really
> weird to call a getter and expect a side effect otherwise.

OK
> > Source/WebCore/storage/StorageQuotaManager.cpp:90
> > +    if (m_pendingInitializationUsers.isEmpty())
> 
> Do we want to do this only if we actually removed the user from
> m_pendingInitializationUsers?

There is no harm in calling it more than necessary.
But this is a small optimization.
Will do.

> > Source/WebCore/storage/StorageQuotaManager.cpp:122
> > +void StorageQuotaManager::processPendingRequests(Optional<uint64_t> newQuota, ShouldProcessFirst shouldProcessFirst)
> 
> It is hard for me to understand what ShouldProcessFirst means here? First?
> Before what? I am calling process function after all.

Renamed to ShouldDequeueFirstPendingRequest.
The point is that on reception of a quota decision from UIProcess, we have all information to take a decision for the first pending request since it was the one triggering the decision from UIProcess.

> > Source/WebCore/storage/StorageQuotaManager.h:68
> > +    void processPendingRequests(Optional<uint64_t>, ShouldProcessFirst = ShouldProcessFirst::No);
> 
> I do not think we should omit the name of the first parameter.

OK

> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:2397
> > +class QuotaUserInitializer : public WebCore::StorageQuotaUser {
> 
> final

OK

[youenn fablet]

Created attachment 365577 [details]
Patch

[EWS Watchlist]

Comment on attachment 365577 [details]
Patch

Attachment 365577 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/11603093

New failing tests:
http/tests/cache-storage/cache-clearing-origin.https.html

[EWS Watchlist]

Created attachment 365613 [details]
Archive of layout-test-results from ews124 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews124  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[youenn fablet]

Created attachment 365624 [details]
Patch for landing

[youenn fablet]

(In reply to Build Bot from comment #14)
> Comment on attachment 365577 [details]
> Patch
> 
> Attachment 365577 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: https://webkit-queues.webkit.org/results/11603093
> 
> New failing tests:
> http/tests/cache-storage/cache-clearing-origin.https.html

Test is already flaky on MacOS.
I updated the expectation for iOS simulator.
We might want to change the way we report DOM Cache presence for a given origin.
Currently, this is based on presence of a folder, even if empty.
This folder will now be created whenever a request for quota is made.

[WebKit Commit Bot]

Comment on attachment 365624 [details]
Patch for landing

Clearing flags on attachment: 365624

Committed r243339: <https://trac.webkit.org/changeset/243339>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/49129800>

[Shawn Roberts]

It looks like the changes in https://trac.webkit.org/changeset/243345/webkit

is causing the following tests to fail around 75% of the time on Mac and iOS Simulator WK2 and Debug

http/tests/cache-storage/cache-records-persistency.https.html (was marked flaky previously on Mac)
http/tests/cache-storage/cache-clearing-origin.https.html

Dashboard :

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html?tests=http%2Ftests%2Fcache-storage%2Fcache-records-persistency.https.html+performance-api%2Fperformance-observer-periodic.html+scrollingcoordinator%2Fios%2Fui-scroll-fixed.html+storage%2Findexeddb%2Fmodern%2Fidbtransaction-objectstore-failures-private.html#showAllRuns=true&tests=http%2Ftests%2Fcache-storage%2Fcache-records-persistency.https.html%20http%2Ftests%2Fcache-storage%2Fcache-clearing-origin.https.html

Prior to change the tests were still flaky failures, but after this change they have about a 75% failure rate.

Reproduced with :

run-webkit-tests http/tests/cache-storage/cache-records-persistency.https.html http/tests/cache-storage/cache-clearing-origin.https.html --iterations 100 --child-process 1 --ios-simulator --exit-after-n-failures=10 --no-retry

[Shawn Roberts]

Pasted wrong revision.

The changes in https://trac.webkit.org/changeset/243339 Caused the above.

[Truitt Savell]

It looks like the changes in https://trac.webkit.org/changeset/243339/webkit

has caused the test http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html

to become significantly flakey. I am able to reproduce the failure using command:
run-webkit-tests http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html --iterations 2500 -f

The test fails often on 243339 and not on 243338.

history:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2FresourceLoadStatistics%2Fwebsite-data-removal-for-site-navigated-to-with-link-decoration.html

Diff:
--- /Volumes/Data/slave/highsierra-debug-tests-wk2/build/layout-test-results/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-expected.txt
+++ /Volumes/Data/slave/highsierra-debug-tests-wk2/build/layout-test-results/http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration-actual.txt
@@ -32,4 +32,4 @@
         localhost
     gotLinkDecorationFromPrevalentResource: No    isPrevalentResource: No
     isVeryPrevalentResource: No
-    dataRecordsRemoved: 1
+    dataRecordsRemoved: 0

[youenn fablet]

I think these issues are explained by https://bugs.webkit.org/show_bug.cgi?id=195707#c17
Either we should change the way we report DOM Cache being there for a given origin or we should delay checking of a user being initialized to the first quota request.
The latter is probably better.