Summary: | Workaround a thread library bug where thread destructors may not get called | ||||||||
---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mark Lam <mark.lam> | ||||||
Component: | JavaScriptCore | Assignee: | Mark Lam <mark.lam> | ||||||
Status: | RESOLVED FIXED | ||||||||
Severity: | Normal | CC: | fpizlo, ggaren, mmirman, msaboff, oliver, webkit-bug-importer | ||||||
Priority: | P2 | Keywords: | InRadar | ||||||
Version: | 528+ (Nightly build) | ||||||||
Hardware: | Unspecified | ||||||||
OS: | Unspecified | ||||||||
Attachments: |
|
Description
Mark Lam
2015-02-03 13:48:53 PST
Created attachment 245965 [details]
the patch
> there's a darwin bug
Is there a Radar tracking that?
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.
Created attachment 245977 [details]
patch 2: no more race condition.
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. (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. Thanks for the review. Landed in r179576: <http://trac.webkit.org/r179576>. |