Bug 210995

Summary:

[WTF] allThreads registration is racy with allThreads unregistration

Product:

WebKit

Reporter:

Yusuke Suzuki <ysuzuki>

Component:

Web Template Framework

Assignee:

Yusuke Suzuki <ysuzuki>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

benjamin, cdumez, cmarcelo, ews-watchlist, keith_miller, mark.lam, saam, webkit-bug-importer

Priority:

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>