[JSC] MachineThreads does not consider situation that one thread has multiple VMs

https://bugs.webkit.org/show_bug.cgi?id=169819

Reviewed by Mark Lam.

The Linux port of PlatformThread suspend/resume mechanism relies on having a thread
specific singleton thread data, and was relying on MachineThreads::Thread to be this
thread specific singleton. But because MachineThreads::Thread is not a thread specific
singleton, we can get a deadlock in the GTK port's DatabaseProcess.

This patch fixes this issue by moving per thread data from MachineThreads::Thread to
MachineThreads::ThreadData, where there will only be one instance of
MachineThreads::ThreadData per thread. Each MachineThreads::Thread will now point to
the same MachineThreads::ThreadData for any given thread.

* heap/MachineStackMarker.cpp:
(pthreadSignalHandlerSuspendResume):
(JSC::threadData):
(JSC::MachineThreads::Thread::Thread):
(JSC::MachineThreads::Thread::createForCurrentThread):
(JSC::MachineThreads::Thread::operator==):
(JSC::MachineThreads::ThreadData::ThreadData):
(JSC::MachineThreads::ThreadData::~ThreadData):
(JSC::MachineThreads::ThreadData::suspend):
(JSC::MachineThreads::ThreadData::resume):
(JSC::MachineThreads::ThreadData::getRegisters):
(JSC::MachineThreads::ThreadData::Registers::stackPointer):
(JSC::MachineThreads::ThreadData::Registers::framePointer):
(JSC::MachineThreads::ThreadData::Registers::instructionPointer):
(JSC::MachineThreads::ThreadData::Registers::llintPC):
(JSC::MachineThreads::ThreadData::freeRegisters):
(JSC::MachineThreads::ThreadData::captureStack):
(JSC::MachineThreads::tryCopyOtherThreadStacks):
(JSC::MachineThreads::Thread::~Thread): Deleted.
(JSC::MachineThreads::Thread::suspend): Deleted.
(JSC::MachineThreads::Thread::resume): Deleted.
(JSC::MachineThreads::Thread::getRegisters): Deleted.
(JSC::MachineThreads::Thread::Registers::stackPointer): Deleted.
(JSC::MachineThreads::Thread::Registers::framePointer): Deleted.
(JSC::MachineThreads::Thread::Registers::instructionPointer): Deleted.
(JSC::MachineThreads::Thread::Registers::llintPC): Deleted.
(JSC::MachineThreads::Thread::freeRegisters): Deleted.
(JSC::MachineThreads::Thread::captureStack): Deleted.
* heap/MachineStackMarker.h:
(JSC::MachineThreads::Thread::operator!=):
(JSC::MachineThreads::Thread::suspend):
(JSC::MachineThreads::Thread::resume):
(JSC::MachineThreads::Thread::getRegisters):
(JSC::MachineThreads::Thread::freeRegisters):
(JSC::MachineThreads::Thread::captureStack):
(JSC::MachineThreads::Thread::platformThread):
(JSC::MachineThreads::Thread::stackBase):
(JSC::MachineThreads::Thread::stackEnd):
* runtime/SamplingProfiler.cpp:
(JSC::FrameWalker::isValidFramePointer):
* runtime/VMTraps.cpp:
(JSC::findActiveVMAndStackBounds):


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

Author: [email protected]

Source/JavaScriptCore/ChangeLog

@@ -1,3 +1,63 @@
+2017-03-23  Yusuke Suzuki  <[email protected]>
+
+        [JSC] MachineThreads does not consider situation that one thread has multiple VMs
+        https://bugs.webkit.org/show_bug.cgi?id=169819
+
+        Reviewed by Mark Lam.
+
+        The Linux port of PlatformThread suspend/resume mechanism relies on having a thread
+        specific singleton thread data, and was relying on MachineThreads::Thread to be this
+        thread specific singleton. But because MachineThreads::Thread is not a thread specific
+        singleton, we can get a deadlock in the GTK port's DatabaseProcess.
+
+        This patch fixes this issue by moving per thread data from MachineThreads::Thread to
+        MachineThreads::ThreadData, where there will only be one instance of
+        MachineThreads::ThreadData per thread. Each MachineThreads::Thread will now point to
+        the same MachineThreads::ThreadData for any given thread.
+
+        * heap/MachineStackMarker.cpp:
+        (pthreadSignalHandlerSuspendResume):
+        (JSC::threadData):
+        (JSC::MachineThreads::Thread::Thread):
+        (JSC::MachineThreads::Thread::createForCurrentThread):
+        (JSC::MachineThreads::Thread::operator==):
+        (JSC::MachineThreads::ThreadData::ThreadData):
+        (JSC::MachineThreads::ThreadData::~ThreadData):
+        (JSC::MachineThreads::ThreadData::suspend):
+        (JSC::MachineThreads::ThreadData::resume):
+        (JSC::MachineThreads::ThreadData::getRegisters):
+        (JSC::MachineThreads::ThreadData::Registers::stackPointer):
+        (JSC::MachineThreads::ThreadData::Registers::framePointer):
+        (JSC::MachineThreads::ThreadData::Registers::instructionPointer):
+        (JSC::MachineThreads::ThreadData::Registers::llintPC):
+        (JSC::MachineThreads::ThreadData::freeRegisters):
+        (JSC::MachineThreads::ThreadData::captureStack):
+        (JSC::MachineThreads::tryCopyOtherThreadStacks):
+        (JSC::MachineThreads::Thread::~Thread): Deleted.
+        (JSC::MachineThreads::Thread::suspend): Deleted.
+        (JSC::MachineThreads::Thread::resume): Deleted.
+        (JSC::MachineThreads::Thread::getRegisters): Deleted.
+        (JSC::MachineThreads::Thread::Registers::stackPointer): Deleted.
+        (JSC::MachineThreads::Thread::Registers::framePointer): Deleted.
+        (JSC::MachineThreads::Thread::Registers::instructionPointer): Deleted.
+        (JSC::MachineThreads::Thread::Registers::llintPC): Deleted.
+        (JSC::MachineThreads::Thread::freeRegisters): Deleted.
+        (JSC::MachineThreads::Thread::captureStack): Deleted.
+        * heap/MachineStackMarker.h:
+        (JSC::MachineThreads::Thread::operator!=):
+        (JSC::MachineThreads::Thread::suspend):
+        (JSC::MachineThreads::Thread::resume):
+        (JSC::MachineThreads::Thread::getRegisters):
+        (JSC::MachineThreads::Thread::freeRegisters):
+        (JSC::MachineThreads::Thread::captureStack):
+        (JSC::MachineThreads::Thread::platformThread):
+        (JSC::MachineThreads::Thread::stackBase):
+        (JSC::MachineThreads::Thread::stackEnd):
+        * runtime/SamplingProfiler.cpp:
+        (JSC::FrameWalker::isValidFramePointer):
+        * runtime/VMTraps.cpp:
+        (JSC::findActiveVMAndStackBounds):
+
 2017-03-23  Mark Lam  <[email protected]>
 
         Clients of JSArray::tryCreateForInitializationPrivate() should do their own null checks.

Source/JavaScriptCore/heap/MachineStackMarker.cpp

@@ -33,6 +33,7 @@
 #include <setjmp.h>
 #include <stdlib.h>
 #include <wtf/MainThread.h>
+#include <wtf/NeverDestroyed.h>
 #include <wtf/StdLibExtras.h>
 
 #if OS(DARWIN)
@@ -69,14 +70,14 @@
 // We use SIGUSR2 to suspend and resume machine threads in JavaScriptCore.
 static const int SigThreadSuspendResume = SIGUSR2;
 static StaticLock globalSignalLock;
-thread_local static std::atomic<JSC::MachineThreads::Thread*> threadLocalCurrentThread;
+thread_local static std::atomic<JSC::MachineThreads::ThreadData*> threadLocalCurrentThread { nullptr };
 
 static void pthreadSignalHandlerSuspendResume(int, siginfo_t*, void* ucontext)
 {
     // Touching thread local atomic types from signal handlers is allowed.
-    JSC::MachineThreads::Thread* thread = threadLocalCurrentThread.load();
+    JSC::MachineThreads::ThreadData* threadData = threadLocalCurrentThread.load();
 
-    if (thread->suspended.load(std::memory_order_acquire)) {
+    if (threadData->suspended.load(std::memory_order_acquire)) {
         // This is signal handler invocation that is intended to be used to resume sigsuspend.
         // So this handler invocation itself should not process.
         //
@@ -88,9 +89,9 @@ static void pthreadSignalHandlerSuspendResume(int, siginfo_t*, void* ucontext)
 
     ucontext_t* userContext = static_cast<ucontext_t*>(ucontext);
 #if CPU(PPC)
-    thread->suspendedMachineContext = *userContext->uc_mcontext.uc_regs;
+    threadData->suspendedMachineContext = *userContext->uc_mcontext.uc_regs;
 #else
-    thread->suspendedMachineContext = userContext->uc_mcontext;
+    threadData->suspendedMachineContext = userContext->uc_mcontext;
 #endif
 
     // Allow suspend caller to see that this thread is suspended.
@@ -99,7 +100,7 @@ static void pthreadSignalHandlerSuspendResume(int, siginfo_t*, void* ucontext)
     //
     // And sem_post emits memory barrier that ensures that suspendedMachineContext is correctly saved.
     // http://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap04.html#tag_04_11
-    sem_post(&thread->semaphoreForSuspendResume);
+    sem_post(&threadData->semaphoreForSuspendResume);
 
     // Reaching here, SigThreadSuspendResume is blocked in this handler (this is configured by sigaction's sa_mask).
     // So before calling sigsuspend, SigThreadSuspendResume to this thread is deferred. This ensures that the handler is not executed recursively.
@@ -109,7 +110,7 @@ static void pthreadSignalHandlerSuspendResume(int, siginfo_t*, void* ucontext)
     sigsuspend(&blockedSignalSet);
 
     // Allow resume caller to see that this thread is resumed.
-    sem_post(&thread->semaphoreForSuspendResume);
+    sem_post(&threadData->semaphoreForSuspendResume);
 }
 #endif // USE(PTHREADS) && !OS(WINDOWS) && !OS(DARWIN)
 
@@ -200,18 +201,29 @@ MachineThreads::~MachineThreads()
     }
 }
 
+static MachineThreads::ThreadData* threadData()
+{
+    static NeverDestroyed<ThreadSpecific<MachineThreads::ThreadData, CanBeGCThread::True>> threadData;
+    return threadData.get();
+}
+
+MachineThreads::Thread::Thread(ThreadData* threadData)
+    : data(threadData)
+{
+    ASSERT(threadData);
+}
+
 Thread* MachineThreads::Thread::createForCurrentThread()
 {
-    auto stackBounds = wtfThreadData().stack();
-    return new Thread(currentPlatformThread(), stackBounds.origin(), stackBounds.end());
+    return new Thread(threadData());
 }
 
 bool MachineThreads::Thread::operator==(const PlatformThread& other) const
 {
 #if OS(DARWIN) || OS(WINDOWS)
-    return platformThread == other;
+    return data->platformThread == other;
 #elif USE(PTHREADS)
-    return !!pthread_equal(platformThread, other);
+    return !!pthread_equal(data->platformThread, other);
 #else
 #error Need a way to compare threads on this platform
 #endif
@@ -308,11 +320,13 @@ void MachineThreads::gatherFromCurrentThread(ConservativeRoots& conservativeRoot
     conservativeRoots.add(currentThreadState.stackTop, currentThreadState.stackOrigin, jitStubRoutines, codeBlocks);
 }
 
-MachineThreads::Thread::Thread(const PlatformThread& platThread, void* base, void* end)
-    : platformThread(platThread)
-    , stackBase(base)
-    , stackEnd(end)
+MachineThreads::ThreadData::ThreadData()
 {
+    auto stackBounds = wtfThreadData().stack();
+    platformThread = currentPlatformThread();
+    stackBase = stackBounds.origin();
+    stackEnd = stackBounds.end();
+
 #if OS(WINDOWS)
     ASSERT(platformThread == GetCurrentThreadId());
     bool isSuccessful =
@@ -345,7 +359,7 @@ MachineThreads::Thread::Thread(const PlatformThread& platThread, void* base, voi
 #endif
 }
 
-MachineThreads::Thread::~Thread()
+MachineThreads::ThreadData::~ThreadData()
 {
 #if OS(WINDOWS)
     CloseHandle(platformThreadHandle);
@@ -354,7 +368,7 @@ MachineThreads::Thread::~Thread()
 #endif
 }
 
-bool MachineThreads::Thread::suspend()
+bool MachineThreads::ThreadData::suspend()
 {
 #if OS(DARWIN)
     kern_return_t result = thread_suspend(platformThread);
@@ -391,7 +405,7 @@ bool MachineThreads::Thread::suspend()
 #endif
 }
 
-void MachineThreads::Thread::resume()
+void MachineThreads::ThreadData::resume()
 {
 #if OS(DARWIN)
     thread_resume(platformThread);
@@ -422,9 +436,9 @@ void MachineThreads::Thread::resume()
 #endif
 }
 
-size_t MachineThreads::Thread::getRegisters(Thread::Registers& registers)
+size_t MachineThreads::ThreadData::getRegisters(ThreadData::Registers& registers)
 {
-    Thread::Registers::PlatformRegisters& regs = registers.regs;
+    ThreadData::Registers::PlatformRegisters& regs = registers.regs;
 #if OS(DARWIN)
 #if CPU(X86)
     unsigned user_count = sizeof(regs)/sizeof(int);
@@ -481,7 +495,7 @@ size_t MachineThreads::Thread::getRegisters(Thread::Registers& registers)
 #endif
 }
 
-void* MachineThreads::Thread::Registers::stackPointer() const
+void* MachineThreads::ThreadData::Registers::stackPointer() const
 {
 #if OS(DARWIN) || OS(WINDOWS) || ((OS(FREEBSD) || defined(__GLIBC__)) && ENABLE(JIT))
     return MachineContext::stackPointer(regs);
@@ -505,7 +519,7 @@ void* MachineThreads::Thread::Registers::stackPointer() const
 }
 
 #if ENABLE(SAMPLING_PROFILER)
-void* MachineThreads::Thread::Registers::framePointer() const
+void* MachineThreads::ThreadData::Registers::framePointer() const
 {
 #if OS(DARWIN) || OS(WINDOWS) || (OS(FREEBSD) || defined(__GLIBC__))
     return MachineContext::framePointer(regs);
@@ -514,7 +528,7 @@ void* MachineThreads::Thread::Registers::framePointer() const
 #endif
 }
 
-void* MachineThreads::Thread::Registers::instructionPointer() const
+void* MachineThreads::ThreadData::Registers::instructionPointer() const
 {
 #if OS(DARWIN) || OS(WINDOWS) || (OS(FREEBSD) || defined(__GLIBC__))
     return MachineContext::instructionPointer(regs);
@@ -523,7 +537,7 @@ void* MachineThreads::Thread::Registers::instructionPointer() const
 #endif
 }
 
-void* MachineThreads::Thread::Registers::llintPC() const
+void* MachineThreads::ThreadData::Registers::llintPC() const
 {
     // LLInt uses regT4 as PC.
 #if OS(DARWIN) || OS(WINDOWS) || (OS(FREEBSD) || defined(__GLIBC__))
@@ -534,9 +548,9 @@ void* MachineThreads::Thread::Registers::llintPC() const
 }
 #endif // ENABLE(SAMPLING_PROFILER)
 
-void MachineThreads::Thread::freeRegisters(Thread::Registers& registers)
+void MachineThreads::ThreadData::freeRegisters(ThreadData::Registers& registers)
 {
-    Thread::Registers::PlatformRegisters& regs = registers.regs;
+    ThreadData::Registers::PlatformRegisters& regs = registers.regs;
 #if USE(PTHREADS) && !OS(WINDOWS) && !OS(DARWIN) && !((OS(FREEBSD) || defined(__GLIBC__)) && ENABLE(JIT))
     pthread_attr_destroy(&regs.attribute);
 #else
@@ -559,7 +573,7 @@ static inline int osRedZoneAdjustment()
     return redZoneAdjustment;
 }
 
-std::pair<void*, size_t> MachineThreads::Thread::captureStack(void* stackTop)
+std::pair<void*, size_t> MachineThreads::ThreadData::captureStack(void* stackTop)
 {
     char* begin = reinterpret_cast_ptr<char*>(stackBase);
     char* end = bitwise_cast<char*>(WTF::roundUpToMultipleOf<sizeof(void*)>(reinterpret_cast<uintptr_t>(stackTop)));
@@ -647,12 +661,12 @@ bool MachineThreads::tryCopyOtherThreadStacks(const AbstractLocker&, void* buffe
                 }
 
                 // Re-do the suspension to get the actual failure result for logging.
-                kern_return_t error = thread_suspend(thread->platformThread);
+                kern_return_t error = thread_suspend(thread->platformThread());
                 ASSERT(error != KERN_SUCCESS);
 
                 WTFReportError(__FILE__, __LINE__, WTF_PRETTY_FUNCTION,
                     "JavaScript garbage collection encountered an invalid thread (err 0x%x): Thread [%d/%d: %p] platformThread %p.",
-                    error, index, numberOfThreads, thread, reinterpret_cast<void*>(thread->platformThread));
+                    error, index, numberOfThreads, thread, reinterpret_cast<void*>(thread->platformThread()));
 
                 // Put the invalid thread on the threadsToBeDeleted list.
                 // We can't just delete it here because we have suspended other

Source/JavaScriptCore/heap/MachineStackMarker.h

@@ -64,14 +64,13 @@ class MachineThreads {
 
     JS_EXPORT_PRIVATE void addCurrentThread(); // Only needs to be called by clients that can use the same heap from multiple threads.
 
-    class Thread {
+    class ThreadData {
         WTF_MAKE_FAST_ALLOCATED;
-        Thread(const PlatformThread& platThread, void* base, void* end);
-
     public:
-        ~Thread();
+        ThreadData();
+        ~ThreadData();
 
-        static Thread* createForCurrentThread();
+        static ThreadData* createForCurrentThread();
 
         struct Registers {
             void* stackPointer() const;
@@ -92,20 +91,16 @@ class MachineThreads {
 #else
 #error Need a thread register struct for this platform
 #endif
-            
+
             PlatformRegisters regs;
         };
-        
-        bool operator==(const PlatformThread& other) const;
-        bool operator!=(const PlatformThread& other) const { return !(*this == other); }
 
         bool suspend();
         void resume();
         size_t getRegisters(Registers&);
         void freeRegisters(Registers&);
         std::pair<void*, size_t> captureStack(void* stackTop);
 
-        Thread* next;
         PlatformThread platformThread;
         void* stackBase;
         void* stackEnd;
@@ -119,6 +114,32 @@ class MachineThreads {
 #endif
     };
 
+    class Thread {
+        WTF_MAKE_FAST_ALLOCATED;
+        Thread(ThreadData*);
+
+    public:
+        using Registers = ThreadData::Registers;
+
+        static Thread* createForCurrentThread();
+
+        bool operator==(const PlatformThread& other) const;
+        bool operator!=(const PlatformThread& other) const { return !(*this == other); }
+
+        bool suspend() { return data->suspend(); }
+        void resume() { data->resume(); }
+        size_t getRegisters(Registers& regs) { return data->getRegisters(regs); }
+        void freeRegisters(Registers& regs) { data->freeRegisters(regs); }
+        std::pair<void*, size_t> captureStack(void* stackTop) { return data->captureStack(stackTop); }
+
+        const PlatformThread& platformThread() { return data->platformThread; }
+        void* stackBase() const { return data->stackBase; }
+        void* stackEnd() const { return data->stackEnd; }
+
+        Thread* next;
+        ThreadData* data;
+    };
+
     Lock& getLock() { return m_registeredThreadsMutex; }
     Thread* threadsListHead(const AbstractLocker&) const { ASSERT(m_registeredThreadsMutex.isLocked()); return m_registeredThreads; }
     Thread* machineThreadForCurrentThread();

Source/JavaScriptCore/runtime/SamplingProfiler.cpp

@@ -169,8 +169,8 @@ class FrameWalker {
     {
         uint8_t* fpCast = bitwise_cast<uint8_t*>(exec);
         for (MachineThreads::Thread* thread = m_vm.heap.machineThreads().threadsListHead(m_machineThreadsLocker); thread; thread = thread->next) {
-            uint8_t* stackBase = static_cast<uint8_t*>(thread->stackBase);
-            uint8_t* stackLimit = static_cast<uint8_t*>(thread->stackEnd);
+            uint8_t* stackBase = static_cast<uint8_t*>(thread->stackBase());
+            uint8_t* stackLimit = static_cast<uint8_t*>(thread->stackEnd());
             RELEASE_ASSERT(stackBase);
             RELEASE_ASSERT(stackLimit);
             if (fpCast <= stackBase && fpCast >= stackLimit)

Source/JavaScriptCore/runtime/VMTraps.cpp

@@ -111,11 +111,11 @@ static Expected<std::pair<VM*, StackBounds>, VMTraps::Error> findActiveVMAndStac
         }
 
         for (MachineThreads::Thread* thread = machineThreads.threadsListHead(machineThreadsLocker); thread; thread = thread->next) {
-            RELEASE_ASSERT(thread->stackBase);
-            RELEASE_ASSERT(thread->stackEnd);
-            if (stackPointer <= thread->stackBase && stackPointer >= thread->stackEnd) {
+            RELEASE_ASSERT(thread->stackBase());
+            RELEASE_ASSERT(thread->stackEnd());
+            if (stackPointer <= thread->stackBase() && stackPointer >= thread->stackEnd()) {
                 activeVM = &vm;
-                stackBounds = StackBounds(thread->stackBase, thread->stackEnd);
+                stackBounds = StackBounds(thread->stackBase(), thread->stackEnd());
                 return VMInspector::FunctorStatus::Done;
             }
         }