RESOLVED FIXED 141209
Workaround a thread library bug where thread destructors may not get called
https://bugs.webkit.org/show_bug.cgi?id=141209
Summary Workaround a thread library bug where thread destructors may not get called
Mark Lam
Reported 2015-02-03 13:48:53 PST
As far as we know, there's a darwin bug where thread destructors may not get called. We will work around this by checking at GC time if the platform thread is still valid. If not, we'll purge it from the VM's registeredThreads list before proceeding with thread scanning activity.
Attachments
the patch (6.01 KB, patch)
2015-02-03 14:02 PST, Mark Lam
msaboff: review-
patch 2: no more race condition. (8.80 KB, patch)
2015-02-03 15:36 PST, Mark Lam
msaboff: review+
Mark Lam
Comment 1 2015-02-03 13:51:37 PST
Mark Lam
Comment 2 2015-02-03 14:02:04 PST
Created attachment 245965 [details] the patch
Alexey Proskuryakov
Comment 3 2015-02-03 14:17:42 PST
> there's a darwin bug Is there a Radar tracking that?
Michael Saboff
Comment 4 2015-02-03 14:29:58 PST
Comment on attachment 245965 [details] the patch r- I believe that there is a race condition. Consider thread A is exiting and it has already called MachineThreads::removeThread() but hasn't really exited. Then thread B executes this code and doesn't purge thread A. After completing this code, thread A really exits and then we call tryCopyOtherThreadStacks(). We'll still try to get thread stack info from the now dead A.
Mark Lam
Comment 5 2015-02-03 15:36:21 PST
Created attachment 245977 [details] patch 2: no more race condition.
Michael Saboff
Comment 6 2015-02-03 15:48:42 PST
Comment on attachment 245977 [details] patch 2: no more race condition. View in context: https://bugs.webkit.org/attachment.cgi?id=245977&action=review r=me with a couple of minor changes. > Source/JavaScriptCore/ChangeLog:15 > + suspension, because the thread is only guaranteed to be alive while it Might want to change "the thread" to "another thread". > Source/JavaScriptCore/heap/MachineStackMarker.cpp:194 > +void MachineThreads::removeThreadWithLockAlreadyAcquired(PlatformThread platformThread) threadToRemove seems like a better name for the argument.
Mark Lam
Comment 7 2015-02-03 15:55:43 PST
(In reply to comment #6) > Comment on attachment 245977 [details] > patch 2: no more race condition. > > View in context: > https://bugs.webkit.org/attachment.cgi?id=245977&action=review > > r=me with a couple of minor changes. > > > Source/JavaScriptCore/ChangeLog:15 > > + suspension, because the thread is only guaranteed to be alive while it > > Might want to change "the thread" to "another thread". Will change the wording to me more clear that it the another thread whose validity we're checking. > > Source/JavaScriptCore/heap/MachineStackMarker.cpp:194 > > +void MachineThreads::removeThreadWithLockAlreadyAcquired(PlatformThread platformThread) > > threadToRemove seems like a better name for the argument. Changed.
Mark Lam
Comment 8 2015-02-03 16:01:04 PST
Thanks for the review. Landed in r179576: <http://trac.webkit.org/r179576>.
Note You need to log in before you can comment on or make changes to this bug.