WebKit Bugzilla
Attachment 342814 Details for
Bug 186644
: Crash under WebCore::SWServer::registrationStoreImportComplete()
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Requests
|
Help
|
New Account
|
Log In
Remember
[x]
|
Forgot Password
Login:
[x]
[patch]
Patch
bug-186644-20180615083548.patch (text/plain), 15.69 KB, created by
Chris Dumez
on 2018-06-15 08:35:18 PDT
(
hide
)
Description:
Patch
Filename:
MIME Type:
Creator:
Chris Dumez
Created:
2018-06-15 08:35:18 PDT
Size:
15.69 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..7749b884cae2ad439f626c919f7c9b9ab3561c9a 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> >@@ -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 >diff --git a/Source/WebCore/workers/service/server/RegistrationDatabase.h b/Source/WebCore/workers/service/server/RegistrationDatabase.h >index f7293545704b7136cc52ec512bc4d9d9cd874ae1..908e2537bf738ac5eedaa993867820d1051ef7f3 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,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; >diff --git a/Source/WebCore/workers/service/server/RegistrationStore.cpp b/Source/WebCore/workers/service/server/RegistrationStore.cpp >index ad80767a68ec3046def41b358a53b6c1cc950189..a59b727143f3b5eebdb707f9f6b7e7d356878282 100644 >--- a/Source/WebCore/workers/service/server/RegistrationStore.cpp >+++ b/Source/WebCore/workers/service/server/RegistrationStore.cpp >@@ -32,16 +32,16 @@ > > 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) >diff --git a/Source/WebCore/workers/service/server/RegistrationStore.h b/Source/WebCore/workers/service/server/RegistrationStore.h >index 29e8214b301c65322a31b16e5a034458e4859b9a..0d2dbc2deb93a419fd9109d62e5f6167795493bd 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,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;
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Formatted Diff
|
Diff
Attachments on
bug 186644
:
342788
|
342794
| 342814