Changeset 232874 in webkit
- Timestamp:
- Jun 15, 2018 9:16:34 AM (6 years ago)
- Location:
- trunk/Source/WebCore
- Files:
-
- 5 edited
Legend:
- Unmodified
- Added
- Removed
-
trunk/Source/WebCore/ChangeLog
r232871 r232874 1 2018-06-15 Chris Dumez <cdumez@apple.com> 2 3 Crash under WebCore::SWServer::registrationStoreImportComplete() 4 https://bugs.webkit.org/show_bug.cgi?id=186644 5 <rdar://problem/40982257> 6 7 Reviewed by Brady Eidson. 8 9 Fix lifetime management issues with RegistrationDatabase. RegistrationDatabase 10 was previously subclassing CrossThreadTaskHandler. CrossThreadTaskHandler 11 currently is not safe for objects that can get destroyed (such as 12 RegistrationDatabase). This is because it does not keep the object alive 13 when going to the background thread or back to the main thread. This would 14 cause crashes such as the one in the radar. 15 16 To address the issue, stop subclassing CrossThreadTaskHandler and use a 17 simple WorkQueue instead. RegistrationDatabase is now ThreadSafeRefCounted 18 and we take care of ref'ing it whenever we dispatch a task to the work queue 19 or back to the main thread. Because the RegistrationDatabase can now outlive 20 the RegistrationStore, m_store is now a WeakPtr. 21 22 * workers/service/server/RegistrationDatabase.cpp: 23 (WebCore::RegistrationDatabase::RegistrationDatabase): 24 (WebCore::RegistrationDatabase::~RegistrationDatabase): 25 (WebCore::RegistrationDatabase::postTaskToWorkQueue): 26 (WebCore::RegistrationDatabase::openSQLiteDatabase): 27 (WebCore::RegistrationDatabase::importRecordsIfNecessary): 28 (WebCore::RegistrationDatabase::pushChanges): 29 (WebCore::RegistrationDatabase::clearAll): 30 (WebCore::RegistrationDatabase::importRecords): 31 (WebCore::RegistrationDatabase::addRegistrationToStore): 32 (WebCore::RegistrationDatabase::databaseFailedToOpen): 33 (WebCore::RegistrationDatabase::databaseOpenedAndRecordsImported): 34 * workers/service/server/RegistrationDatabase.h: 35 (WebCore::RegistrationDatabase::create): 36 * workers/service/server/RegistrationStore.cpp: 37 (WebCore::RegistrationStore::RegistrationStore): 38 (WebCore::RegistrationStore::~RegistrationStore): 39 (WebCore::RegistrationStore::pushChangesToDatabase): 40 (WebCore::RegistrationStore::clearAll): 41 * workers/service/server/RegistrationStore.h: 42 1 43 2018-06-15 Zalan Bujtas <zalan@apple.com> 2 44 -
trunk/Source/WebCore/workers/service/server/RegistrationDatabase.cpp
r232516 r232874 39 39 #include "SecurityOrigin.h" 40 40 #include <wtf/CompletionHandler.h> 41 #include <wtf/CrossThreadCopier.h> 41 42 #include <wtf/MainThread.h> 42 43 #include <wtf/NeverDestroyed.h> … … 92 93 } 93 94 94 RegistrationDatabase::RegistrationDatabase(RegistrationStore& store, const String& databaseDirectory) 95 : CrossThreadTaskHandler("ServiceWorker I/O Thread") 96 , m_store(store) 97 , m_databaseDirectory(databaseDirectory) 95 RegistrationDatabase::RegistrationDatabase(RegistrationStore& store, String&& databaseDirectory) 96 : m_workQueue(WorkQueue::create("ServiceWorker I/O Thread", WorkQueue::Type::Serial)) 97 , m_store(makeWeakPtr(store)) 98 , m_session(m_store->server().sessionID()) 99 , m_databaseDirectory(WTFMove(databaseDirectory)) 98 100 , m_databaseFilePath(FileSystem::pathByAppendingComponent(m_databaseDirectory, databaseFilename())) 99 101 { 100 102 ASSERT(isMainThread()); 101 103 102 postTask (CrossThreadTask([this] {104 postTaskToWorkQueue([this] { 103 105 importRecordsIfNecessary(); 104 }) );106 }); 105 107 } 106 108 107 109 RegistrationDatabase::~RegistrationDatabase() 108 110 { 109 ASSERT(!m_database);110 111 ASSERT(isMainThread()); 112 113 // The database needs to be destroyed on the background thread. 114 if (m_database) 115 m_workQueue->dispatch([database = WTFMove(m_database)] { }); 116 } 117 118 void RegistrationDatabase::postTaskToWorkQueue(Function<void()>&& task) 119 { 120 m_workQueue->dispatch([protectedThis = makeRef(*this), task = WTFMove(task)]() mutable { 121 task(); 122 }); 111 123 } 112 124 … … 131 143 132 144 m_database = nullptr; 133 postTaskReply(createCrossThreadTask(*this, &RegistrationDatabase::databaseFailedToOpen)); 145 callOnMainThread([protectedThis = makeRef(*this)] { 146 protectedThis->databaseFailedToOpen(); 147 }); 134 148 }); 135 149 … … 141 155 return; 142 156 } 157 158 // Disable threading checks. We always access the database from our serial WorkQueue. Such accesses 159 // are safe since work queue tasks are guaranteed to run one after another. However, tasks will not 160 // necessary run on the same thread every time (as per GCD documentation). 161 m_database->disableThreadingChecks(); 143 162 144 163 errorMessage = ensureValidRecordsTable(); … … 160 179 openSQLiteDatabase(m_databaseFilePath); 161 180 162 postTaskReply(createCrossThreadTask(*this, &RegistrationDatabase::databaseOpenedAndRecordsImported)); 181 callOnMainThread([protectedThis = makeRef(*this)] { 182 protectedThis->databaseOpenedAndRecordsImported(); 183 }); 163 184 } 164 185 … … 250 271 } 251 272 252 void RegistrationDatabase::pushChanges(Vector<ServiceWorkerContextData>&& datas, WTF::CompletionHandler<void()>&& completionHandler)253 { 254 postTask (CrossThreadTask([this, datas = crossThreadCopy(datas), completionHandler = WTFMove(completionHandler)]() mutable {273 void RegistrationDatabase::pushChanges(Vector<ServiceWorkerContextData>&& datas, CompletionHandler<void()>&& completionHandler) 274 { 275 postTaskToWorkQueue([this, datas = crossThreadCopy(datas), completionHandler = WTFMove(completionHandler)]() mutable { 255 276 doPushChanges(WTFMove(datas)); 256 277 … … 258 279 return; 259 280 260 postTaskReply(CrossThreadTask([completionHandler = WTFMove(completionHandler)] {281 callOnMainThread([completionHandler = WTFMove(completionHandler)] { 261 282 completionHandler(); 262 }) );263 }) );264 } 265 266 void RegistrationDatabase::clearAll( WTF::CompletionHandler<void()>&& completionHandler)267 { 268 postTask (CrossThreadTask([this, completionHandler = WTFMove(completionHandler)]() mutable {283 }); 284 }); 285 } 286 287 void RegistrationDatabase::clearAll(CompletionHandler<void()>&& completionHandler) 288 { 289 postTaskToWorkQueue([this, completionHandler = WTFMove(completionHandler)]() mutable { 269 290 m_database = nullptr; 270 291 … … 272 293 SQLiteFileSystem::deleteEmptyDatabaseDirectory(m_databaseDirectory); 273 294 274 postTaskReply(CrossThreadTask([completionHandler = WTFMove(completionHandler)] {295 callOnMainThread([completionHandler = WTFMove(completionHandler)] { 275 296 completionHandler(); 276 }) );277 }) );297 }); 298 }); 278 299 } 279 300 … … 384 405 auto serviceWorkerData = ServiceWorkerData { workerIdentifier, scriptURL, ServiceWorkerState::Activated, *workerType, registrationIdentifier }; 385 406 auto registration = ServiceWorkerRegistrationData { WTFMove(*key), registrationIdentifier, URL(originURL, scopePath), *updateViaCache, lastUpdateCheckTime, std::nullopt, std::nullopt, WTFMove(serviceWorkerData) }; 386 auto contextData = ServiceWorkerContextData { std::nullopt, WTFMove(registration), workerIdentifier, WTFMove(script), WTFMove(contentSecurityPolicy), WTFMove(scriptURL), *workerType, m_store.server().sessionID(), true, WTFMove(scriptResourceMap) }; 387 388 postTaskReply(createCrossThreadTask(*this, &RegistrationDatabase::addRegistrationToStore, WTFMove(contextData))); 407 auto contextData = ServiceWorkerContextData { std::nullopt, WTFMove(registration), workerIdentifier, WTFMove(script), WTFMove(contentSecurityPolicy), WTFMove(scriptURL), *workerType, m_session, true, WTFMove(scriptResourceMap) }; 408 409 callOnMainThread([protectedThis = makeRef(*this), contextData = contextData.isolatedCopy()]() mutable { 410 protectedThis->addRegistrationToStore(WTFMove(contextData)); 411 }); 389 412 } 390 413 … … 397 420 void RegistrationDatabase::addRegistrationToStore(ServiceWorkerContextData&& context) 398 421 { 399 m_store.addRegistrationFromDatabase(WTFMove(context)); 422 if (m_store) 423 m_store->addRegistrationFromDatabase(WTFMove(context)); 400 424 } 401 425 402 426 void RegistrationDatabase::databaseFailedToOpen() 403 427 { 404 m_store.databaseFailedToOpen(); 428 if (m_store) 429 m_store->databaseFailedToOpen(); 405 430 } 406 431 407 432 void RegistrationDatabase::databaseOpenedAndRecordsImported() 408 433 { 409 m_store.databaseOpenedAndRecordsImported(); 434 if (m_store) 435 m_store->databaseOpenedAndRecordsImported(); 410 436 } 411 437 -
trunk/Source/WebCore/workers/service/server/RegistrationDatabase.h
r227161 r232874 29 29 30 30 #include "SecurityOrigin.h" 31 #include <wtf/CrossThreadTaskHandler.h> 31 #include <pal/SessionID.h> 32 #include <wtf/ThreadSafeRefCounted.h> 33 #include <wtf/WeakPtr.h> 34 #include <wtf/WorkQueue.h> 32 35 #include <wtf/text/WTFString.h> 33 36 … … 40 43 WEBCORE_EXPORT String serviceWorkerRegistrationDatabaseFilename(const String& databaseDirectory); 41 44 42 class RegistrationDatabase : public CrossThreadTaskHandler{45 class RegistrationDatabase : public ThreadSafeRefCounted<RegistrationDatabase, WTF::DestructionThread::Main> { 43 46 WTF_MAKE_FAST_ALLOCATED; 44 47 public: 45 RegistrationDatabase(RegistrationStore&, const String& databaseDirectory); 48 static Ref<RegistrationDatabase> create(RegistrationStore& store, String&& databaseDirectory) 49 { 50 return adoptRef(*new RegistrationDatabase(store, WTFMove(databaseDirectory))); 51 } 52 46 53 ~RegistrationDatabase(); 47 54 48 55 bool isClosed() const { return !m_database; } 49 56 50 void pushChanges(Vector<ServiceWorkerContextData>&&, WTF::CompletionHandler<void()>&&);51 void clearAll( WTF::CompletionHandler<void()>&&);57 void pushChanges(Vector<ServiceWorkerContextData>&&, CompletionHandler<void()>&&); 58 void clearAll(CompletionHandler<void()>&&); 52 59 53 60 private: 54 // Methods to be run on the task thread 61 RegistrationDatabase(RegistrationStore&, String&& databaseDirectory); 62 63 void postTaskToWorkQueue(Function<void()>&&); 64 65 // Methods to be run on the work queue. 55 66 void openSQLiteDatabase(const String& fullFilename); 56 67 String ensureValidRecordsTable(); … … 60 71 void doClearOrigin(const SecurityOrigin&); 61 72 62 // Replies to the main thread 73 // Replies to the main thread. 63 74 void addRegistrationToStore(ServiceWorkerContextData&&); 64 75 void databaseFailedToOpen(); 65 76 void databaseOpenedAndRecordsImported(); 66 77 67 RegistrationStore& m_store; 78 Ref<WorkQueue> m_workQueue; 79 WeakPtr<RegistrationStore> m_store; 80 PAL::SessionID m_session; 68 81 String m_databaseDirectory; 69 82 String m_databaseFilePath; -
trunk/Source/WebCore/workers/service/server/RegistrationStore.cpp
r226064 r232874 33 33 namespace WebCore { 34 34 35 RegistrationStore::RegistrationStore(SWServer& server, const String& databaseDirectory)35 RegistrationStore::RegistrationStore(SWServer& server, String&& databaseDirectory) 36 36 : m_server(server) 37 , m_database( *this, databaseDirectory)37 , m_database(RegistrationDatabase::create(*this, WTFMove(databaseDirectory))) 38 38 , m_databasePushTimer(*this, &RegistrationStore::pushChangesToDatabase) 39 39 { … … 42 42 RegistrationStore::~RegistrationStore() 43 43 { 44 ASSERT(m_database .isClosed());44 ASSERT(m_database->isClosed()); 45 45 } 46 46 … … 60 60 61 61 m_updatedRegistrations.clear(); 62 m_database .pushChanges(WTFMove(changesToPush), WTFMove(completionHandler));62 m_database->pushChanges(WTFMove(changesToPush), WTFMove(completionHandler)); 63 63 } 64 64 … … 67 67 m_updatedRegistrations.clear(); 68 68 m_databasePushTimer.stop(); 69 m_database .clearAll(WTFMove(completionHandler));69 m_database->clearAll(WTFMove(completionHandler)); 70 70 } 71 71 -
trunk/Source/WebCore/workers/service/server/RegistrationStore.h
r228978 r232874 35 35 #include <wtf/CompletionHandler.h> 36 36 #include <wtf/HashMap.h> 37 #include <wtf/WeakPtr.h> 37 38 #include <wtf/text/WTFString.h> 38 39 … … 42 43 class SWServerRegistration; 43 44 44 class RegistrationStore {45 class RegistrationStore : public CanMakeWeakPtr<RegistrationStore> { 45 46 WTF_MAKE_FAST_ALLOCATED; 46 47 public: 47 explicit RegistrationStore(SWServer&, const String& databaseDirectory);48 explicit RegistrationStore(SWServer&, String&& databaseDirectory); 48 49 ~RegistrationStore(); 49 50 … … 68 69 69 70 SWServer& m_server; 70 Re gistrationDatabasem_database;71 Ref<RegistrationDatabase> m_database; 71 72 72 73 HashMap<ServiceWorkerRegistrationKey, ServiceWorkerContextData> m_updatedRegistrations;
Note: See TracChangeset
for help on using the changeset viewer.