Changeset 232874 in webkit


Ignore:
Timestamp:
Jun 15, 2018 9:16:34 AM (6 years ago)
Author:
Chris Dumez
Message:

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:
Location:
trunk/Source/WebCore
Files:
5 edited

Legend:

Unmodified
Added
Removed
  • trunk/Source/WebCore/ChangeLog

    r232871 r232874  
     12018-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
    1432018-06-15  Zalan Bujtas  <zalan@apple.com>
    244
  • trunk/Source/WebCore/workers/service/server/RegistrationDatabase.cpp

    r232516 r232874  
    3939#include "SecurityOrigin.h"
    4040#include <wtf/CompletionHandler.h>
     41#include <wtf/CrossThreadCopier.h>
    4142#include <wtf/MainThread.h>
    4243#include <wtf/NeverDestroyed.h>
     
    9293}
    9394
    94 RegistrationDatabase::RegistrationDatabase(RegistrationStore& store, const String& databaseDirectory)
    95     : CrossThreadTaskHandler("ServiceWorker I/O Thread")
    96     , m_store(store)
    97     , m_databaseDirectory(databaseDirectory)
     95RegistrationDatabase::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))
    98100    , m_databaseFilePath(FileSystem::pathByAppendingComponent(m_databaseDirectory, databaseFilename()))
    99101{
    100102    ASSERT(isMainThread());
    101103
    102     postTask(CrossThreadTask([this] {
     104    postTaskToWorkQueue([this] {
    103105        importRecordsIfNecessary();
    104     }));
     106    });
    105107}
    106108
    107109RegistrationDatabase::~RegistrationDatabase()
    108110{
    109     ASSERT(!m_database);
    110111    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
     118void RegistrationDatabase::postTaskToWorkQueue(Function<void()>&& task)
     119{
     120    m_workQueue->dispatch([protectedThis = makeRef(*this), task = WTFMove(task)]() mutable {
     121        task();
     122    });
    111123}
    112124
     
    131143
    132144        m_database = nullptr;
    133         postTaskReply(createCrossThreadTask(*this, &RegistrationDatabase::databaseFailedToOpen));
     145        callOnMainThread([protectedThis = makeRef(*this)] {
     146            protectedThis->databaseFailedToOpen();
     147        });
    134148    });
    135149
     
    141155        return;
    142156    }
     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();
    143162   
    144163    errorMessage = ensureValidRecordsTable();
     
    160179        openSQLiteDatabase(m_databaseFilePath);
    161180
    162     postTaskReply(createCrossThreadTask(*this, &RegistrationDatabase::databaseOpenedAndRecordsImported));
     181    callOnMainThread([protectedThis = makeRef(*this)] {
     182        protectedThis->databaseOpenedAndRecordsImported();
     183    });
    163184}
    164185
     
    250271}
    251272
    252 void RegistrationDatabase::pushChanges(Vector<ServiceWorkerContextData>&& datas, WTF::CompletionHandler<void()>&& completionHandler)
    253 {
    254     postTask(CrossThreadTask([this, datas = crossThreadCopy(datas), completionHandler = WTFMove(completionHandler)]() mutable {
     273void RegistrationDatabase::pushChanges(Vector<ServiceWorkerContextData>&& datas, CompletionHandler<void()>&& completionHandler)
     274{
     275    postTaskToWorkQueue([this, datas = crossThreadCopy(datas), completionHandler = WTFMove(completionHandler)]() mutable {
    255276        doPushChanges(WTFMove(datas));
    256277
     
    258279            return;
    259280
    260         postTaskReply(CrossThreadTask([completionHandler = WTFMove(completionHandler)] {
     281        callOnMainThread([completionHandler = WTFMove(completionHandler)] {
    261282            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
     287void RegistrationDatabase::clearAll(CompletionHandler<void()>&& completionHandler)
     288{
     289    postTaskToWorkQueue([this, completionHandler = WTFMove(completionHandler)]() mutable {
    269290        m_database = nullptr;
    270291
     
    272293        SQLiteFileSystem::deleteEmptyDatabaseDirectory(m_databaseDirectory);
    273294
    274         postTaskReply(CrossThreadTask([completionHandler = WTFMove(completionHandler)] {
     295        callOnMainThread([completionHandler = WTFMove(completionHandler)] {
    275296            completionHandler();
    276         }));
    277     }));
     297        });
     298    });
    278299}
    279300
     
    384405        auto serviceWorkerData = ServiceWorkerData { workerIdentifier, scriptURL, ServiceWorkerState::Activated, *workerType, registrationIdentifier };
    385406        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        });
    389412    }
    390413
     
    397420void RegistrationDatabase::addRegistrationToStore(ServiceWorkerContextData&& context)
    398421{
    399     m_store.addRegistrationFromDatabase(WTFMove(context));
     422    if (m_store)
     423        m_store->addRegistrationFromDatabase(WTFMove(context));
    400424}
    401425
    402426void RegistrationDatabase::databaseFailedToOpen()
    403427{
    404     m_store.databaseFailedToOpen();
     428    if (m_store)
     429        m_store->databaseFailedToOpen();
    405430}
    406431
    407432void RegistrationDatabase::databaseOpenedAndRecordsImported()
    408433{
    409     m_store.databaseOpenedAndRecordsImported();
     434    if (m_store)
     435        m_store->databaseOpenedAndRecordsImported();
    410436}
    411437
  • trunk/Source/WebCore/workers/service/server/RegistrationDatabase.h

    r227161 r232874  
    2929
    3030#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>
    3235#include <wtf/text/WTFString.h>
    3336
     
    4043WEBCORE_EXPORT String serviceWorkerRegistrationDatabaseFilename(const String& databaseDirectory);
    4144
    42 class RegistrationDatabase : public CrossThreadTaskHandler {
     45class RegistrationDatabase : public ThreadSafeRefCounted<RegistrationDatabase, WTF::DestructionThread::Main> {
    4346WTF_MAKE_FAST_ALLOCATED;
    4447public:
    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
    4653    ~RegistrationDatabase();
    4754
    4855    bool isClosed() const { return !m_database; }
    4956
    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()>&&);
    5259
    5360private:
    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.
    5566    void openSQLiteDatabase(const String& fullFilename);
    5667    String ensureValidRecordsTable();
     
    6071    void doClearOrigin(const SecurityOrigin&);
    6172
    62     // Replies to the main thread
     73    // Replies to the main thread.
    6374    void addRegistrationToStore(ServiceWorkerContextData&&);
    6475    void databaseFailedToOpen();
    6576    void databaseOpenedAndRecordsImported();
    6677
    67     RegistrationStore& m_store;
     78    Ref<WorkQueue> m_workQueue;
     79    WeakPtr<RegistrationStore> m_store;
     80    PAL::SessionID m_session;
    6881    String m_databaseDirectory;
    6982    String m_databaseFilePath;
  • trunk/Source/WebCore/workers/service/server/RegistrationStore.cpp

    r226064 r232874  
    3333namespace WebCore {
    3434
    35 RegistrationStore::RegistrationStore(SWServer& server, const String& databaseDirectory)
     35RegistrationStore::RegistrationStore(SWServer& server, String&& databaseDirectory)
    3636    : m_server(server)
    37     , m_database(*this, databaseDirectory)
     37    , m_database(RegistrationDatabase::create(*this, WTFMove(databaseDirectory)))
    3838    , m_databasePushTimer(*this, &RegistrationStore::pushChangesToDatabase)
    3939{
     
    4242RegistrationStore::~RegistrationStore()
    4343{
    44     ASSERT(m_database.isClosed());
     44    ASSERT(m_database->isClosed());
    4545}
    4646
     
    6060
    6161    m_updatedRegistrations.clear();
    62     m_database.pushChanges(WTFMove(changesToPush), WTFMove(completionHandler));
     62    m_database->pushChanges(WTFMove(changesToPush), WTFMove(completionHandler));
    6363}
    6464
     
    6767    m_updatedRegistrations.clear();
    6868    m_databasePushTimer.stop();
    69     m_database.clearAll(WTFMove(completionHandler));
     69    m_database->clearAll(WTFMove(completionHandler));
    7070}
    7171
  • trunk/Source/WebCore/workers/service/server/RegistrationStore.h

    r228978 r232874  
    3535#include <wtf/CompletionHandler.h>
    3636#include <wtf/HashMap.h>
     37#include <wtf/WeakPtr.h>
    3738#include <wtf/text/WTFString.h>
    3839
     
    4243class SWServerRegistration;
    4344
    44 class RegistrationStore {
     45class RegistrationStore : public CanMakeWeakPtr<RegistrationStore> {
    4546WTF_MAKE_FAST_ALLOCATED;
    4647public:
    47     explicit RegistrationStore(SWServer&, const String& databaseDirectory);
     48    explicit RegistrationStore(SWServer&, String&& databaseDirectory);
    4849    ~RegistrationStore();
    4950
     
    6869
    6970    SWServer& m_server;
    70     RegistrationDatabase m_database;
     71    Ref<RegistrationDatabase> m_database;
    7172
    7273    HashMap<ServiceWorkerRegistrationKey, ServiceWorkerContextData> m_updatedRegistrations;
Note: See TracChangeset for help on using the changeset viewer.