Changeset 218516 in webkit

Timestamp:

Jun 19, 2017 4:19:11 PM (7 years ago)

Author:

beidson@apple.com

Message:

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:

Location:

trunk/Source/WebCore

Files:

• ChangeLog (modified) (1 diff)
• Modules/indexeddb/server/UniqueIDBDatabase.cpp (modified) (4 diffs)
• Modules/indexeddb/server/UniqueIDBDatabase.h (modified) (1 diff)

trunk/Source/WebCore/ChangeLog

@@ +1 @@
+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>
 

trunk/Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp

@@ -1707 +1707 @@
 void UniqueIDBDatabase::postDatabaseTask(CrossThreadTask&& task)
 {
-    m_databaseQueue.append(WTFMove(task));
+    m_databaseQueue.append([protectedThis = makeRef(*this), task = WTFMove(task)]() mutable {
+        task.performTask();
+    });
     ++m_queuedTaskCount;
 
@@ -1716 +1718 @@
 {
     ASSERT(!isMainThread());
-    m_databaseReplyQueue.append(WTFMove(task));
+
+    m_databaseReplyQueue.append([protectedThis = makeRef(*this), task = WTFMove(task)]() mutable {
+        task.performTask();
+    });
     ++m_queuedTaskCount;
 
@@ -1730 +1735 @@
     ASSERT(task);
 
-    // Performing the task might end up removing the last reference to this.
-    Ref<UniqueIDBDatabase> protectedThis(*this);
-
-    task->performTask();
+    (*task)();
     --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)] {
     });
 }
@@ -1749 +1752 @@
     ASSERT(task);
 
-    // Performing the task might end up removing the last reference to this.
-    Ref<UniqueIDBDatabase> protectedThis(*this);
-
-    task->performTask();
+    (*task)();
     --m_queuedTaskCount;
 

trunk/Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.h

@@ -267 +267 @@
     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 };