Bug 141209 – Workaround a thread library bug where thread destructors may not get called

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Mark Lam]

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.

[Mark Lam]

<rdar://problem/17979784>

[Mark Lam]

Created attachment 245965 [details]
the patch

[Alexey Proskuryakov]

> there's a darwin bug

Is there a Radar tracking that?

[Michael Saboff]

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]

Created attachment 245977 [details]
patch 2: no more race condition.

[Michael Saboff]

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]

(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]

Thanks for the review.  Landed in r179576: <http://trac.webkit.org/r179576>.