Changeset 244027 in webkit

Timestamp:

Apr 8, 2019, 10:29:51 AM (7 years ago)

Author:

youenn@apple.com

Message:

Make sure UniqueIDBDatabaseConnection unregisters itself even if its database is gone
​https://bugs.webkit.org/show_bug.cgi?id=196651

Reviewed by Brady Eidson.

In UniqueIDBDatabase methods, many operations are refing the transaction
so that it stays alive until a quota check decision is made.
This extends the lifetime of the transaction which may be lasting
longer than its database that may be cleared without waiting for the quota check decisions.

We therefore need to make sure that the transaction is cleaning itself correctly at destruction time.

Make sure that the transaction is unregistering itself from its IDBServer.
To do so, the transaction keeps a weak ref to the IDBServer.

This is timing sensitive hence difficult to test.

• Modules/indexeddb/server/IDBServer.h:
• Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp:

(WebCore::IDBServer::UniqueIDBDatabaseConnection::didAbortTransaction):
Like done below for UniqueIDBDatabaseConnection::didCommitTransaction,
add a check to ensure that either the database is we are in an error case.

• Modules/indexeddb/server/UniqueIDBDatabaseTransaction.cpp:

(WebCore::IDBServer::UniqueIDBDatabaseTransaction::UniqueIDBDatabaseTransaction):
(WebCore::IDBServer::UniqueIDBDatabaseTransaction::~UniqueIDBDatabaseTransaction):

• Modules/indexeddb/server/UniqueIDBDatabaseTransaction.h:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• Modules/indexeddb/server/IDBServer.h (modified) (2 diffs)
• Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp (modified) (1 diff)
• Modules/indexeddb/server/UniqueIDBDatabaseTransaction.cpp (modified) (1 diff)
• Modules/indexeddb/server/UniqueIDBDatabaseTransaction.h (modified) (2 diffs)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+2019-04-08  Youenn Fablet  <youenn@apple.com>
+
+        Make sure UniqueIDBDatabaseConnection unregisters itself even if its database is gone
+        https://bugs.webkit.org/show_bug.cgi?id=196651
+
+        Reviewed by Brady Eidson.
+
+        In UniqueIDBDatabase methods, many operations are refing the transaction
+        so that it stays alive until a quota check decision is made.
+        This extends the lifetime of the transaction which may be lasting
+        longer than its database that may be cleared without waiting for the quota check decisions.
+
+        We therefore need to make sure that the transaction is cleaning itself correctly at destruction time.
+
+        Make sure that the transaction is unregistering itself from its IDBServer.
+        To do so, the transaction keeps a weak ref to the IDBServer.
+
+        This is timing sensitive hence difficult to test.
+
+        * Modules/indexeddb/server/IDBServer.h:
+        * Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp:
+        (WebCore::IDBServer::UniqueIDBDatabaseConnection::didAbortTransaction):
+        Like done below for UniqueIDBDatabaseConnection::didCommitTransaction,
+        add a check to ensure that either the database is we are in an error case.
+        * Modules/indexeddb/server/UniqueIDBDatabaseTransaction.cpp:
+        (WebCore::IDBServer::UniqueIDBDatabaseTransaction::UniqueIDBDatabaseTransaction):
+        (WebCore::IDBServer::UniqueIDBDatabaseTransaction::~UniqueIDBDatabaseTransaction):
+        * Modules/indexeddb/server/UniqueIDBDatabaseTransaction.h:
+
 2019-04-08  Christopher Reid  <chris.reid@sony.com>
 

trunk/Source/WebCore/Modules/indexeddb/server/IDBServer.h

@@ -42 +42 @@
 #include <wtf/RefCounted.h>
 #include <wtf/RefPtr.h>
+#include <wtf/WeakPtr.h>
 
 namespace WebCore {
@@ -58 +59 @@
 class IDBBackingStoreTemporaryFileHandler;
 
-class IDBServer : public RefCounted<IDBServer>, public CrossThreadTaskHandler {
+class IDBServer : public RefCounted<IDBServer>, public CrossThreadTaskHandler, public CanMakeWeakPtr<IDBServer> {
 public:
     using QuotaManagerGetter = WTF::Function<StorageQuotaManager*(PAL::SessionID, const ClientOrigin&)>;

trunk/Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp

@@ -170 +170 @@
     auto takenTransaction = m_transactionMap.take(transactionIdentifier);
 
-    ASSERT(m_database);
-    ASSERT(takenTransaction || m_database->hardClosedForUserDelete());
+    ASSERT(takenTransaction || (!m_database && !error.isNull()) || (m_database && m_database->hardClosedForUserDelete()));
     if (takenTransaction)
         m_connectionToClient->didAbortTransaction(transactionIdentifier, error);

trunk/Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseTransaction.cpp

@@ -54 +54 @@
         m_originalDatabaseInfo = std::make_unique<IDBDatabaseInfo>(database->info());
 
-    database->server().registerTransaction(*this);
+    auto& server = database->server();
+    m_server = makeWeakPtr(server);
+    server.registerTransaction(*this);
 }
 
 UniqueIDBDatabaseTransaction::~UniqueIDBDatabaseTransaction()
 {
-    auto database = m_databaseConnection->database();
-    if (!database)
-        return;
-
-    database->transactionDestroyed(*this);
-    database->server().unregisterTransaction(*this);
+    if (auto database = m_databaseConnection->database())
+        database->transactionDestroyed(*this);
+
+    if (m_server)
+        m_server->unregisterTransaction(*this);
 }
 

trunk/Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseTransaction.h

@@ -51 +51 @@
 namespace IDBServer {
 
+class IDBServer;
 class UniqueIDBDatabaseConnection;
 
@@ -94 +95 @@
     Ref<UniqueIDBDatabaseConnection> m_databaseConnection;
     IDBTransactionInfo m_transactionInfo;
+    WeakPtr<IDBServer> m_server;
 
     std::unique_ptr<IDBDatabaseInfo> m_originalDatabaseInfo;