[Mac WK2] storage/indexeddb/IDBObject-leak.html is flaky

https://bugs.webkit.org/show_bug.cgi?id=195036

Reviewed by Geoffrey Garen.

When connection to IDBServer is lost, IDBDatabase in web process should not only stop active transactions, but
also transactions in committing process.

Also, TransactionOpration should clear its perform function when the operation is being completed, otherwise
there is a reference cycle of TransactionOpration.

Covered by existing tests storage/indexeddb/IDBObject-leak.html.

* Modules/indexeddb/IDBDatabase.cpp:
(WebCore::IDBDatabase::connectionToServerLost): notify committing transasctions that connection is lost.
* Modules/indexeddb/IDBTransaction.cpp:
(WebCore::IDBTransaction::connectionClosedFromServer): notify IDBConnectionProxy that transaction ends.
* Modules/indexeddb/client/IDBConnectionProxy.cpp:
(WebCore::IDBClient::IDBConnectionProxy::forgetTransaction): clear finished transactions.
* Modules/indexeddb/client/IDBConnectionProxy.h:
* Modules/indexeddb/client/TransactionOperation.h:
(WebCore::IDBClient::TransactionOperation::doComplete): clear perform function unconditionally when the
operation is in completion process.


Canonical link: https://commits.webkit.org/209432@main
git-svn-id: https://svn.webkit.org/repository/webkit/trunk@242110 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog

@@ -1,3 +1,29 @@
+2019-02-26  Sihui Liu  <sihui_liu@apple.com>
+
+        [Mac WK2] storage/indexeddb/IDBObject-leak.html is flaky
+        https://bugs.webkit.org/show_bug.cgi?id=195036
+
+        Reviewed by Geoffrey Garen.
+
+        When connection to IDBServer is lost, IDBDatabase in web process should not only stop active transactions, but 
+        also transactions in committing process.
+
+        Also, TransactionOpration should clear its perform function when the operation is being completed, otherwise 
+        there is a reference cycle of TransactionOpration.
+
+        Covered by existing tests storage/indexeddb/IDBObject-leak.html.
+
+        * Modules/indexeddb/IDBDatabase.cpp:
+        (WebCore::IDBDatabase::connectionToServerLost): notify committing transasctions that connection is lost.
+        * Modules/indexeddb/IDBTransaction.cpp:
+        (WebCore::IDBTransaction::connectionClosedFromServer): notify IDBConnectionProxy that transaction ends.
+        * Modules/indexeddb/client/IDBConnectionProxy.cpp:
+        (WebCore::IDBClient::IDBConnectionProxy::forgetTransaction): clear finished transactions.
+        * Modules/indexeddb/client/IDBConnectionProxy.h:
+        * Modules/indexeddb/client/TransactionOperation.h:
+        (WebCore::IDBClient::TransactionOperation::doComplete): clear perform function unconditionally when the 
+        operation is in completion process. 
+
 2019-02-26  Zalan Bujtas  <zalan@apple.com>
 
         [ContentChangeObserver] clearContentChangeObservers should be internal to ContentChangeObserver class

Source/WebCore/Modules/indexeddb/IDBDatabase.cpp

@@ -264,8 +264,12 @@ void IDBDatabase::connectionToServerLost(const IDBError& error)
     m_closePending = true;
     m_closedInServer = true;
 
-    auto transactions = copyToVector(m_activeTransactions.values());
-    for (auto& transaction : transactions)
+    auto activeTransactions = copyToVector(m_activeTransactions.values());
+    for (auto& transaction : activeTransactions)
+        transaction->connectionClosedFromServer(error);
+
+    auto committingTransactions = copyToVector(m_committingTransactions.values());
+    for (auto& transaction : committingTransactions)
         transaction->connectionClosedFromServer(error);
 
     auto errorEvent = Event::create(m_eventNames.errorEvent, Event::CanBubble::Yes, Event::IsCancelable::No);

Source/WebCore/Modules/indexeddb/IDBTransaction.cpp

@@ -1440,7 +1440,7 @@ void IDBTransaction::connectionClosedFromServer(const IDBError& error)
     LOG(IndexedDB, "IDBTransaction::connectionClosedFromServer - %s", error.message().utf8().data());
 
     m_database->willAbortTransaction(*this);
-    transitionedToFinishing(IndexedDB::TransactionState::Aborting);
+    m_state = IndexedDB::TransactionState::Aborting;
 
     abortInProgressOperations(error);
 
@@ -1454,6 +1454,7 @@ void IDBTransaction::connectionClosedFromServer(const IDBError& error)
     m_currentlyCompletingRequest = nullptr;
 
     connectionProxy().forgetActiveOperations(operations);
+    connectionProxy().forgetTransaction(*this);
 
     m_pendingTransactionOperationQueue.clear();
     m_abortQueue.clear();

Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp

@@ -517,6 +517,15 @@ void IDBConnectionProxy::forgetActiveOperations(const Vector<RefPtr<TransactionO
         m_activeOperations.remove(operation->identifier());
 }
 
+void IDBConnectionProxy::forgetTransaction(IDBTransaction& transaction)
+{
+    Locker<Lock> locker(m_transactionMapLock);
+
+    m_pendingTransactions.remove(transaction.info().identifier());
+    m_committingTransactions.remove(transaction.info().identifier());
+    m_abortingTransactions.remove(transaction.info().identifier());
+}
+
 template<typename KeyType, typename ValueType>
 void removeItemsMatchingCurrentThread(HashMap<KeyType, ValueType>& map)
 {

Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.h

@@ -122,6 +122,7 @@ class IDBConnectionProxy {
     void unregisterDatabaseConnection(IDBDatabase&);
 
     void forgetActiveOperations(const Vector<RefPtr<TransactionOperation>>&);
+    void forgetTransaction(IDBTransaction&);
     void forgetActivityForCurrentThread();
 
 private:

Source/WebCore/Modules/indexeddb/client/TransactionOperation.h

@@ -85,6 +85,9 @@ class TransactionOperation : public ThreadSafeRefCounted<TransactionOperation> {
     {
         ASSERT(m_originThread.ptr() == &Thread::current());
 
+        if (m_performFunction)
+            m_performFunction = { };
+
         // Due to race conditions between the server sending an "operation complete" message and the client
         // forcefully aborting an operation, it's unavoidable that this method might be called twice.
         // It's okay to handle that gracefully with an early return.
@@ -98,8 +101,6 @@ class TransactionOperation : public ThreadSafeRefCounted<TransactionOperation> {
         // so we need to do this trick to null it out without first destroying it.
         Function<void(const IDBResultData&)> oldCompleteFunction;
         std::swap(m_completeFunction, oldCompleteFunction);
-
-        m_performFunction = { };
     }
 
     const IDBResourceIdentifier& identifier() const { return m_identifier; }