Bug 195302 – Check IDB quota usage through QuotaManager

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[youenn fablet]

Check IDB quota usage through QuotaManager

[youenn fablet]

Created attachment 363573 [details]
Patch

[youenn fablet]

Created attachment 363988 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:62:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 31 files


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

[EWS Watchlist]

Comment on attachment 363988 [details]
Patch

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

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 363990 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

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

[EWS Watchlist]

Comment on attachment 363988 [details]
Patch

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

New failing tests:
storage/indexeddb/transaction-coordination-across-databases-private.html
storage/indexeddb/transaction-coordination-across-databases.html
storage/indexeddb/storage-limit.html
storage/websql/open-database-while-transaction-in-progress.html
storage/websql/transaction-database-expand-quota.html
storage/indexeddb/storage-limit-2.html

[EWS Watchlist]

Created attachment 363991 [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 363988 [details]
Patch

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

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 363993 [details]
Archive of layout-test-results from ews114 for mac-highsierra

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

[EWS Watchlist]

Comment on attachment 363988 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall-no-ssrcs.https.html

[EWS Watchlist]

Created attachment 364005 [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 364201 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:62:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Tools/WebKitTestRunner/cocoa/TestWebsiteDataStoreDelegate.mm:38:  The parameter name "quota" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 44 files


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

[EWS Watchlist]

Created attachment 364211 [details]
Archive of layout-test-results from ews103 for mac-highsierra

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

[youenn fablet]

Created attachment 364217 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:62:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
ERROR: Tools/WebKitTestRunner/cocoa/TestWebsiteDataStoreDelegate.mm:38:  The parameter name "quota" adds no information, so it should be removed.  [readability/parameter_name] [5]
Total errors found: 2 in 45 files


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

[youenn fablet]

Created attachment 364283 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:62:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 32 files


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

[youenn fablet]

Created attachment 364308 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:62:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 32 files


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

[youenn fablet]

Created attachment 364310 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:62:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 32 files


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

[youenn fablet]

Created attachment 364345 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:62:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 32 files


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

[EWS Watchlist]

Comment on attachment 364345 [details]
Patch

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

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

[EWS Watchlist]

Created attachment 364368 [details]
Archive of layout-test-results from ews205 for win-future

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

[youenn fablet]

Created attachment 364395 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:62:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 31 files


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

[youenn fablet]

Created attachment 364418 [details]
Patch

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:62:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 31 files


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

[youenn fablet]

Wincairo error seems unrelated.

[youenn fablet]

ping review

[Chris Dumez]

Comment on attachment 364418 [details]
Patch

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

Would be good if Brady could take a look but it seems fine.

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:703
> +void IDBServer::QuotaUser::clearSpaceUsed()

I'd call it 'resetSpaceSpaced'. Clear makes it sound like it might actually remove files.

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:782
> +    quotaManager->requestSpace(taskSize, [callback = WTFMove(callback)](auto decision) mutable {

Doesn't this work?

quotaManager->requestSpace(taskSize, WTFMove(callback));

> Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:74
> +    return [&server](PAL::SessionID, const auto& origin) {

Seems risky to capture server by reference here given that you are not calling the lambda right away and leaving it up to the caller. If the caller were to store this lambda and dispatch then call it, we could crash.

[youenn fablet]

Thanks for the review.

(In reply to Chris Dumez from comment #33)
> Comment on attachment 364418 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=364418&action=review
> 
> Would be good if Brady could take a look but it seems fine.

Sure, I'll wait to land this patch until this afternoon.

> > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:703
> > +void IDBServer::QuotaUser::clearSpaceUsed()
> 
> I'd call it 'resetSpaceSpaced'. Clear makes it sound like it might actually
> remove files.

OK

> > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:782
> > +    quotaManager->requestSpace(taskSize, [callback = WTFMove(callback)](auto decision) mutable {
> 
> Doesn't this work?
> 
> quotaManager->requestSpace(taskSize, WTFMove(callback));

Right.

> > Source/WebCore/Modules/indexeddb/shared/InProcessIDBServer.cpp:74
> > +    return [&server](PAL::SessionID, const auto& origin) {
> 
> Seems risky to capture server by reference here given that you are not
> calling the lambda right away and leaving it up to the caller. If the caller
> were to store this lambda and dispatch then call it, we could crash.

Well InProcessIDBServer is passing *this in the IDBServer constructor and IDBServer keeps it as a reference so I thought that was fine.
Looking more closely though, it seems that the *this reference might never be used in the case of InProcessIDBServer. I'll make InProcessIDBServer WeakPtr-able to make things stronger.

[youenn fablet]

Created attachment 364551 [details]
Updated according review comments

[EWS Watchlist]

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


ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:62:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
Total errors found: 1 in 31 files


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

[WebKit Commit Bot]

Comment on attachment 364551 [details]
Updated according review comments

Clearing flags on attachment: 364551

Committed r242911: <https://trac.webkit.org/changeset/242911>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/48864151>

[Shawn Roberts]

http/tests/security/cross-origin-indexeddb.html imported/blink/fast/text/output-isolate-at-end-of-line-crash.html legacy-animation-engine/imported/blink/fast/text/output-isolate-at-end-of-line-crash.html is crashing on  Mac and iOS Sim Debug bots.

Crashing started to happen after this revision.

I cannot repro locally. 

Dashboard : 

https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fsecurity%2Fcross-origin-indexeddb.html%20imported%2Fblink%2Ffast%2Ftext%2Foutput-isolate-at-end-of-line-crash.html%20legacy-animation-engine%2Fimported%2Fblink%2Ffast%2Ftext%2Foutput-isolate-at-end-of-line-crash.html

Crash log : 

https://build.webkit.org/results/Apple%20Mojave%20Debug%20WK2%20(Tests)/r243285%20(2035)/http/tests/security/cross-origin-worker-indexeddb-crash-log.txt