Crash under WebCore::SWServer::registrationStoreImportComplete()
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Jun 2018 16:16:34 +0000 (16:16 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 15 Jun 2018 16:16:34 +0000 (16:16 +0000)
https://bugs.webkit.org/show_bug.cgi?id=186644
<rdar://problem/40982257>

Reviewed by Brady Eidson.

Fix lifetime management issues with RegistrationDatabase. RegistrationDatabase
was previously subclassing CrossThreadTaskHandler. CrossThreadTaskHandler
currently is not safe for objects that can get destroyed (such as
RegistrationDatabase). This is because it does not keep the object alive
when going to the background thread or back to the main thread. This would
cause crashes such as the one in the radar.

To address the issue, stop subclassing CrossThreadTaskHandler and use a
simple WorkQueue instead. RegistrationDatabase is now ThreadSafeRefCounted
and we take care of ref'ing it whenever we dispatch a task to the work queue
or back to the main thread. Because the RegistrationDatabase can now outlive
the RegistrationStore, m_store is now a WeakPtr.

* workers/service/server/RegistrationDatabase.cpp:
(WebCore::RegistrationDatabase::RegistrationDatabase):
(WebCore::RegistrationDatabase::~RegistrationDatabase):
(WebCore::RegistrationDatabase::postTaskToWorkQueue):
(WebCore::RegistrationDatabase::openSQLiteDatabase):
(WebCore::RegistrationDatabase::importRecordsIfNecessary):
(WebCore::RegistrationDatabase::pushChanges):
(WebCore::RegistrationDatabase::clearAll):
(WebCore::RegistrationDatabase::importRecords):
(WebCore::RegistrationDatabase::addRegistrationToStore):
(WebCore::RegistrationDatabase::databaseFailedToOpen):
(WebCore::RegistrationDatabase::databaseOpenedAndRecordsImported):
* workers/service/server/RegistrationDatabase.h:
(WebCore::RegistrationDatabase::create):
* workers/service/server/RegistrationStore.cpp:
(WebCore::RegistrationStore::RegistrationStore):
(WebCore::RegistrationStore::~RegistrationStore):
(WebCore::RegistrationStore::pushChangesToDatabase):
(WebCore::RegistrationStore::clearAll):
* workers/service/server/RegistrationStore.h:

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@232874 268f45cc-cd09-0410-ab3c-d52691b4dbfc

Source/WebCore/ChangeLog
Source/WebCore/workers/service/server/RegistrationDatabase.cpp
Source/WebCore/workers/service/server/RegistrationDatabase.h
Source/WebCore/workers/service/server/RegistrationStore.cpp
Source/WebCore/workers/service/server/RegistrationStore.h

index 44553ce..056130b 100644 (file)
@@ -1,3 +1,45 @@
+2018-06-15  Chris Dumez  <cdumez@apple.com>
+
+        Crash under WebCore::SWServer::registrationStoreImportComplete()
+        https://bugs.webkit.org/show_bug.cgi?id=186644
+        <rdar://problem/40982257>
+
+        Reviewed by Brady Eidson.
+
+        Fix lifetime management issues with RegistrationDatabase. RegistrationDatabase
+        was previously subclassing CrossThreadTaskHandler. CrossThreadTaskHandler
+        currently is not safe for objects that can get destroyed (such as
+        RegistrationDatabase). This is because it does not keep the object alive
+        when going to the background thread or back to the main thread. This would
+        cause crashes such as the one in the radar.
+
+        To address the issue, stop subclassing CrossThreadTaskHandler and use a
+        simple WorkQueue instead. RegistrationDatabase is now ThreadSafeRefCounted
+        and we take care of ref'ing it whenever we dispatch a task to the work queue
+        or back to the main thread. Because the RegistrationDatabase can now outlive
+        the RegistrationStore, m_store is now a WeakPtr.
+
+        * workers/service/server/RegistrationDatabase.cpp:
+        (WebCore::RegistrationDatabase::RegistrationDatabase):
+        (WebCore::RegistrationDatabase::~RegistrationDatabase):
+        (WebCore::RegistrationDatabase::postTaskToWorkQueue):
+        (WebCore::RegistrationDatabase::openSQLiteDatabase):
+        (WebCore::RegistrationDatabase::importRecordsIfNecessary):
+        (WebCore::RegistrationDatabase::pushChanges):
+        (WebCore::RegistrationDatabase::clearAll):
+        (WebCore::RegistrationDatabase::importRecords):
+        (WebCore::RegistrationDatabase::addRegistrationToStore):
+        (WebCore::RegistrationDatabase::databaseFailedToOpen):
+        (WebCore::RegistrationDatabase::databaseOpenedAndRecordsImported):
+        * workers/service/server/RegistrationDatabase.h:
+        (WebCore::RegistrationDatabase::create):
+        * workers/service/server/RegistrationStore.cpp:
+        (WebCore::RegistrationStore::RegistrationStore):
+        (WebCore::RegistrationStore::~RegistrationStore):
+        (WebCore::RegistrationStore::pushChangesToDatabase):
+        (WebCore::RegistrationStore::clearAll):
+        * workers/service/server/RegistrationStore.h:
+
 2018-06-15  Zalan Bujtas  <zalan@apple.com>
 
         [LFC] Align compute functions styles.
index 7d19f51..7749b88 100644 (file)
@@ -38,6 +38,7 @@
 #include "SWServer.h"
 #include "SecurityOrigin.h"
 #include <wtf/CompletionHandler.h>
+#include <wtf/CrossThreadCopier.h>
 #include <wtf/MainThread.h>
 #include <wtf/NeverDestroyed.h>
 #include <wtf/Scope.h>
@@ -91,23 +92,34 @@ static inline void cleanOldDatabases(const String& databaseDirectory)
         SQLiteFileSystem::deleteDatabaseFile(FileSystem::pathByAppendingComponent(databaseDirectory, databaseFilenameFromVersion(version)));
 }
 
-RegistrationDatabase::RegistrationDatabase(RegistrationStore& store, const String& databaseDirectory)
-    : CrossThreadTaskHandler("ServiceWorker I/O Thread")
-    , m_store(store)
-    , m_databaseDirectory(databaseDirectory)
+RegistrationDatabase::RegistrationDatabase(RegistrationStore& store, String&& databaseDirectory)
+    : m_workQueue(WorkQueue::create("ServiceWorker I/O Thread", WorkQueue::Type::Serial))
+    , m_store(makeWeakPtr(store))
+    , m_session(m_store->server().sessionID())
+    , m_databaseDirectory(WTFMove(databaseDirectory))
     , m_databaseFilePath(FileSystem::pathByAppendingComponent(m_databaseDirectory, databaseFilename()))
 {
     ASSERT(isMainThread());
 
-    postTask(CrossThreadTask([this] {
+    postTaskToWorkQueue([this] {
         importRecordsIfNecessary();
-    }));
+    });
 }
 
 RegistrationDatabase::~RegistrationDatabase()
 {
-    ASSERT(!m_database);
     ASSERT(isMainThread());
+
+    // The database needs to be destroyed on the background thread.
+    if (m_database)
+        m_workQueue->dispatch([database = WTFMove(m_database)] { });
+}
+
+void RegistrationDatabase::postTaskToWorkQueue(Function<void()>&& task)
+{
+    m_workQueue->dispatch([protectedThis = makeRef(*this), task = WTFMove(task)]() mutable {
+        task();
+    });
 }
 
 void RegistrationDatabase::openSQLiteDatabase(const String& fullFilename)
@@ -130,7 +142,9 @@ void RegistrationDatabase::openSQLiteDatabase(const String& fullFilename)
 #endif
 
         m_database = nullptr;
-        postTaskReply(createCrossThreadTask(*this, &RegistrationDatabase::databaseFailedToOpen));
+        callOnMainThread([protectedThis = makeRef(*this)] {
+            protectedThis->databaseFailedToOpen();
+        });
     });
 
     SQLiteFileSystem::ensureDatabaseDirectoryExists(m_databaseDirectory);
@@ -140,6 +154,11 @@ void RegistrationDatabase::openSQLiteDatabase(const String& fullFilename)
         errorMessage = "Failed to open registration database";
         return;
     }
+
+    // Disable threading checks. We always access the database from our serial WorkQueue. Such accesses
+    // are safe since work queue tasks are guaranteed to run one after another. However, tasks will not
+    // necessary run on the same thread every time (as per GCD documentation).
+    m_database->disableThreadingChecks();
     
     errorMessage = ensureValidRecordsTable();
     if (!errorMessage.isNull())
@@ -159,7 +178,9 @@ void RegistrationDatabase::importRecordsIfNecessary()
     if (FileSystem::fileExists(m_databaseFilePath))
         openSQLiteDatabase(m_databaseFilePath);
 
-    postTaskReply(createCrossThreadTask(*this, &RegistrationDatabase::databaseOpenedAndRecordsImported));
+    callOnMainThread([protectedThis = makeRef(*this)] {
+        protectedThis->databaseOpenedAndRecordsImported();
+    });
 }
 
 String RegistrationDatabase::ensureValidRecordsTable()
@@ -249,32 +270,32 @@ static std::optional<WorkerType> stringToWorkerType(const String& type)
     return std::nullopt;
 }
 
-void RegistrationDatabase::pushChanges(Vector<ServiceWorkerContextData>&& datas, WTF::CompletionHandler<void()>&& completionHandler)
+void RegistrationDatabase::pushChanges(Vector<ServiceWorkerContextData>&& datas, CompletionHandler<void()>&& completionHandler)
 {
-    postTask(CrossThreadTask([this, datas = crossThreadCopy(datas), completionHandler = WTFMove(completionHandler)]() mutable {
+    postTaskToWorkQueue([this, datas = crossThreadCopy(datas), completionHandler = WTFMove(completionHandler)]() mutable {
         doPushChanges(WTFMove(datas));
 
         if (!completionHandler)
             return;
 
-        postTaskReply(CrossThreadTask([completionHandler = WTFMove(completionHandler)] {
+        callOnMainThread([completionHandler = WTFMove(completionHandler)] {
             completionHandler();
-        }));
-    }));
+        });
+    });
 }
 
-void RegistrationDatabase::clearAll(WTF::CompletionHandler<void()>&& completionHandler)
+void RegistrationDatabase::clearAll(CompletionHandler<void()>&& completionHandler)
 {
-    postTask(CrossThreadTask([this, completionHandler = WTFMove(completionHandler)]() mutable {
+    postTaskToWorkQueue([this, completionHandler = WTFMove(completionHandler)]() mutable {
         m_database = nullptr;
 
         SQLiteFileSystem::deleteDatabaseFile(m_databaseFilePath);
         SQLiteFileSystem::deleteEmptyDatabaseDirectory(m_databaseDirectory);
 
-        postTaskReply(CrossThreadTask([completionHandler = WTFMove(completionHandler)] {
+        callOnMainThread([completionHandler = WTFMove(completionHandler)] {
             completionHandler();
-        }));
-    }));
+        });
+    });
 }
 
 void RegistrationDatabase::doPushChanges(Vector<ServiceWorkerContextData>&& datas)
@@ -383,9 +404,11 @@ String RegistrationDatabase::importRecords()
         auto registrationIdentifier = generateObjectIdentifier<ServiceWorkerRegistrationIdentifierType>();
         auto serviceWorkerData = ServiceWorkerData { workerIdentifier, scriptURL, ServiceWorkerState::Activated, *workerType, registrationIdentifier };
         auto registration = ServiceWorkerRegistrationData { WTFMove(*key), registrationIdentifier, URL(originURL, scopePath), *updateViaCache, lastUpdateCheckTime, std::nullopt, std::nullopt, WTFMove(serviceWorkerData) };
-        auto contextData = ServiceWorkerContextData { std::nullopt, WTFMove(registration), workerIdentifier, WTFMove(script), WTFMove(contentSecurityPolicy), WTFMove(scriptURL), *workerType, m_store.server().sessionID(), true, WTFMove(scriptResourceMap) };
+        auto contextData = ServiceWorkerContextData { std::nullopt, WTFMove(registration), workerIdentifier, WTFMove(script), WTFMove(contentSecurityPolicy), WTFMove(scriptURL), *workerType, m_session, true, WTFMove(scriptResourceMap) };
 
-        postTaskReply(createCrossThreadTask(*this, &RegistrationDatabase::addRegistrationToStore, WTFMove(contextData)));
+        callOnMainThread([protectedThis = makeRef(*this), contextData = contextData.isolatedCopy()]() mutable {
+            protectedThis->addRegistrationToStore(WTFMove(contextData));
+        });
     }
 
     if (result != SQLITE_DONE)
@@ -396,17 +419,20 @@ String RegistrationDatabase::importRecords()
 
 void RegistrationDatabase::addRegistrationToStore(ServiceWorkerContextData&& context)
 {
-    m_store.addRegistrationFromDatabase(WTFMove(context));
+    if (m_store)
+        m_store->addRegistrationFromDatabase(WTFMove(context));
 }
 
 void RegistrationDatabase::databaseFailedToOpen()
 {
-    m_store.databaseFailedToOpen();
+    if (m_store)
+        m_store->databaseFailedToOpen();
 }
 
 void RegistrationDatabase::databaseOpenedAndRecordsImported()
 {
-    m_store.databaseOpenedAndRecordsImported();
+    if (m_store)
+        m_store->databaseOpenedAndRecordsImported();
 }
 
 } // namespace WebCore
index f729354..908e253 100644 (file)
 #if ENABLE(SERVICE_WORKER)
 
 #include "SecurityOrigin.h"
-#include <wtf/CrossThreadTaskHandler.h>
+#include <pal/SessionID.h>
+#include <wtf/ThreadSafeRefCounted.h>
+#include <wtf/WeakPtr.h>
+#include <wtf/WorkQueue.h>
 #include <wtf/text/WTFString.h>
 
 namespace WebCore {
@@ -39,19 +42,27 @@ struct ServiceWorkerContextData;
 
 WEBCORE_EXPORT String serviceWorkerRegistrationDatabaseFilename(const String& databaseDirectory);
 
-class RegistrationDatabase : public CrossThreadTaskHandler {
+class RegistrationDatabase : public ThreadSafeRefCounted<RegistrationDatabase, WTF::DestructionThread::Main> {
 WTF_MAKE_FAST_ALLOCATED;
 public:
-    RegistrationDatabase(RegistrationStore&, const String& databaseDirectory);
+    static Ref<RegistrationDatabase> create(RegistrationStore& store, String&& databaseDirectory)
+    {
+        return adoptRef(*new RegistrationDatabase(store, WTFMove(databaseDirectory)));
+    }
+
     ~RegistrationDatabase();
 
     bool isClosed() const { return !m_database; }
 
-    void pushChanges(Vector<ServiceWorkerContextData>&&, WTF::CompletionHandler<void()>&&);
-    void clearAll(WTF::CompletionHandler<void()>&&);
+    void pushChanges(Vector<ServiceWorkerContextData>&&, CompletionHandler<void()>&&);
+    void clearAll(CompletionHandler<void()>&&);
 
 private:
-    // Methods to be run on the task thread
+    RegistrationDatabase(RegistrationStore&, String&& databaseDirectory);
+
+    void postTaskToWorkQueue(Function<void()>&&);
+
+    // Methods to be run on the work queue.
     void openSQLiteDatabase(const String& fullFilename);
     String ensureValidRecordsTable();
     String importRecords();
@@ -59,12 +70,14 @@ private:
     void doPushChanges(Vector<ServiceWorkerContextData>&&);
     void doClearOrigin(const SecurityOrigin&);
 
-    // Replies to the main thread
+    // Replies to the main thread.
     void addRegistrationToStore(ServiceWorkerContextData&&);
     void databaseFailedToOpen();
     void databaseOpenedAndRecordsImported(); 
 
-    RegistrationStore& m_store;
+    Ref<WorkQueue> m_workQueue;
+    WeakPtr<RegistrationStore> m_store;
+    PAL::SessionID m_session;
     String m_databaseDirectory;
     String m_databaseFilePath;
     std::unique_ptr<SQLiteDatabase> m_database;
index ad80767..a59b727 100644 (file)
 
 namespace WebCore {
 
-RegistrationStore::RegistrationStore(SWServer& server, const String& databaseDirectory)
+RegistrationStore::RegistrationStore(SWServer& server, String&& databaseDirectory)
     : m_server(server)
-    , m_database(*this, databaseDirectory)
+    , m_database(RegistrationDatabase::create(*this, WTFMove(databaseDirectory)))
     , m_databasePushTimer(*this, &RegistrationStore::pushChangesToDatabase)
 {
 }
 
 RegistrationStore::~RegistrationStore()
 {
-    ASSERT(m_database.isClosed());
+    ASSERT(m_database->isClosed());
 }
 
 void RegistrationStore::scheduleDatabasePushIfNecessary()
@@ -59,14 +59,14 @@ void RegistrationStore::pushChangesToDatabase(WTF::CompletionHandler<void()>&& c
         changesToPush.uncheckedAppend(WTFMove(value));
 
     m_updatedRegistrations.clear();
-    m_database.pushChanges(WTFMove(changesToPush), WTFMove(completionHandler));
+    m_database->pushChanges(WTFMove(changesToPush), WTFMove(completionHandler));
 }
 
 void RegistrationStore::clearAll(WTF::CompletionHandler<void()>&& completionHandler)
 {
     m_updatedRegistrations.clear();
     m_databasePushTimer.stop();
-    m_database.clearAll(WTFMove(completionHandler));
+    m_database->clearAll(WTFMove(completionHandler));
 }
 
 void RegistrationStore::flushChanges(WTF::CompletionHandler<void()>&& completionHandler)
index 29e8214..0d2dbc2 100644 (file)
@@ -34,6 +34,7 @@
 #include "Timer.h"
 #include <wtf/CompletionHandler.h>
 #include <wtf/HashMap.h>
+#include <wtf/WeakPtr.h>
 #include <wtf/text/WTFString.h>
 
 namespace WebCore {
@@ -41,10 +42,10 @@ namespace WebCore {
 class SWServer;
 class SWServerRegistration;
 
-class RegistrationStore {
+class RegistrationStore : public CanMakeWeakPtr<RegistrationStore> {
 WTF_MAKE_FAST_ALLOCATED;
 public:
-    explicit RegistrationStore(SWServer&, const String& databaseDirectory);
+    explicit RegistrationStore(SWServer&, String&& databaseDirectory);
     ~RegistrationStore();
 
     void clearAll(WTF::CompletionHandler<void()>&&);
@@ -67,7 +68,7 @@ private:
     void pushChangesToDatabase() { pushChangesToDatabase({ }); }
 
     SWServer& m_server;
-    RegistrationDatabase m_database;
+    Ref<RegistrationDatabase> m_database;
 
     HashMap<ServiceWorkerRegistrationKey, ServiceWorkerContextData> m_updatedRegistrations;
     Timer m_databasePushTimer;