Bug 192556 – IndexedDB: quota should be able to increase

• Status: RESOLVED
• Resolution: INVALID
• Component: New Bugs

[Sihui Liu]

Instead of using a fixed high storage limit, we should set a small initial quota, and let the user increase it later.

[Sihui Liu]

Created attachment 356969 [details]
Patch

[Sihui Liu]

Created attachment 356991 [details]
Patch

[Sihui Liu]

Created attachment 356997 [details]
Patch

[Chris Dumez]

Comment on attachment 356991 [details]
Patch

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

> Source/WebCore/ChangeLog:8
> +        Instead of setting a fixed high storage limit, now we have a small initail IDB quota and 

typo: initail

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:542
> +    if (is<Document>(*scriptExecutionContext())) {

How do we know scriptExecutionContext() cannot return null?

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:543
> +        Document& document = downcast<Document>(*scriptExecutionContext());

auto&

> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:544
> +        if (Page* page = document.page())

auto*

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:114
> +    if (!SQLiteFileSystem::ensureDatabaseDirectoryExists(m_databaseDirectoryPath)) {

ASSERT(!isMainThread()); ?

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:138
> +    if (m_quotaDatabase.tableExists("Quota"))

"Quota"_s

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:141
> +    if (!m_quotaDatabase.executeCommand("CREATE TABLE Quota (origin TEXT UNIQUE ON CONFLICT REPLACE, quota INTEGER NOT NULL ON CONFLICT FAIL);"))

String needs _s suffix.

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:155
> +    SQLiteStatement statement(m_quotaDatabase, "SELECT quota FROM Quota where origin=?;");

_s suffix

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:181
> +    SQLiteStatement statement(m_quotaDatabase, "INSERT OR REPLACE INTO Quota VALUES (?, ?)");

_s suffix

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:202
> +    SQLiteStatement statement(m_quotaDatabase, "DELETE FROM Quota WHERE origin LIKE ?");

_s suffix

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:220
> +    SQLiteStatement statement(m_quotaDatabase, "SELECT quota FROM Quota");

_s suffix

> Source/WebCore/Modules/indexeddb/server/IDBServer.h:52
> +const uint64_t defaultPerOriginQuota = 5 * MB;

5 may be small. I would have done 50 do avoid prompting too much.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.h:46
> +const uint64_t QuotaIncreaseSize = 5 * MB;

Seems odd that the default quota is duplicated in several places.

> Source/WebCore/Modules/indexeddb/shared/IDBError.h:58
> +    IDBQuotaDetails quotaDetails() const { return m_quotaDetails; };

could return a const IDBQuotaDetails&.

> Source/WebCore/page/SecurityOriginData.cpp:128
> +        return fromDatabaseIdentifier(originIdentifier.substring(0, seperator));

Typo: seperator

> Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.mm:694
> +    WebSecurityOrigin *webOrigin = [[WebSecurityOrigin alloc] _initWithWebCoreSecurityOrigin:&frame.document()->securityOrigin()];

Can we use adoptNS() instead of explicit release?

> LayoutTests/storage/indexeddb/open-twice-workers.html:12
> +    window.testRunner.clearAllDatabases();

Why is this needed? WebKitTestRunner / DumpRenderTree should do cleanup between tests. It is bad practices to rely on tests to clean up things.

[EWS Watchlist]

Comment on attachment 356997 [details]
Patch

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

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 357001 [details]
Archive of layout-test-results from ews103 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 356997 [details]
Patch

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

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 357003 [details]
Archive of layout-test-results from ews107 for mac-sierra-wk2

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

[Sihui Liu]

Comment on attachment 356991 [details]
Patch

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

>> Source/WebCore/ChangeLog:8
>> +        Instead of setting a fixed high storage limit, now we have a small initail IDB quota and 
> 
> typo: initail

Okay.

>> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:542
>> +    if (is<Document>(*scriptExecutionContext())) {
> 
> How do we know scriptExecutionContext() cannot return null?

Seems we cannot. Add scriptExecutionContext() condition here.

>> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:543
>> +        Document& document = downcast<Document>(*scriptExecutionContext());
> 
> auto&

Okay.

>> Source/WebCore/Modules/indexeddb/IDBRequest.cpp:544
>> +        if (Page* page = document.page())
> 
> auto*

Okay.

>> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:114
>> +    if (!SQLiteFileSystem::ensureDatabaseDirectoryExists(m_databaseDirectoryPath)) {
> 
> ASSERT(!isMainThread()); ?

Added.

>> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:141
>> +    if (!m_quotaDatabase.executeCommand("CREATE TABLE Quota (origin TEXT UNIQUE ON CONFLICT REPLACE, quota INTEGER NOT NULL ON CONFLICT FAIL);"))
> 
> String needs _s suffix.

Added to all.

>> Source/WebCore/Modules/indexeddb/server/IDBServer.h:52
>> +const uint64_t defaultPerOriginQuota = 5 * MB;
> 
> 5 may be small. I would have done 50 do avoid prompting too much.

Okay.

>> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.h:46
>> +const uint64_t QuotaIncreaseSize = 5 * MB;
> 
> Seems odd that the default quota is duplicated in several places.

This is not the default quota, it's used to make sure expected database usage is bigger than current database size, as what you did for web database. 
It's used when returning the QuotaExceededError from database, so I didn't put it in idbserver.

>> Source/WebCore/Modules/indexeddb/shared/IDBError.h:58
>> +    IDBQuotaDetails quotaDetails() const { return m_quotaDetails; };
> 
> could return a const IDBQuotaDetails&.

Okay.

>> Source/WebCore/page/SecurityOriginData.cpp:128
>> +        return fromDatabaseIdentifier(originIdentifier.substring(0, seperator));
> 
> Typo: seperator

Nice catch!

>> Source/WebKitLegacy/mac/WebCoreSupport/WebChromeClient.mm:694
>> +    WebSecurityOrigin *webOrigin = [[WebSecurityOrigin alloc] _initWithWebCoreSecurityOrigin:&frame.document()->securityOrigin()];
> 
> Can we use adoptNS() instead of explicit release?

Okay.

>> LayoutTests/storage/indexeddb/open-twice-workers.html:12
>> +    window.testRunner.clearAllDatabases();
> 
> Why is this needed? WebKitTestRunner / DumpRenderTree should do cleanup between tests. It is bad practices to rely on tests to clean up things.

It seems WebKitTestRunner / DumpRenderTree doesn't do the work well, because tests are failing for QuotaExceededError after I set the default quota to 5 MB. This means database files from previous tests are left on disk and use up the quota. I will check WebKitTestRunner / DumpRenderTree.

[Sihui Liu]

Created attachment 357088 [details]
Patch

[Sihui Liu]

Created attachment 357089 [details]
Patch

[Sihui Liu]

Created attachment 357092 [details]
Patch

[EWS Watchlist]

Comment on attachment 357092 [details]
Patch

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

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 357095 [details]
Archive of layout-test-results from ews101 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 357092 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/IndexedDB/idbcursor_advance_index2.htm

[EWS Watchlist]

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

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

[EWS Watchlist]

Comment on attachment 357092 [details]
Patch

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

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 357097 [details]
Archive of layout-test-results from ews117 for mac-sierra

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

[EWS Watchlist]

Comment on attachment 357092 [details]
Patch

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

New failing tests:
imported/w3c/web-platform-tests/webrtc/simplecall-no-ssrcs.https.html
imported/w3c/web-platform-tests/IndexedDB/idbcursor-advance-invalid.htm
imported/w3c/web-platform-tests/IndexedDB/idbcursor-continuePrimaryKey-exceptions.htm

[EWS Watchlist]

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

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

[Geoffrey Garen]

Thread 3 Crashed:: IndexedDatabase Server
0   com.apple.WebCore             	0x00000001058b9ec3 WTF::CrossThreadQueue<WTF::CrossThreadTask>::tryGetMessage() + 51 (memory:2726)
1   com.apple.WebCore             	0x00000001059111a8 WebCore::IDBServer::UniqueIDBDatabase::executeNextDatabaseTask() + 24 (Optional.h:516)
2   com.apple.JavaScriptCore      	0x0000000109427cf8 WTF::CrossThreadTaskHandler::taskRunLoop() + 120 (memory:2733)
3   com.apple.JavaScriptCore      	0x0000000109467324 WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) + 212 (memory:2735)
4   com.apple.JavaScriptCore      	0x0000000109468ec9 WTF::wtfThreadEntryPoint(void*) + 9 (ThreadingPthreads.cpp:203)
5   libsystem_pthread.dylib       	0x00007fff9201093b _pthread_body + 180
6   libsystem_pthread.dylib       	0x00007fff92010887 _pthread_start + 286
7   libsystem_pthread.dylib       	0x00007fff9201008d thread_start + 13

My guess here is that UniqueIDBDatabase was deleted.

run-webkit-tests --guard-malloc --repeat-each=1000 imported/w3c/web-platform-tests/IndexedDB/idbcursor_advance_index2.htm might make this crash reproducible locally. Or you might need to run all the tests in the imported/w3c/web-platform-tests/IndexedDB/ directory, in case this crash depends on past state.

It would also be interesting to split out a patch just to make the change to TestController.cpp. I wonder if the change to start calling ClearIndexedDatabases() has exposed some pre-existing bugs. It would be nice to isolate whether this crash is new to your patch, or a pre-existing bug. That would tell us a lot about where start looking.

[Sihui Liu]

Will be solved in <rdar://problem/47403621>.

[Alex Christensen]

Comment on attachment 357092 [details]
Patch

This has been requesting review for more than one year.  If this is still needed, please rebase and re-request review.