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 objects that are incorrectly collected.
<rdar://problem/18736465>
<rdar://problem/20922230>
Created attachment 252987 [details] the patch. Uploading the patch for testing. Not ready for review yet.
Created attachment 252991 [details] patch 2: fixed Windows build problem.
Created attachment 253001 [details] patch 3: Fixed a ChangeLog comment. It's ready for review now.
Comment on attachment 253001 [details] patch 3: Fixed a ChangeLog comment. View in context: https://bugs.webkit.org/attachment.cgi?id=253001&action=review > Source/JavaScriptCore/ChangeLog:20 > + handle as the source handle in a call to the DuplicateHandle function.â non-ASCII character
Thanks for the review. Landed in r184229: <http://trac.webkit.org/r184229>.
Comment on attachment 253001 [details] patch 3: Fixed a ChangeLog comment. View in context: https://bugs.webkit.org/attachment.cgi?id=253001&action=review > Source/JavaScriptCore/heap/MachineStackMarker.cpp:246 > +#if OS(WINDOWS) > + HANDLE platformThreadHandle; > +#endif Should we change the type of PlatformThread in WTF? Might other uses of PlatformThread have this bug?
(In reply to comment #10) > Comment on attachment 253001 [details] > patch 3: Fixed a ChangeLog comment. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=253001&action=review > > > Source/JavaScriptCore/heap/MachineStackMarker.cpp:246 > > +#if OS(WINDOWS) > > + HANDLE platformThreadHandle; > > +#endif > > Should we change the type of PlatformThread in WTF? Might other uses of > PlatformThread have this bug? I just did a grep throughout Source and didn’t see any references to any PlatformThread other than in MachineStackMarker. In WTF, there’s a PlatformThreadSpecificKey, but that’s a different mechanism. I also grep’ed for uses of the problematic GetCurrentThread(), and the only other use of it is in WTF/wtf/CurrentTime.cpp, and it’s using it correctly. So, I think there’s no outstanding issue.