Various IndexedDB crashes as an after effect of previous test.
authorbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 19 Jun 2017 23:19:11 +0000 (23:19 +0000)
committerbeidson@apple.com <beidson@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Mon, 19 Jun 2017 23:19:11 +0000 (23:19 +0000)
<rdar://problem/31418761> and https://bugs.webkit.org/show_bug.cgi?id=170436

Reviewed by Chris Dumez.

No new test (No consistent test possible, in practice covered by all existing IDB tests)

This is timing related, where a UniqueIDBDatabase can be destroyed on the main thread while
it still has one task left to try to execute on the IDBServer thread.

The background thread tasks don't Ref<> the UniqueIDBDatabase, so even though task execution
took a Ref<> protector, there was still a small window for a race.

Should be closed up by making the background thread tasks themselves protect this.

* Modules/indexeddb/server/UniqueIDBDatabase.cpp:
(WebCore::IDBServer::UniqueIDBDatabase::postDatabaseTask):
(WebCore::IDBServer::UniqueIDBDatabase::postDatabaseTaskReply):
(WebCore::IDBServer::UniqueIDBDatabase::executeNextDatabaseTask):
(WebCore::IDBServer::UniqueIDBDatabase::executeNextDatabaseTaskReply):
* Modules/indexeddb/server/UniqueIDBDatabase.h:

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

Source/WebCore/ChangeLog
Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp
Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.h

index 3e4f59c..fc8faea 100644 (file)
@@ -1,3 +1,27 @@
+2017-06-19  Brady Eidson  <beidson@apple.com>
+
+        Various IndexedDB crashes as an after effect of previous test.
+        <rdar://problem/31418761> and https://bugs.webkit.org/show_bug.cgi?id=170436
+
+        Reviewed by Chris Dumez.
+
+        No new test (No consistent test possible, in practice covered by all existing IDB tests)
+
+        This is timing related, where a UniqueIDBDatabase can be destroyed on the main thread while
+        it still has one task left to try to execute on the IDBServer thread.
+        
+        The background thread tasks don't Ref<> the UniqueIDBDatabase, so even though task execution
+        took a Ref<> protector, there was still a small window for a race.
+        
+        Should be closed up by making the background thread tasks themselves protect this.
+        
+        * Modules/indexeddb/server/UniqueIDBDatabase.cpp:
+        (WebCore::IDBServer::UniqueIDBDatabase::postDatabaseTask):
+        (WebCore::IDBServer::UniqueIDBDatabase::postDatabaseTaskReply):
+        (WebCore::IDBServer::UniqueIDBDatabase::executeNextDatabaseTask):
+        (WebCore::IDBServer::UniqueIDBDatabase::executeNextDatabaseTaskReply):
+        * Modules/indexeddb/server/UniqueIDBDatabase.h:
+
 2017-06-19  Sam Weinig  <sam@webkit.org>
 
         [WebIDL] Add support for serializers that have members that are themselves serializers (or inherit being a serializer from a parent)
 2017-06-19  Sam Weinig  <sam@webkit.org>
 
         [WebIDL] Add support for serializers that have members that are themselves serializers (or inherit being a serializer from a parent)
index bbac07c..74fc288 100644 (file)
@@ -1706,7 +1706,9 @@ void UniqueIDBDatabase::transactionCompleted(RefPtr<UniqueIDBDatabaseTransaction
 
 void UniqueIDBDatabase::postDatabaseTask(CrossThreadTask&& task)
 {
 
 void UniqueIDBDatabase::postDatabaseTask(CrossThreadTask&& task)
 {
-    m_databaseQueue.append(WTFMove(task));
+    m_databaseQueue.append([protectedThis = makeRef(*this), task = WTFMove(task)]() mutable {
+        task.performTask();
+    });
     ++m_queuedTaskCount;
 
     m_server.postDatabaseTask(createCrossThreadTask(*this, &UniqueIDBDatabase::executeNextDatabaseTask));
     ++m_queuedTaskCount;
 
     m_server.postDatabaseTask(createCrossThreadTask(*this, &UniqueIDBDatabase::executeNextDatabaseTask));
@@ -1715,7 +1717,10 @@ void UniqueIDBDatabase::postDatabaseTask(CrossThreadTask&& task)
 void UniqueIDBDatabase::postDatabaseTaskReply(CrossThreadTask&& task)
 {
     ASSERT(!isMainThread());
 void UniqueIDBDatabase::postDatabaseTaskReply(CrossThreadTask&& task)
 {
     ASSERT(!isMainThread());
-    m_databaseReplyQueue.append(WTFMove(task));
+
+    m_databaseReplyQueue.append([protectedThis = makeRef(*this), task = WTFMove(task)]() mutable {
+        task.performTask();
+    });
     ++m_queuedTaskCount;
 
     m_server.postDatabaseTaskReply(createCrossThreadTask(*this, &UniqueIDBDatabase::executeNextDatabaseTaskReply));
     ++m_queuedTaskCount;
 
     m_server.postDatabaseTaskReply(createCrossThreadTask(*this, &UniqueIDBDatabase::executeNextDatabaseTaskReply));
@@ -1729,14 +1734,12 @@ void UniqueIDBDatabase::executeNextDatabaseTask()
     auto task = m_databaseQueue.tryGetMessage();
     ASSERT(task);
 
     auto task = m_databaseQueue.tryGetMessage();
     ASSERT(task);
 
-    // Performing the task might end up removing the last reference to this.
-    Ref<UniqueIDBDatabase> protectedThis(*this);
-
-    task->performTask();
+    (*task)();
     --m_queuedTaskCount;
 
     --m_queuedTaskCount;
 
-    // Release the ref in the main thread to ensure it's deleted there as expected in case of being the last reference.
-    callOnMainThread([protectedThis = WTFMove(protectedThis)] {
+    // Release the task on the main thread in case it holds the last reference to this,
+    // as UniqueIDBDatabase objects must be deleted on the main thread.
+    callOnMainThread([task = WTFMove(task)] {
     });
 }
 
     });
 }
 
@@ -1748,10 +1751,7 @@ void UniqueIDBDatabase::executeNextDatabaseTaskReply()
     auto task = m_databaseReplyQueue.tryGetMessage();
     ASSERT(task);
 
     auto task = m_databaseReplyQueue.tryGetMessage();
     ASSERT(task);
 
-    // Performing the task might end up removing the last reference to this.
-    Ref<UniqueIDBDatabase> protectedThis(*this);
-
-    task->performTask();
+    (*task)();
     --m_queuedTaskCount;
 
     // If this database was force closed (e.g. for a user delete) and there are no more
     --m_queuedTaskCount;
 
     // If this database was force closed (e.g. for a user delete) and there are no more
index 819a5ea..0f49a9d 100644 (file)
@@ -266,8 +266,8 @@ private:
 
     bool m_deleteBackingStoreInProgress { false };
 
 
     bool m_deleteBackingStoreInProgress { false };
 
-    CrossThreadQueue<CrossThreadTask> m_databaseQueue;
-    CrossThreadQueue<CrossThreadTask> m_databaseReplyQueue;
+    CrossThreadQueue<Function<void ()>> m_databaseQueue;
+    CrossThreadQueue<Function<void ()>> m_databaseReplyQueue;
     std::atomic<uint64_t> m_queuedTaskCount { 0 };
 
     bool m_hardClosedForUserDelete { false };
     std::atomic<uint64_t> m_queuedTaskCount { 0 };
 
     bool m_hardClosedForUserDelete { false };