ThreadSafeRefCounted<DestructionThread::Main> is not safe to use in the UIProcess
authorcdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 2 Jul 2019 23:07:45 +0000 (23:07 +0000)
committercdumez@apple.com <cdumez@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 2 Jul 2019 23:07:45 +0000 (23:07 +0000)
https://bugs.webkit.org/show_bug.cgi?id=199420
<rdar://problem/52289717>

Reviewed by Ryosuke Niwa.

Source/WebKit:

Update IPC::Connection and DeviceIdHashSaltStorage to use DestructionThread::MainRunLoop
instead of DestructionThread::Main, since both classes are used in the UIProcess.

Using DestructionThread::Main is not safe in the UIProcess because its implementation relies
on isMainThread() / callOnMainThread(). Those get confused about which thread is the main
thread when an application uses both WK1 and WK2.

* Platform/IPC/Connection.h:
* UIProcess/DeviceIdHashSaltStorage.h:

Source/WTF:

* wtf/MainThread.cpp:
(WTF::isMainRunLoop):
(WTF::callOnMainRunLoop):
* wtf/MainThread.h:
Add some function to MainThread.h to be used by ThreadSafeRefCounted to interact with the
main RunLoop. This is used to avoid a circular dependency between RunLoop (which is
ThreadSafeRefCounted) and ThreadSafeReCounted.

* wtf/ThreadSafeRefCounted.h:
(WTF::ThreadSafeRefCounted::deref const):
Add a new DestructionThread::MainRunLoop enum value to be used by classes that need to
be destroyed on the main RunLoop rather than the main thread (which may be different
when WK1 is invoved)

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

Source/WTF/ChangeLog
Source/WTF/wtf/MainThread.cpp
Source/WTF/wtf/MainThread.h
Source/WTF/wtf/ThreadSafeRefCounted.h
Source/WebKit/ChangeLog
Source/WebKit/Platform/IPC/Connection.h
Source/WebKit/UIProcess/DeviceIdHashSaltStorage.h

index 6bbace6..984cbef 100644 (file)
@@ -1,3 +1,25 @@
+2019-07-02  Chris Dumez  <cdumez@apple.com>
+
+        ThreadSafeRefCounted<DestructionThread::Main> is not safe to use in the UIProcess
+        https://bugs.webkit.org/show_bug.cgi?id=199420
+        <rdar://problem/52289717>
+
+        Reviewed by Ryosuke Niwa.
+
+        * wtf/MainThread.cpp:
+        (WTF::isMainRunLoop):
+        (WTF::callOnMainRunLoop):
+        * wtf/MainThread.h:
+        Add some function to MainThread.h to be used by ThreadSafeRefCounted to interact with the
+        main RunLoop. This is used to avoid a circular dependency between RunLoop (which is
+        ThreadSafeRefCounted) and ThreadSafeReCounted.
+
+        * wtf/ThreadSafeRefCounted.h:
+        (WTF::ThreadSafeRefCounted::deref const):
+        Add a new DestructionThread::MainRunLoop enum value to be used by classes that need to
+        be destroyed on the main RunLoop rather than the main thread (which may be different
+        when WK1 is invoved)
+
 2019-07-02  Robin Morisset  <rmorisset@apple.com>
 
         [WHLSL] the initializer in VariableDeclaration should be a std::unique_ptr, not Optional<UniqueRef<..>>
index fce7b46..9edd8ef 100644 (file)
@@ -35,6 +35,7 @@
 #include <wtf/Lock.h>
 #include <wtf/MonotonicTime.h>
 #include <wtf/NeverDestroyed.h>
+#include <wtf/RunLoop.h>
 #include <wtf/StdLibExtras.h>
 #include <wtf/ThreadSpecific.h>
 #include <wtf/Threading.h>
@@ -126,6 +127,16 @@ void dispatchFunctionsFromMainThread()
     }
 }
 
+bool isMainRunLoop()
+{
+    return RunLoop::isMain();
+}
+
+void callOnMainRunLoop(Function<void()>&& function)
+{
+    RunLoop::main().dispatch(WTFMove(function));
+}
+
 void callOnMainThread(Function<void()>&& function)
 {
     ASSERT(function);
index 4e45c68..0834133 100644 (file)
@@ -57,6 +57,9 @@ WTF_EXPORT_PRIVATE bool isMainThreadIfInitialized();
 
 WTF_EXPORT_PRIVATE bool canAccessThreadLocalDataForThread(Thread&);
 
+WTF_EXPORT_PRIVATE bool isMainRunLoop();
+WTF_EXPORT_PRIVATE void callOnMainRunLoop(Function<void()>&&);
+
 #if USE(WEB_THREAD)
 WTF_EXPORT_PRIVATE bool isWebThread();
 WTF_EXPORT_PRIVATE bool isUIThread();
index c4e809a..8a624b2 100644 (file)
@@ -64,7 +64,7 @@ private:
     mutable std::atomic<unsigned> m_refCount { 1 };
 };
 
-enum class DestructionThread { Any, Main };
+enum class DestructionThread { Any, Main, MainRunLoop };
 
 template<class T, DestructionThread destructionThread = DestructionThread::Any> class ThreadSafeRefCounted : public ThreadSafeRefCountedBase {
 public:
@@ -72,13 +72,27 @@ public:
     {
         if (!derefBase())
             return;
-        if (destructionThread == DestructionThread::Any || isMainThread()) {
+
+        auto deleteThis = [this] {
             delete static_cast<const T*>(this);
-            return;
+        };
+        switch (destructionThread) {
+        case DestructionThread::Any:
+            break;
+        case DestructionThread::Main:
+            if (!isMainThread()) {
+                callOnMainThread(WTFMove(deleteThis));
+                return;
+            }
+            break;
+        case DestructionThread::MainRunLoop:
+            if (!isMainRunLoop()) {
+                callOnMainRunLoop(WTFMove(deleteThis));
+                return;
+            }
+            break;
         }
-        callOnMainThread([this] {
-            delete static_cast<const T*>(this);
-        });
+        deleteThis();
     }
 
 protected:
index fed13cc..467043f 100644 (file)
@@ -1,3 +1,21 @@
+2019-07-02  Chris Dumez  <cdumez@apple.com>
+
+        ThreadSafeRefCounted<DestructionThread::Main> is not safe to use in the UIProcess
+        https://bugs.webkit.org/show_bug.cgi?id=199420
+        <rdar://problem/52289717>
+
+        Reviewed by Ryosuke Niwa.
+
+        Update IPC::Connection and DeviceIdHashSaltStorage to use DestructionThread::MainRunLoop
+        instead of DestructionThread::Main, since both classes are used in the UIProcess.
+
+        Using DestructionThread::Main is not safe in the UIProcess because its implementation relies
+        on isMainThread() / callOnMainThread(). Those get confused about which thread is the main
+        thread when an application uses both WK1 and WK2.
+
+        * Platform/IPC/Connection.h:
+        * UIProcess/DeviceIdHashSaltStorage.h:
+
 2019-07-02  Patrick Griffis  <pgriffis@igalia.com>
 
         [GTK][WPE] Explicitly blacklist problematic directories for sandbox
index 3edf82a..4de0687 100644 (file)
@@ -90,7 +90,7 @@ template<typename AsyncReplyResult> struct AsyncReplyError {
 class MachMessage;
 class UnixMessage;
 
-class Connection : public ThreadSafeRefCounted<Connection, WTF::DestructionThread::Main> {
+class Connection : public ThreadSafeRefCounted<Connection, WTF::DestructionThread::MainRunLoop> {
 public:
     class Client : public MessageReceiver {
     public:
index 28caa7c..72cfa7b 100644 (file)
@@ -36,7 +36,7 @@
 
 namespace WebKit {
 
-class DeviceIdHashSaltStorage : public ThreadSafeRefCounted<DeviceIdHashSaltStorage, WTF::DestructionThread::Main> {
+class DeviceIdHashSaltStorage : public ThreadSafeRefCounted<DeviceIdHashSaltStorage, WTF::DestructionThread::MainRunLoop> {
 public:
     static Ref<DeviceIdHashSaltStorage> create(const String& deviceIdHashSaltStorageDirectory);
     ~DeviceIdHashSaltStorage();