[dawn][d3d] Adds loop for WaitForMultipleObjects with INFINITY.

- Adding the loop addresses the reverted workaround.
- Also includes a small fix for potential race in spontaneous
  event handling.
- Re-enables all the related tests now that they seem to be
  passing fine now on the arm64 Qualcomm devices with this fix.
- Also reverts "Fix MapAsyncAndWait use-after-free"
  commit 37905ab.

Original change's description:
> Fix MapAsyncAndWait use-after-free
>
> Fixes an issue where the callback registered in
> DawnTestBase::MapAsyncAndWait could cause an access violation during
> global test teardown due to trying to run a callback that was allocated
> on the stack and had already gone out of scope. This only occurred when
> instance.WaitAny() timed out, which would cause the callback to still
> be around until the instance got destroyed and ensured that all pending
> callbacks were complete.
>
> Now, the actual callback to run is provided via a shared_ptr and the
> lambda provided to buffer.MapAsync() checks if it is null before running
> it. After instance.WaitAny() returns but before we make any asserts, we
> set the pointer to null, preventing the soon-to-be-dangling pointer from
> being used in the future.
>
> This is meant as a temporary workaround, as WaitAny() should always run
> the provided callback. This can be reverted once the underlying issue
> is fixed.
>
> Bug: 459864803, 460743383
> Change-Id: I5384e74ff6ab9da6104b07e6d9d235bf5d4b40c2
> Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/273736
> Commit-Queue: Brian Sheedy <bsheedy@google.com>
> Reviewed-by: Loko Kung <lokokung@google.com>
> Auto-Submit: Brian Sheedy <bsheedy@google.com>
> Reviewed-by: Kai Ninomiya <kainino@chromium.org>

Bug: 459864803, 460743383, 461837996, 469328928, 465497435
Change-Id: Iabf26a66fbd6c92bf788a283c05b64d72e6f12ee
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/277975
Reviewed-by: Brian Sheedy <bsheedy@google.com>
Commit-Queue: Kai Ninomiya <kainino@chromium.org>
Auto-Submit: Loko Kung <lokokung@google.com>
Commit-Queue: Loko Kung <lokokung@google.com>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>

Author: lokokung

src/dawn/native/EventManager.cpp

@@ -317,14 +317,7 @@ FutureID EventManager::TrackEvent(Ref<TrackedEvent>&& event) {
     FutureID futureID = mNextFutureID++;
     event->mFutureID = futureID;
 
-    // Handle the event now if it's spontaneous and ready.
-    if (event->mCallbackMode == wgpu::CallbackMode::AllowSpontaneous) {
-        if (event->IsReadyToComplete()) {
-            event->EnsureComplete(EventCompletionType::Ready);
-            return futureID;
-        }
-    }
-
+    // For queue events, we schedule a task to set the event ready w.r.t the completion serial.
     if (const auto* queueAndSerial = event->GetIfQueueAndSerial()) {
         if (auto q = queueAndSerial->queue.Promote()) {
             q->TrackSerialTask(queueAndSerial->completionSerial, [this, event]() {
@@ -341,6 +334,16 @@ FutureID EventManager::TrackEvent(Ref<TrackedEvent>&& event) {
     }
 
     mEvents.Use([&](auto events) {
+        // For ready spontaneous events, handle the event now. Note that we need to do the check in
+        // a |Use| scope of |mEvents| to ensure that other threads that may reference the returned
+        // Future will reflect the proper state of the event.
+        if (event->mCallbackMode == wgpu::CallbackMode::AllowSpontaneous) {
+            if (event->IsReadyToComplete()) {
+                event->EnsureComplete(EventCompletionType::Ready);
+                return;
+            }
+        }
+
         if (!events->has_value()) {
             // We are shutting down, so if the event isn't spontaneous, call the callback now.
             if (event->mCallbackMode != wgpu::CallbackMode::AllowSpontaneous) {

src/dawn/native/WaitAnySystemEvent.h

@@ -85,16 +85,32 @@ template <typename It>
         handles.push_back((*it).first.mPrimitive.Get());
     }
     DAWN_ASSERT(handles.size() <= MAXIMUM_WAIT_OBJECTS);
-    DWORD status = WaitForMultipleObjects(handles.size(), handles.data(), /*bWaitAll=*/false,
-                                          ToMilliseconds(timeout));
-    if (status == WAIT_TIMEOUT) {
-        return false;
-    }
-    DAWN_CHECK(WAIT_OBJECT_0 <= status && status < WAIT_OBJECT_0 + count);
-    const size_t completedIndex = status - WAIT_OBJECT_0;
 
-    *(*(begin + completedIndex)).second = true;
-    return true;
+    // TODO(crbug.com/344798087): Handle the issue of timeouts in a more general way further up the
+    // stack.
+    // Note that without the looping here, we observed timeouts on arm64 Qualcomm devices. This
+    // looping for the INIFINITY timeout case is also used as a workaround in the Vulkan backend and
+    // should be sufficient for the time being because other cases, i.e. when the timeout is not
+    // INFINITY, the user code would be expected to explicitly handle the timeout case anyways.
+    DWORD status = WAIT_TIMEOUT;
+    DWORD timeoutMS = ToMilliseconds(timeout);
+    while (1) {
+        status = WaitForMultipleObjects(handles.size(), handles.data(), /*bWaitAll=*/false,
+                                        ToMilliseconds(timeout));
+        if (status == WAIT_TIMEOUT) {
+            if (timeoutMS == INFINITE) {
+                continue;
+            } else {
+                return false;
+            }
+        }
+
+        DAWN_CHECK(WAIT_OBJECT_0 <= status && status < WAIT_OBJECT_0 + count);
+        const size_t completedIndex = status - WAIT_OBJECT_0;
+
+        *(*(begin + completedIndex)).second = true;
+        return true;
+    }
 #elif DAWN_PLATFORM_IS(POSIX)
     absl::InlinedVector<pollfd, 4 /* avoid heap allocation for small waits */> pollfds;
     pollfds.reserve(count);

src/dawn/tests/DawnTest.cpp

@@ -2087,34 +2087,14 @@ void DawnTestBase::MapAsyncAndWait(const wgpu::Buffer& buffer,
     if (!UsesWire()) {
         // We use a new mock callback here so that the validation on the call happens as soon as the
         // scope of this call ends.
-        auto mockCb =
-            std::make_shared<MockCppCallback<void (*)(wgpu::MapAsyncStatus, wgpu::StringView)>>();
-        EXPECT_CALL(*mockCb, Call(wgpu::MapAsyncStatus::Success, _)).Times(1);
-
-        // TODO(crbug.com/460743383): This is a workaround for teardown causing WaitAny to return
-        // without calling the callback. Revert this to the state before
-        // https://dawn-review.googlesource.com/c/dawn/+/273736 when this is fixed.
-        // The mock callback is local to this function, but the async map request can live longer
-        // if the test times out. To prevent a use-after-free, we use a shared pointer to
-        // control the mock callback's availability.
-        wgpu::WaitStatus status = instance.WaitAny(
-            buffer.MapAsync(mapMode, offset, size, wgpu::CallbackMode::WaitAnyOnly,
-                            [mockCb](wgpu::MapAsyncStatus status, wgpu::StringView message) {
-                                if (mockCb != nullptr) {
-                                    mockCb->Callback()(status, message);
-                                }
-                            }),
-            UINT64_MAX);
-
-        // Disarm the callback. Since we need to verify expectations on the
-        // callback after disarming it, swap with a nullptr instead of simply
-        // setting it to null directly.
-        auto swappedCb =
-            std::make_shared<MockCppCallback<void (*)(wgpu::MapAsyncStatus, wgpu::StringView)>>();
-        swappedCb.reset();
-        mockCb.swap(swappedCb);
-        testing::Mock::VerifyAndClearExpectations(swappedCb.get());
-        ASSERT_EQ(status, wgpu::WaitStatus::Success);
+        MockCppCallback<void (*)(wgpu::MapAsyncStatus, wgpu::StringView)> mockCb;
+        EXPECT_CALL(mockCb, Call(wgpu::MapAsyncStatus::Success, _)).Times(1);
+
+        ASSERT_EQ(
+            instance.WaitAny(buffer.MapAsync(mapMode, offset, size, wgpu::CallbackMode::WaitAnyOnly,
+                                             mockCb.Callback()),
+                             UINT64_MAX),
+            wgpu::WaitStatus::Success);
     } else {
         bool done = false;
         buffer.MapAsync(mapMode, offset, size, wgpu::CallbackMode::AllowProcessEvents,

src/dawn/tests/end2end/BufferTests.cpp

@@ -125,11 +125,6 @@ void CheckMapping(const void* actual, const void* expected, size_t size) {
 
 // Test that the simplest map read works
 TEST_P(BufferMappingTests, MapRead_Basic) {
-    // TODO(crbug.com/469328928, crbug.com/465497435): Flakily times out on
-    // Snapdragon X Elite SoCs, suspected of causing a crash in global test
-    // teardown as a result.
-    DAWN_SUPPRESS_TEST_IF(IsWindows() && IsQualcomm());
-
     wgpu::Buffer buffer = CreateMapReadBuffer(4);
 
     const uint32_t myData = 0x01020304;
@@ -144,11 +139,6 @@ TEST_P(BufferMappingTests, MapRead_Basic) {
 
 // Test map-reading a zero-sized buffer.
 TEST_P(BufferMappingTests, MapRead_ZeroSized) {
-    // TODO(crbug.com/469328928, crbug.com/465497435): Flakily times out on
-    // Snapdragon X Elite SoCs, suspected of causing a crash in global test
-    // teardown as a result.
-    DAWN_SUPPRESS_TEST_IF(IsWindows() && IsQualcomm());
-
     wgpu::Buffer buffer = CreateMapReadBuffer(0);
 
     MapAsyncAndWait(buffer, wgpu::MapMode::Read, 0, wgpu::kWholeMapSize);
@@ -158,11 +148,6 @@ TEST_P(BufferMappingTests, MapRead_ZeroSized) {
 
 // Test map-reading with a non-zero offset
 TEST_P(BufferMappingTests, MapRead_NonZeroOffset) {
-    // TODO(crbug.com/469328928, crbug.com/465497435): Flakily times out on
-    // Snapdragon X Elite SoCs, suspected of causing a crash in global test
-    // teardown as a result.
-    DAWN_SUPPRESS_TEST_IF(IsWindows() && IsQualcomm());
-
     uint32_t myData[3] = {0x01020304, 0x05060708, 0x090A0B0C};
 
     wgpu::Buffer buffer = CreateMapReadBuffer(sizeof(myData));
@@ -180,11 +165,6 @@ TEST_P(BufferMappingTests, MapRead_NonZeroOffset) {
 
 // Map read and unmap twice. Test that both of these two iterations work.
 TEST_P(BufferMappingTests, MapRead_Twice) {
-    // TODO(crbug.com/469328928, crbug.com/465497435): Flakily times out on
-    // Snapdragon X Elite SoCs, suspected of causing a crash in global test
-    // teardown as a result.
-    DAWN_SUPPRESS_TEST_IF(IsWindows() && IsQualcomm());
-
     wgpu::Buffer buffer = CreateMapReadBuffer(4);
 
     uint32_t myData = 0x01020304;
@@ -204,11 +184,6 @@ TEST_P(BufferMappingTests, MapRead_Twice) {
 
 // Map read and test multiple get mapped range data
 TEST_P(BufferMappingTests, MapRead_MultipleMappedRange) {
-    // TODO(crbug.com/469328928, crbug.com/465497435): Flakily times out on
-    // Snapdragon X Elite SoCs, suspected of causing a crash in global test
-    // teardown as a result.
-    DAWN_SUPPRESS_TEST_IF(IsWindows() && IsQualcomm());
-
     wgpu::Buffer buffer = CreateMapReadBuffer(12);
 
     uint32_t myData[] = {0x00010203, 0x04050607, 0x08090a0b};
@@ -224,11 +199,6 @@ TEST_P(BufferMappingTests, MapRead_MultipleMappedRange) {
 
 // Test map-reading a large buffer.
 TEST_P(BufferMappingTests, MapRead_Large) {
-    // TODO(crbug.com/469328928, crbug.com/465497435): Flakily times out on
-    // Snapdragon X Elite SoCs, suspected of causing a crash in global test
-    // teardown as a result.
-    DAWN_SUPPRESS_TEST_IF(IsWindows() && IsQualcomm());
-
     constexpr uint32_t kDataSize = 1000 * 1000;
     constexpr size_t kByteSize = kDataSize * sizeof(uint32_t);
     wgpu::Buffer buffer = CreateMapReadBuffer(kByteSize);
@@ -264,11 +234,6 @@ TEST_P(BufferMappingTests, MapRead_Large) {
 
 // Test that GetConstMappedRange works inside map-read callback
 TEST_P(BufferMappingTests, MapRead_InCallback) {
-    // TODO(crbug.com/469328928, crbug.com/465497435): Flakily times out on
-    // Snapdragon X Elite SoCs, suspected of causing a crash in global test
-    // teardown as a result.
-    DAWN_SUPPRESS_TEST_IF(IsWindows() && IsQualcomm());
-
     constexpr size_t kBufferSize = 12;
     wgpu::Buffer buffer = CreateMapReadBuffer(kBufferSize);
 
@@ -1373,10 +1338,6 @@ class BufferMapExtendedUsagesTests : public DawnTest {
         DAWN_TEST_UNSUPPORTED_IF(UsesWire());
         // Skip all tests if the required feature is not supported.
         DAWN_TEST_UNSUPPORTED_IF(!SupportsFeatures({wgpu::FeatureName::BufferMapExtendedUsages}));
-
-        // TODO(crbug.com/465167911): Flakily gets unexpected nullptrs on
-        // Snapdragon X Elite SoCs.
-        DAWN_SUPPRESS_TEST_IF(IsWindows() && IsQualcomm());
     }
 
     std::vector<wgpu::FeatureName> GetRequiredFeatures() override {

src/dawn/tests/end2end/DepthStencilCopyTests.cpp

@@ -665,9 +665,6 @@ TEST_P(DepthCopyTests, FromNonZeroMipDepthAspect) {
 // inevitably written. So we need to make sure the original content of the buffer that's outside of
 // the copy extent is still correctly preserved.
 TEST_P(DepthCopyTests, PreserveBufferContent) {
-    // TODO(crbug.com/461837996): Flaky on Snapdragon X Elite.
-    DAWN_SUPPRESS_TEST_IF(IsWindows() && IsQualcomm());
-
     constexpr uint32_t kBufferCopyOffsets[] = {0u, 4u, 512u};
     constexpr uint32_t kTestTextureSizes[][2] = {
         {1, 1},
@@ -1220,9 +1217,6 @@ TEST_P(StencilCopyTests, FromNonZeroMipStencilAspect) {
 // inevitably written. So we need to make sure the original content of the buffer that's outside of
 // the copy extent is still correctly preserved.
 TEST_P(StencilCopyTests, PreserveBufferContent) {
-    // TODO(crbug.com/461837996): Flaky on Snapdragon X Elite.
-    DAWN_SUPPRESS_TEST_IF(IsWindows() && IsQualcomm());
-
     constexpr uint32_t kBufferCopyOffsets[] = {0u, 4u, 512u};
     constexpr uint32_t kTestTextureSizes[][2] = {
         {1, 1},