Bug 157843

When the IDBServer decides to close a database connection itself, a lot of cleanup has to take place. The server has to: - Error out all requests it knows about - Abort all transactions it knows about - Tell the connection that it is being closed - Wait for the connection to acknowledge that it was closed on the server The client has to: - Error out all requests it hasn't sent to the server - Abort all transactions that haven't already been aborted by the server - Send acknowledgement to the server that it has been closed. Additionally, because of the asynchronous nature of requests being "in flight" between the client and server, a few design assumptions change, requiring changing some asserts and null checks to be graceful failures, etc etc.

Created attachment 279266 [details] Patch v1

Attachment 279266 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:488: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:510: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.h:101: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.h:134: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 279266 [details] Patch v1 View in context: https://bugs.webkit.org/attachment.cgi?id=279266&action=review > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:1087 > + Vector<RefPtr<IDBClient::TransactionOperation>> operations; > + operations.reserveInitialCapacity(m_transactionOperationMap.size()); > + for (auto& operation : m_transactionOperationMap.values()) > + operations.uncheckedAppend(operation); copyValuesToVector > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:495 > + // If the modification time is in the future, don't both doing anything. > + if (modificationTime > std::chrono::system_clock::now()) { Is this really a common case worth optimizing for? > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:519 > + for (auto* connection : m_databaseConnections.values()) { > + const auto& identifier = connection->database().identifier(); > + for (auto& origin : origins) { This could get slow if there are a lot of origins and connections. Is there any way to use a HashSet or something?

(In reply to comment #4) > Comment on attachment 279266 [details] > Patch v1 > > View in context: > https://bugs.webkit.org/attachment.cgi?id=279266&action=review > > > Source/WebCore/Modules/indexeddb/IDBTransaction.cpp:1087 > > + Vector<RefPtr<IDBClient::TransactionOperation>> operations; > > + operations.reserveInitialCapacity(m_transactionOperationMap.size()); > > + for (auto& operation : m_transactionOperationMap.values()) > > + operations.uncheckedAppend(operation); > > copyValuesToVector Yup. > > > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:495 > > + // If the modification time is in the future, don't both doing anything. > > + if (modificationTime > std::chrono::system_clock::now()) { > > Is this really a common case worth optimizing for? The commonality is indeterminate, since this is directly exposed as API. The optimization definitely seems worthwhile because skipping it would involve performing a whole bunch of i/o just to do nothing. > > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:519 > > + for (auto* connection : m_databaseConnections.values()) { > > + const auto& identifier = connection->database().identifier(); > > + for (auto& origin : origins) { > > This could get slow if there are a lot of origins and connections. Is there > any way to use a HashSet or something? The common case for known usage of this API is 1 origin. For 1 origin, the Vector impl is faster and more space efficient than a HashSet, even before allowing for the construction of the HashSet itself. I agree that if 1000's of origins came in the HashSet would be clearly superior, but in that case the entire operation will be very long running. Especially since we have no reason to believe that happens, combined with the fact that this is already not blocking the main thread of either the Web or UI process, I think that is a premature optimization.

The patch is obviously causing havoc with the imported w3c tests (which I hadn't run locally, as I always assume storage/indexeddb is a better suite anyways) Checking that locally...

(In reply to comment #6) > The patch is obviously causing havoc with the imported w3c tests (which I > hadn't run locally, as I always assume storage/indexeddb is a better suite > anyways) > > Checking that locally... Never mind, nothing to see here. Charge forward.

Created attachment 279278 [details] Patch for landing

Attachment 279278 [details] did not pass style-queue: ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:488: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:510: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.h:101: Extra space before ( in function call [whitespace/parens] [4] ERROR: Source/WebCore/Modules/indexeddb/server/IDBServer.h:134: Extra space before ( in function call [whitespace/parens] [4] Total errors found: 4 in 38 files If any of these errors are false positives, please file a bug against check-webkit-style.

Comment on attachment 279278 [details] Patch for landing Clearing flags on attachment: 279278 Committed r201098: <http://trac.webkit.org/changeset/201098>

All reviewed patches have been landed. Closing bug.