Instead of using a fixed high storage limit, we should set a small initial quota, and let the user increase it later.
Created attachment 356969 [details] Patch
Created attachment 356991 [details] Patch
Created attachment 356997 [details] Patch
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.
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.
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
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.
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
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.
Created attachment 357088 [details] Patch
Created attachment 357089 [details] Patch
Created attachment 357092 [details] Patch
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.
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
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
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
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.
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
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
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
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.
Will be solved in <rdar://problem/47403621>.
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.