Attachment 346914 Details for Bug 188474 – Patch

[patch] Patch

bug-188474-20180810114927.patch (text/plain), 10.39 KB, created by Sihui Liu on 2018-08-10 11:49:28 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Sihui Liu

Created: 2018-08-10 11:49:28 PDT

Size: 10.39 KB

patch

obsolete

>Subversion Revision: 234733
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 994a0560fbed876db99b3c2467c2feac27e8ba24..6f4722606d1eba0f13d7b8ec2e15e16fd7782912 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,35 @@
>+2018-08-10  Sihui Liu  <sihui_liu@apple.com>
>+
>+        CrashTracer: com.apple.WebKit.Storage at WebCore::IDBServer::UniqueIDBDatabase::connectionClosedFromClient(WebCore::IDBServer::UniqueIDBDatabaseConnection&)
>+        https://bugs.webkit.org/show_bug.cgi?id=188474
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        UniqueIDBDatabaseConnection is refcounted by UniqueIDBDatabaseTransaction and it refcounts
>+        UniqueIDBDatabaseTransaction. This cycle could make UniqueIDBDatabaseConnection outlives
>+        UniqueIDBDatabase, so its reference to UniqueIDBDatabase may be stale. Calling a function
>+        on a stale object is probably the reason of recent various storage process crashes in
>+        indexedDB.
>+
>+        This patch makes m_database a WeakPtr and adds assertions that could help us debug the
>+        crashes.
>+
>+        * Modules/indexeddb/server/UniqueIDBDatabase.h:
>+        * Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp:
>+        (WebCore::IDBServer::UniqueIDBDatabaseConnection::UniqueIDBDatabaseConnection):
>+        (WebCore::IDBServer::UniqueIDBDatabaseConnection::~UniqueIDBDatabaseConnection):
>+        (WebCore::IDBServer::UniqueIDBDatabaseConnection::database):
>+        (WebCore::IDBServer::UniqueIDBDatabaseConnection::abortTransactionWithoutCallback):
>+        (WebCore::IDBServer::UniqueIDBDatabaseConnection::connectionClosedFromClient):
>+        (WebCore::IDBServer::UniqueIDBDatabaseConnection::confirmDidCloseFromServer):
>+        (WebCore::IDBServer::UniqueIDBDatabaseConnection::didFireVersionChangeEvent):
>+        (WebCore::IDBServer::UniqueIDBDatabaseConnection::didFinishHandlingVersionChange):
>+        (WebCore::IDBServer::UniqueIDBDatabaseConnection::createVersionChangeTransaction):
>+        (WebCore::IDBServer::UniqueIDBDatabaseConnection::establishTransaction):
>+        (WebCore::IDBServer::UniqueIDBDatabaseConnection::didAbortTransaction):
>+        * Modules/indexeddb/server/UniqueIDBDatabaseConnection.h:
>+        (WebCore::IDBServer::UniqueIDBDatabaseConnection::database): Deleted.
>+
> 2018-08-09  Saam Barati  <sbarati@apple.com>
> 
>         memoryFootprint should return size_t not optional<size_t>
>diff --git a/Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.h b/Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.h
>index 9a8b1c5d062ab4bd1f93a88569f27486d5193dc8..f3b8788fe877151fb8ad5a9eb1bd30bb70c15d04 100644
>--- a/Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.h
>+++ b/Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.h
>@@ -72,7 +72,7 @@ typedef Function<void(const IDBError&, const IDBGetResult&)> GetResultCallback;
> typedef Function<void(const IDBError&, const IDBGetAllResult&)> GetAllResultsCallback;
> typedef Function<void(const IDBError&, uint64_t)> CountCallback;
> 
>-class UniqueIDBDatabase {
>+class UniqueIDBDatabase : public CanMakeWeakPtr<UniqueIDBDatabase> {
> public:
>     UniqueIDBDatabase(IDBServer&, const IDBDatabaseIdentifier&);
>     UniqueIDBDatabase(UniqueIDBDatabase&) = delete;
>diff --git a/Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp b/Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp
>index 84015d86c4ca48f01bdd95ac4c44f63468c0aa0a..714bb2963566ae5e21fbbe4abba4c8bf3c51b5cc 100644
>--- a/Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp
>+++ b/Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.cpp
>@@ -44,20 +44,27 @@ Ref<UniqueIDBDatabaseConnection> UniqueIDBDatabaseConnection::create(UniqueIDBDa
> }
> 
> UniqueIDBDatabaseConnection::UniqueIDBDatabaseConnection(UniqueIDBDatabase& database, ServerOpenDBRequest& request)
>-    : m_database(database)
>+    : m_database(makeWeakPtr(database))
>     , m_connectionToClient(request.connection())
>     , m_openRequestIdentifier(request.requestData().requestIdentifier())
> {
>-    m_database.server().registerDatabaseConnection(*this);
>+    m_database->server().registerDatabaseConnection(*this);
>     m_connectionToClient.registerDatabaseConnection(*this);
> }
> 
> UniqueIDBDatabaseConnection::~UniqueIDBDatabaseConnection()
> {
>-    m_database.server().unregisterDatabaseConnection(*this);
>+    if (m_database)
>+        m_database->server().unregisterDatabaseConnection(*this);
>     m_connectionToClient.unregisterDatabaseConnection(*this);
> }
> 
>+UniqueIDBDatabase& UniqueIDBDatabaseConnection::database()
>+{
>+    ASSERT(m_database);
>+    return *m_database;
>+}
>+
> bool UniqueIDBDatabaseConnection::hasNonFinishedTransactions() const
> {
>     return !m_transactionMap.isEmpty();
>@@ -70,10 +77,13 @@ void UniqueIDBDatabaseConnection::abortTransactionWithoutCallback(UniqueIDBDatab
>     const auto& transactionIdentifier = transaction.info().identifier();
>     RefPtr<UniqueIDBDatabaseConnection> protectedThis(this);
> 
>-    m_database.abortTransaction(transaction, [this, protectedThis, transactionIdentifier](const IDBError&) {
>-        ASSERT(m_transactionMap.contains(transactionIdentifier));
>-        m_transactionMap.remove(transactionIdentifier);
>-    });
>+    ASSERT(m_database);
>+    if (m_database) {
>+        m_database->abortTransaction(transaction, [this, protectedThis, transactionIdentifier](const IDBError&) {
>+            ASSERT(m_transactionMap.contains(transactionIdentifier));
>+            m_transactionMap.remove(transactionIdentifier);
>+        });
>+    }
> }
> 
> void UniqueIDBDatabaseConnection::connectionPendingCloseFromClient()
>@@ -87,28 +97,36 @@ void UniqueIDBDatabaseConnection::connectionClosedFromClient()
> {
>     LOG(IndexedDB, "UniqueIDBDatabaseConnection::connectionClosedFromClient - %s - %" PRIu64, m_openRequestIdentifier.loggingString().utf8().data(), identifier());
> 
>-    m_database.connectionClosedFromClient(*this);
>+    ASSERT(m_database);
>+    if (m_database)
>+        m_database->connectionClosedFromClient(*this);
> }
> 
> void UniqueIDBDatabaseConnection::confirmDidCloseFromServer()
> {
>     LOG(IndexedDB, "UniqueIDBDatabaseConnection::confirmDidCloseFromServer - %s - %" PRIu64, m_openRequestIdentifier.loggingString().utf8().data(), identifier());
> 
>-    m_database.confirmDidCloseFromServer(*this);
>+    ASSERT(m_database);
>+    if (m_database)
>+        m_database->confirmDidCloseFromServer(*this);
> }
> 
> void UniqueIDBDatabaseConnection::didFireVersionChangeEvent(const IDBResourceIdentifier& requestIdentifier)
> {
>     LOG(IndexedDB, "UniqueIDBDatabaseConnection::didFireVersionChangeEvent - %s - %" PRIu64, m_openRequestIdentifier.loggingString().utf8().data(), identifier());
> 
>-    m_database.didFireVersionChangeEvent(*this, requestIdentifier);
>+    ASSERT(m_database);
>+    if (m_database)
>+        m_database->didFireVersionChangeEvent(*this, requestIdentifier);
> }
> 
> void UniqueIDBDatabaseConnection::didFinishHandlingVersionChange(const IDBResourceIdentifier& transactionIdentifier)
> {
>     LOG(IndexedDB, "UniqueIDBDatabaseConnection::didFinishHandlingVersionChange - %s - %" PRIu64, transactionIdentifier.loggingString().utf8().data(), identifier());
> 
>-    m_database.didFinishHandlingVersionChange(*this, transactionIdentifier);
>+    ASSERT(m_database);
>+    if (m_database)
>+        m_database->didFinishHandlingVersionChange(*this, transactionIdentifier);
> }
> 
> void UniqueIDBDatabaseConnection::fireVersionChangeEvent(const IDBResourceIdentifier& requestIdentifier, uint64_t requestedVersion)
>@@ -122,7 +140,7 @@ UniqueIDBDatabaseTransaction& UniqueIDBDatabaseConnection::createVersionChangeTr
>     LOG(IndexedDB, "UniqueIDBDatabaseConnection::createVersionChangeTransaction - %s - %" PRIu64, m_openRequestIdentifier.loggingString().utf8().data(), identifier());
>     ASSERT(!m_closePending);
> 
>-    IDBTransactionInfo info = IDBTransactionInfo::versionChange(m_connectionToClient, m_database.info(), newVersion);
>+    IDBTransactionInfo info = IDBTransactionInfo::versionChange(m_connectionToClient, m_database->info(), newVersion);
> 
>     Ref<UniqueIDBDatabaseTransaction> transaction = UniqueIDBDatabaseTransaction::create(*this, info);
>     m_transactionMap.set(transaction->info().identifier(), &transaction.get());
>@@ -142,7 +160,9 @@ void UniqueIDBDatabaseConnection::establishTransaction(const IDBTransactionInfo&
> 
>     Ref<UniqueIDBDatabaseTransaction> transaction = UniqueIDBDatabaseTransaction::create(*this, info);
>     m_transactionMap.set(transaction->info().identifier(), &transaction.get());
>-    m_database.enqueueTransaction(WTFMove(transaction));
>+
>+    ASSERT(m_database);
>+    m_database->enqueueTransaction(WTFMove(transaction));
> }
> 
> void UniqueIDBDatabaseConnection::didAbortTransaction(UniqueIDBDatabaseTransaction& transaction, const IDBError& error)
>@@ -152,7 +172,8 @@ void UniqueIDBDatabaseConnection::didAbortTransaction(UniqueIDBDatabaseTransacti
>     auto transactionIdentifier = transaction.info().identifier();
>     auto takenTransaction = m_transactionMap.take(transactionIdentifier);
> 
>-    ASSERT(takenTransaction || m_database.hardClosedForUserDelete());
>+    ASSERT(m_database);
>+    ASSERT(takenTransaction || m_database->hardClosedForUserDelete());
>     if (takenTransaction)
>         m_connectionToClient.didAbortTransaction(transactionIdentifier, error);
> }
>diff --git a/Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.h b/Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.h
>index 2f1a29a2e88e52589e581ca921f69945bc239393..06bfe2b64995669ef58dc8882535be3206458aa9 100644
>--- a/Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.h
>+++ b/Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabaseConnection.h
>@@ -32,6 +32,7 @@
> #include <wtf/Identified.h>
> #include <wtf/Ref.h>
> #include <wtf/RefCounted.h>
>+#include <wtf/WeakPtr.h>
> 
> namespace WebCore {
> 
>@@ -52,7 +53,7 @@ public:
>     ~UniqueIDBDatabaseConnection();
> 
>     const IDBResourceIdentifier& openRequestIdentifier() { return m_openRequestIdentifier; }
>-    UniqueIDBDatabase& database() { return m_database; }
>+    UniqueIDBDatabase& database();
>     IDBConnectionToClient& connectionToClient() { return m_connectionToClient; }
> 
>     void connectionPendingCloseFromClient();
>@@ -86,7 +87,7 @@ public:
> private:
>     UniqueIDBDatabaseConnection(UniqueIDBDatabase&, ServerOpenDBRequest&);
> 
>-    UniqueIDBDatabase& m_database;
>+    WeakPtr<UniqueIDBDatabase> m_database;
>     IDBConnectionToClient& m_connectionToClient;
>     IDBResourceIdentifier m_openRequestIdentifier;
>

Actions: View | Formatted Diff | Diff

Attachments on bug 188474: 346914 | 346923 | 346932 | 346939 | 346940