Bug 210995

Summary: [WTF] allThreads registration is racy with allThreads unregistration
Product: WebKit Reporter: Yusuke Suzuki <ysuzuki>
Component: Web Template FrameworkAssignee: Yusuke Suzuki <ysuzuki>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, ews-watchlist, keith_miller, mark.lam, sbarati, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch keith_miller: review+

Description Yusuke Suzuki 2020-04-24 14:18:13 PDT
...
Comment 1 Yusuke Suzuki 2020-04-24 14:45:02 PDT
Created attachment 397512 [details]
Patch
Comment 2 Yusuke Suzuki 2020-04-24 14:47:02 PDT
<rdar://problem/61609690>
Comment 3 Yusuke Suzuki 2020-04-24 14:50:42 PDT
Created attachment 397513 [details]
Patch
Comment 4 Keith Miller 2020-04-24 15:01:12 PDT
Comment on attachment 397513 [details]
Patch

r=me
Comment 5 Mark Lam 2020-04-24 15:10:00 PDT
Comment on attachment 397513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397513&action=review

r=me too.

> Source/WTF/ChangeLog:15
> +            5. Caller: Register the new thread to allThreads while it already finished its execution.

/Register/Registers/
/while/after/

> Source/WTF/wtf/Threading.cpp:200
> +    // However, it is also possible that the launched thread has been finished its execution before it is registered in allThreads here! In this case, the thread already

/has been finished/has finished/.
/thread already/thread has already/

> Source/WTF/wtf/Threading.cpp:201
> +    // called Thread::didExit to unregister itself from allThreads. Registering such a thread will register stale thread pointer to allThreads, which will not be removed

/register stale/register a stale/

> Source/WTF/wtf/Threading.cpp:202
> +    // even after Thread is destroyed. Register a thread only when it did not unregister itself from allThreads yet.

/when it did not unregister/when it has not unregistered/

> Source/WTF/wtf/Threading.cpp:232
> +        m_didUnregisterFromAllThreads = true;

Pity we can't use m_isShuttingDown.  So many flags to say the effectively / nearly the same thing:
    m_isShuttingDown, m_didExit, m_didUnregisterFromAllThreads

Maybe we can consider consolidating these later in another patch.
Comment 6 Saam Barati 2020-04-24 15:56:04 PDT
Comment on attachment 397513 [details]
Patch

crazy condition. LGTM too
Comment 7 Yusuke Suzuki 2020-04-24 15:57:14 PDT
Comment on attachment 397513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397513&action=review

>> Source/WTF/ChangeLog:15
>> +            5. Caller: Register the new thread to allThreads while it already finished its execution.
> 
> /Register/Registers/
> /while/after/

Fixed.

>> Source/WTF/wtf/Threading.cpp:200
>> +    // However, it is also possible that the launched thread has been finished its execution before it is registered in allThreads here! In this case, the thread already
> 
> /has been finished/has finished/.
> /thread already/thread has already/

Fixed.

>> Source/WTF/wtf/Threading.cpp:201
>> +    // called Thread::didExit to unregister itself from allThreads. Registering such a thread will register stale thread pointer to allThreads, which will not be removed
> 
> /register stale/register a stale/

Fixed.

>> Source/WTF/wtf/Threading.cpp:202
>> +    // even after Thread is destroyed. Register a thread only when it did not unregister itself from allThreads yet.
> 
> /when it did not unregister/when it has not unregistered/

Fixed.

> Source/WTF/wtf/Threading.h:315
> +    bool m_didUnregisterFromAllThreads : 1;

For now, I'll make this as separate bool flag since this is guarded bool which can be modified by multiple threads with mutex-guards, while the other bitflags are not guarded by this same mutex.
Comment 8 Yusuke Suzuki 2020-04-24 16:03:12 PDT
Comment on attachment 397513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397513&action=review

>> Source/WTF/wtf/Threading.cpp:232
>> +        m_didUnregisterFromAllThreads = true;
> 
> Pity we can't use m_isShuttingDown.  So many flags to say the effectively / nearly the same thing:
>     m_isShuttingDown, m_didExit, m_didUnregisterFromAllThreads
> 
> Maybe we can consider consolidating these later in another patch.

Not sure we can do that. They are because of complex mutex interactions & ThreadGroup unregistrations...
Comment 9 Mark Lam 2020-04-24 16:27:11 PDT
Comment on attachment 397513 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=397513&action=review

>>> Source/WTF/wtf/Threading.cpp:232
>>> +        m_didUnregisterFromAllThreads = true;
>> 
>> Pity we can't use m_isShuttingDown.  So many flags to say the effectively / nearly the same thing:
>>     m_isShuttingDown, m_didExit, m_didUnregisterFromAllThreads
>> 
>> Maybe we can consider consolidating these later in another patch.
> 
> Not sure we can do that. They are because of complex mutex interactions & ThreadGroup unregistrations...

I agree that it may not be possible, or even if possible, may not be desirable.  Perhaps the fields can be named something better.  It's not super important for now.
Comment 10 Yusuke Suzuki 2020-04-24 16:56:46 PDT
Committed r260682: <https://trac.webkit.org/changeset/260682>