...
Created attachment 397512 [details] Patch
<rdar://problem/61609690>
Created attachment 397513 [details] Patch
Comment on attachment 397513 [details] Patch r=me
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 on attachment 397513 [details] Patch crazy condition. LGTM too
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 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 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.
Committed r260682: <https://trac.webkit.org/changeset/260682>