Code path Monitor::Wait that handles timeout contains a race between removing
waiter from the list and another thread signaling waiter's event:
T1: WaitForSingleObject(wait_data->event_, ...) returns with WAIT_TIMEOUT
T2: SetEvent(wait_data->event_)
T1: data_.RemoveWaiter(wait_data->event_)
This race leaves wait_data->event_ signaled, which breaks an important invariant
the method relies on: if WaitForSingleObject returns successfully (neither
timedout nor failed), that implies that wait_data was removed from the
waiters list by the method that signaled the event (SignalAndRemoveAllWaiters
or SignalAndRemoveFirstWaiter). However if wait_data->event_ is left signaled
the next invocation to WaitForSingleObject will return prior to any invocation
of SignalAndRemoveAllWaiters/SignalAndRemoveFirstWaiter, which means that
wait_data->event_ will be left in the list, which can lead to all sorts of
bugs, for example:
T1: // Assuming that wait_data WD1 for thread T1 is left with signaled event.
T1: ml.Wait() | waiters list: WD1
T1: | WaitForSingleObject(...) returns "spuriously" | waiters list: WD1
T1: // WD1 is still on the waiters list, even though |
T1 // it is not waiting anymore | waiters list: WD1
|
T2: ml.Wait() | waiters list: WD1 -> WD2
|
T1: // wait on the same monitor again |
T1: ml.Wait() | waiters list: WD1 -> WD2
T1: | GetMonitorWaitDataForThread() | waiters list: WD1 -> WD2
T1: | | wait_data->next_ = NULL | waiters list: WD1
T3: // Notify all waiters. Only T1 will wake up!
T3: ml.NotifyAll()
Notice how waiting on the same monitor caused us to corrupt the singled
linked list of waiters, because GetMonitorWaitDataForThread(...) does
wait_data->next_ = NULL - which means that if WD1 was accidentally left
in the list of waiters then all elements on that list after WD1 are lost.
This means that NotifyAll will not wake up all threads.
This was causing deadlocks in GC (#29261) by breaking ThreadBarrier logic.
Fixes https://github.com/dart-lang/sdk/issues/29261
Bug:
Change-Id: Ia61efa065bc7db8fe4bbf549032f35932356a0f4
Reviewed-on: https://dart-review.googlesource.com/20760
Reviewed-by: Zach Anderson <zra@google.com>
Commit-Queue: Vyacheslav Egorov <vegorov@google.com>
Like HOST_ARCH_*, HOST_OS_* describes the OS the VM is running on, which may be different from the OS the VM is generating code for during AOT compilation.
Currently we conflate the two when emitting AOT as assembly, and we get away with it because Flutter only uses assembly for targeting iOS and one can only target iOS from a Mac, but we expect to use assembly for Android as well so native tools can unwind Dart frames.
R=zra@google.com
Review-Url: https://codereview.chromium.org/2750843003 .
Also:
- Reaps exited threads in the thread pool before putting
a thread on the idle list so that a new arriving task
isn't blocked on a supposedly idle thread in the middle
of a join.
- Stops trying to join eventhandler threads on
Windows. Now that we're using the correct exit() call,
we probably don't have to worry about exit code pollution,
so joining the threads is unnecessary.
related #26400R=asiva@google.com, iposva@google.com
Review URL: https://codereview.chromium.org/1978153002 .
Ben Konyi