Bug 210995 – [WTF] allThreads registration is racy with allThreads unregistration

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 397512 [details]
Patch

[Yusuke Suzuki]

<rdar://problem/61609690>

[Yusuke Suzuki]

Created attachment 397513 [details]
Patch

[Keith Miller]

Comment on attachment 397513 [details]
Patch

r=me

[Mark Lam]

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.

[Saam Barati]

Comment on attachment 397513 [details]
Patch

crazy condition. LGTM too

[Yusuke Suzuki]

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.

[Yusuke Suzuki]

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...

[Mark Lam]

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.

[Yusuke Suzuki]

Committed r260682: <https://trac.webkit.org/changeset/260682>