Attachment 342788 Details for Bug 186644 – Patch

[patch] Patch

bug-186644-20180614210937.patch (text/plain), 14.84 KB, created by Chris Dumez on 2018-06-14 21:09:10 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Chris Dumez

Created: 2018-06-14 21:09:10 PDT

Size: 14.84 KB

patch

obsolete

>Subversion Revision: 232862
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 6b9657cccc20f43865b53672c8911d414dc6ab7e..aab4aca76353033ed6661399907450198935e9c2 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,45 @@
>+2018-06-14  Chris Dumez  <cdumez@apple.com>
>+
>+        Crash under WebCore::SWServer::registrationStoreImportComplete()
>+        https://bugs.webkit.org/show_bug.cgi?id=186644
>+        <rdar://problem/40982257>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        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-14  Matt Lewis  <jlewis3@apple.com>
> 
>         Unreviewed, rolling out r232823.
>diff --git a/Source/WebCore/workers/service/server/RegistrationDatabase.cpp b/Source/WebCore/workers/service/server/RegistrationDatabase.cpp
>index 7d19f517ca932b91a24376ea19885e3000a56cff..ea2ce5da769bc1c58cf323f239ca6051877c83ad 100644
>--- a/Source/WebCore/workers/service/server/RegistrationDatabase.cpp
>+++ b/Source/WebCore/workers/service/server/RegistrationDatabase.cpp
>@@ -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>
>@@ -92,22 +93,36 @@ static inline void cleanOldDatabases(const String& databaseDirectory)
> }
> 
> RegistrationDatabase::RegistrationDatabase(RegistrationStore& store, const String& databaseDirectory)
>-    : CrossThreadTaskHandler("ServiceWorker I/O Thread")
>-    , m_store(store)
>+    : m_workQueue(WorkQueue::create("ServiceWorker I/O Thread", WorkQueue::Type::Serial))
>+    , m_store(makeWeakPtr(store))
>+    , m_session(m_store->server().sessionID())
>     , m_databaseDirectory(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(WTF::Function<void()>&& task)
>+{
>+    m_workQueue->dispatch([protectedThis = makeRef(*this), task = WTFMove(task)]() mutable {
>+        task();
>+
>+        // Make sure we ever get destroyed on the main thread.
>+        callOnMainThread([protectedThis = WTFMove(protectedThis)] { });
>+    });
> }
> 
> void RegistrationDatabase::openSQLiteDatabase(const String& fullFilename)
>@@ -130,7 +145,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 +157,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 +181,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()
>@@ -251,30 +275,30 @@ static std::optional<WorkerType> stringToWorkerType(const String& type)
> 
> void RegistrationDatabase::pushChanges(Vector<ServiceWorkerContextData>&& datas, WTF::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)
> {
>-    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 +407,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 +422,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
>diff --git a/Source/WebCore/workers/service/server/RegistrationDatabase.h b/Source/WebCore/workers/service/server/RegistrationDatabase.h
>index f7293545704b7136cc52ec512bc4d9d9cd874ae1..46b1aeb1b75205e177e84b0a4e9a17bcfb67eb1d 100644
>--- a/Source/WebCore/workers/service/server/RegistrationDatabase.h
>+++ b/Source/WebCore/workers/service/server/RegistrationDatabase.h
>@@ -28,7 +28,10 @@
> #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,10 +42,14 @@ struct ServiceWorkerContextData;
> 
> WEBCORE_EXPORT String serviceWorkerRegistrationDatabaseFilename(const String& databaseDirectory);
> 
>-class RegistrationDatabase : public CrossThreadTaskHandler {
>+class RegistrationDatabase : public ThreadSafeRefCounted<RegistrationDatabase> {
> WTF_MAKE_FAST_ALLOCATED;
> public:
>-    RegistrationDatabase(RegistrationStore&, const String& databaseDirectory);
>+    static Ref<RegistrationDatabase> create(RegistrationStore& store, const String& databaseDirectory)
>+    {
>+        return adoptRef(*new RegistrationDatabase(store, databaseDirectory));
>+    }
>+
>     ~RegistrationDatabase();
> 
>     bool isClosed() const { return !m_database; }
>@@ -51,7 +58,11 @@ public:
>     void clearAll(WTF::CompletionHandler<void()>&&);
> 
> private:
>-    // Methods to be run on the task thread
>+    RegistrationDatabase(RegistrationStore&, const String& databaseDirectory);
>+
>+    void postTaskToWorkQueue(WTF::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;
>diff --git a/Source/WebCore/workers/service/server/RegistrationStore.cpp b/Source/WebCore/workers/service/server/RegistrationStore.cpp
>index ad80767a68ec3046def41b358a53b6c1cc950189..0651648e5fbde39a6cfed6e0aeeaa9605f454fb6 100644
>--- a/Source/WebCore/workers/service/server/RegistrationStore.cpp
>+++ b/Source/WebCore/workers/service/server/RegistrationStore.cpp
>@@ -34,14 +34,14 @@ namespace WebCore {
> 
> RegistrationStore::RegistrationStore(SWServer& server, const String& databaseDirectory)
>     : m_server(server)
>-    , m_database(*this, databaseDirectory)
>+    , m_database(RegistrationDatabase::create(*this, 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)
>diff --git a/Source/WebCore/workers/service/server/RegistrationStore.h b/Source/WebCore/workers/service/server/RegistrationStore.h
>index 29e8214b301c65322a31b16e5a034458e4859b9a..c4de7a6320cd79703a81d1bb33ea1151e17b1bc3 100644
>--- a/Source/WebCore/workers/service/server/RegistrationStore.h
>+++ b/Source/WebCore/workers/service/server/RegistrationStore.h
>@@ -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,7 +42,7 @@ namespace WebCore {
> class SWServer;
> class SWServerRegistration;
> 
>-class RegistrationStore {
>+class RegistrationStore : public CanMakeWeakPtr<RegistrationStore> {
> WTF_MAKE_FAST_ALLOCATED;
> public:
>     explicit RegistrationStore(SWServer&, const String& databaseDirectory);
>@@ -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;

Actions: View | Formatted Diff | Diff

Attachments on bug 186644: 342788 | 342794 | 342814