Skip to site navigation (Press enter)

[webkit-changes] [184229] trunk/Source/JavaScriptCore

mark . lam Tue, 12 May 2015 18:48:07 -0700

Title: [184229] trunk/Source/_javascript_Core

Revision: 184229
Author: [email protected]
Date: 2015-05-12 18:47:15 -0700 (Tue, 12 May 2015)

Log Message

Windows: Cannot use HANDLE from GetCurrentThread() to get the CONTEXT of another thread.
https://bugs.webkit.org/show_bug.cgi?id=144924


Reviewed by Alex Christensen.

The present stack scanning code in the Windows port is expecting that the
GetCurrentThread() API will provide a unique HANDLE for each thread.  The code
then saves and later uses that HANDLE with GetThreadContext() to get the
runtime state of the target thread from the GC thread.  According to
https://msdn.microsoft.com/en-us/library/windows/desktop/ms683182(v=vs.85).aspx,
GetCurrentThread() does not provide this unique HANDLE that we expect:

    "The function cannot be used by one thread to create a handle that can
    be used by other threads to refer to the first thread. The handle is
    always interpreted as referring to the thread that is using it. A
    thread can create a "real" handle to itself that can be used by other
    threads, or inherited by other processes, by specifying the pseudo
    handle as the source handle in a call to the DuplicateHandle function."

As a result of this, GetCurrentThread() always returns the same HANDLE value, and
we end up never scanning the stacks of other threads because we wrongly think that
they are all equal (in identity) to the scanning thread.  This, in turn, results
in crashes due to objects that are incorrectly collected.

The fix is to call DuplicateHandle() to create a HANDLE that we can use.  The
MachineThreads::Thread class already accurately tracks the period of time when
we need that HANDLE for the VM.  Hence, the life-cycle of the HANDLE can be tied
to the life-cycle of the MachineThreads::Thread object for the corresponding thread.

* heap/MachineStackMarker.cpp:
(JSC::getCurrentPlatformThread):
(JSC::MachineThreads::Thread::Thread):
(JSC::MachineThreads::Thread::~Thread):
(JSC::MachineThreads::Thread::suspend):
(JSC::MachineThreads::Thread::resume):
(JSC::MachineThreads::Thread::getRegisters):

Modified Paths

trunk/Source/_javascript_Core/ChangeLog
trunk/Source/_javascript_Core/heap/MachineStackMarker.cpp

Diff

Modified: trunk/Source/_javascript_Core/ChangeLog (184228 => 184229)


--- trunk/Source/_javascript_Core/ChangeLog	2015-05-13 01:47:00 UTC (rev 184228)
+++ trunk/Source/_javascript_Core/ChangeLog	2015-05-13 01:47:15 UTC (rev 184229)
@@ -1,3 +1,42 @@
+2015-05-12  Mark Lam  <[email protected]>
+
+        Windows: Cannot use HANDLE from GetCurrentThread() to get the CONTEXT of another thread.
+        https://bugs.webkit.org/show_bug.cgi?id=144924
+
+        Reviewed by Alex Christensen.
+
+        The present stack scanning code in the Windows port is expecting that the
+        GetCurrentThread() API will provide a unique HANDLE for each thread.  The code
+        then saves and later uses that HANDLE with GetThreadContext() to get the
+        runtime state of the target thread from the GC thread.  According to
+        https://msdn.microsoft.com/en-us/library/windows/desktop/ms683182(v=vs.85).aspx,
+        GetCurrentThread() does not provide this unique HANDLE that we expect:
+
+            "The function cannot be used by one thread to create a handle that can
+            be used by other threads to refer to the first thread. The handle is
+            always interpreted as referring to the thread that is using it. A
+            thread can create a "real" handle to itself that can be used by other
+            threads, or inherited by other processes, by specifying the pseudo
+            handle as the source handle in a call to the DuplicateHandle function."
+
+        As a result of this, GetCurrentThread() always returns the same HANDLE value, and
+        we end up never scanning the stacks of other threads because we wrongly think that
+        they are all equal (in identity) to the scanning thread.  This, in turn, results
+        in crashes due to objects that are incorrectly collected.
+
+        The fix is to call DuplicateHandle() to create a HANDLE that we can use.  The
+        MachineThreads::Thread class already accurately tracks the period of time when
+        we need that HANDLE for the VM.  Hence, the life-cycle of the HANDLE can be tied
+        to the life-cycle of the MachineThreads::Thread object for the corresponding thread.
+
+        * heap/MachineStackMarker.cpp:
+        (JSC::getCurrentPlatformThread):
+        (JSC::MachineThreads::Thread::Thread):
+        (JSC::MachineThreads::Thread::~Thread):
+        (JSC::MachineThreads::Thread::suspend):
+        (JSC::MachineThreads::Thread::resume):
+        (JSC::MachineThreads::Thread::getRegisters):
+
 2015-05-12  Benjamin Poulain  <[email protected]>
 
         [JSC] Make the NegZero backward propagated flags of ArithMod stricter

Modified: trunk/Source/_javascript_Core/heap/MachineStackMarker.cpp (184228 => 184229)


--- trunk/Source/_javascript_Core/heap/MachineStackMarker.cpp	2015-05-13 01:47:00 UTC (rev 184228)
+++ trunk/Source/_javascript_Core/heap/MachineStackMarker.cpp	2015-05-13 01:47:15 UTC (rev 184229)
@@ -72,7 +72,7 @@
 #if OS(DARWIN)
 typedef mach_port_t PlatformThread;
 #elif OS(WINDOWS)
-typedef HANDLE PlatformThread;
+typedef DWORD PlatformThread;
 #elif USE(PTHREADS)
 typedef pthread_t PlatformThread;
 static const int SigThreadSuspendResume = SIGUSR2;
@@ -151,7 +151,7 @@
 #if OS(DARWIN)
     return pthread_mach_thread_np(pthread_self());
 #elif OS(WINDOWS)
-    return GetCurrentThread();
+    return GetCurrentThreadId();
 #elif USE(PTHREADS)
     return pthread_self();
 #endif
@@ -176,10 +176,23 @@
         sigemptyset(&mask);
         sigaddset(&mask, SigThreadSuspendResume);
         pthread_sigmask(SIG_UNBLOCK, &mask, 0);
+#elif OS(WINDOWS)
+        ASSERT(platformThread == GetCurrentThreadId());
+        bool isSuccessful =
+            DuplicateHandle(GetCurrentProcess(), GetCurrentThread(), GetCurrentProcess(),
+                &platformThreadHandle, 0, FALSE, DUPLICATE_SAME_ACCESS);
+        RELEASE_ASSERT(isSuccessful);
 #endif
     }
 
 public:
+    ~Thread()
+    {
+#if OS(WINDOWS)
+        CloseHandle(platformThreadHandle);
+#endif
+    }
+
     static Thread* createForCurrentThread()
     {
         return new Thread(getCurrentPlatformThread(), wtfThreadData().stack().origin());
@@ -228,6 +241,9 @@
     Thread* next;
     PlatformThread platformThread;
     void* stackBase;
+#if OS(WINDOWS)
+    HANDLE platformThreadHandle;
+#endif
 };
 
 MachineThreads::MachineThreads(Heap* heap)
@@ -335,7 +351,7 @@
     kern_return_t result = thread_suspend(platformThread);
     return result == KERN_SUCCESS;
 #elif OS(WINDOWS)
-    bool threadIsSuspended = (SuspendThread(platformThread) != (DWORD)-1);
+    bool threadIsSuspended = (SuspendThread(platformThreadHandle) != (DWORD)-1);
     ASSERT(threadIsSuspended);
     return threadIsSuspended;
 #elif USE(PTHREADS)
@@ -351,7 +367,7 @@
 #if OS(DARWIN)
     thread_resume(platformThread);
 #elif OS(WINDOWS)
-    ResumeThread(platformThread);
+    ResumeThread(platformThreadHandle);
 #elif USE(PTHREADS)
     pthread_kill(platformThread, SigThreadSuspendResume);
 #else
@@ -396,7 +412,7 @@
 
 #elif OS(WINDOWS)
     regs.ContextFlags = CONTEXT_INTEGER | CONTEXT_CONTROL;
-    GetThreadContext(platformThread, &regs);
+    GetThreadContext(platformThreadHandle, &regs);
     return sizeof(CONTEXT);
 #elif USE(PTHREADS)
     pthread_attr_init(&regs);

_______________________________________________
webkit-changes mailing list
[email protected]
https://lists.webkit.org/mailman/listinfo/webkit-changes