Fix crash from async callback calling Queue::Submit

An async callback which calls Queue::Submit will cause
reentrance in QueueBase::Tick and CreatePipelineAsyncTracker::Tick,
which invalidates the task queue being used by the original call,
and leads to a crash from an invalid pointer.

The Tick functions should remove the tasks from the queues before
the callbacks are called, so invalidation doesn't cause a crash.

Bug: dawn:729

Change-Id: I0d952d51040a3d1a475767400de3333a8b9b0821
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/45900
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>

src/dawn_native/CreatePipelineAsyncTracker.cpp

@@ -98,10 +98,20 @@ namespace dawn_native {
     }
 
     void CreatePipelineAsyncTracker::Tick(ExecutionSerial finishedSerial) {
+        // If a user calls Queue::Submit inside Create*PipelineAsync, then the device will be
+        // ticked, which in turns ticks the tracker, causing reentrance here. To prevent the
+        // reentrant call from invalidating mCreatePipelineAsyncTasksInFlight while in use by the
+        // first call, we remove the tasks to finish from the queue, update
+        // mCreatePipelineAsyncTasksInFlight, then run the callbacks.
+        std::vector<std::unique_ptr<CreatePipelineAsyncTaskBase>> tasks;
         for (auto& task : mCreatePipelineAsyncTasksInFlight.IterateUpTo(finishedSerial)) {
-            task->Finish(WGPUCreatePipelineAsyncStatus_Success);
+            tasks.push_back(std::move(task));
         }
         mCreatePipelineAsyncTasksInFlight.ClearUpTo(finishedSerial);
+
+        for (auto& task : tasks) {
+            task->Finish(WGPUCreatePipelineAsyncStatus_Success);
+        }
     }
 
     void CreatePipelineAsyncTracker::ClearForShutDown() {

src/dawn_native/Queue.cpp

@@ -223,10 +223,20 @@ namespace dawn_native {
     }
 
     void QueueBase::Tick(ExecutionSerial finishedSerial) {
+        // If a user calls Queue::Submit inside a task, for example in a Buffer::MapAsync callback,
+        // then the device will be ticked, which in turns ticks the queue, causing reentrance here.
+        // To prevent the reentrant call from invalidating mTasksInFlight while in use by the first
+        // call, we remove the tasks to finish from the queue, update mTasksInFlight, then run the
+        // callbacks.
+        std::vector<std::unique_ptr<TaskInFlight>> tasks;
         for (auto& task : mTasksInFlight.IterateUpTo(finishedSerial)) {
-            task->Finish();
+            tasks.push_back(std::move(task));
         }
         mTasksInFlight.ClearUpTo(finishedSerial);
+
+        for (auto& task : tasks) {
+            task->Finish();
+        }
     }
 
     void QueueBase::HandleDeviceLoss() {

src/dawn_native/null/DeviceNull.cpp

@@ -334,7 +334,13 @@ namespace dawn_native { namespace null {
     }
 
     MaybeError Queue::SubmitImpl(uint32_t, CommandBufferBase* const*) {
-        ToBackend(GetDevice())->SubmitPendingOperations();
+        Device* device = ToBackend(GetDevice());
+
+        // The Vulkan, D3D12 and Metal implementation all tick the device here,
+        // for testing purposes we should also tick in the null implementation.
+        DAWN_TRY(device->Tick());
+
+        device->SubmitPendingOperations();
         return {};
     }
 

src/tests/unittests/validation/QueueSubmitValidationTests.cpp

@@ -14,6 +14,7 @@
 
 #include "tests/unittests/validation/ValidationTest.h"
 
+#include "utils/ComboRenderPipelineDescriptor.h"
 #include "utils/WGPUHelpers.h"
 
 namespace {
@@ -129,4 +130,95 @@ namespace {
         ASSERT_DEVICE_ERROR(queue.Submit(1, &commands));
     }
 
+    // Test that submitting in a buffer mapping callback doesn't cause re-entrance problems.
+    TEST_F(QueueSubmitValidationTest, SubmitInBufferMapCallback) {
+        // Create a buffer for mapping, to run our callback.
+        wgpu::BufferDescriptor descriptor;
+        descriptor.size = 4;
+        descriptor.usage = wgpu::BufferUsage::MapWrite;
+        wgpu::Buffer buffer = device.CreateBuffer(&descriptor);
+
+        struct CallbackData {
+            wgpu::Device device;
+            wgpu::Buffer buffer;
+        } callbackData = {device, buffer};
+
+        const auto callback = [](WGPUBufferMapAsyncStatus status, void* userdata) {
+            CallbackData* data = reinterpret_cast<CallbackData*>(userdata);
+
+            data->buffer.Unmap();
+
+            wgpu::Queue queue = data->device.GetQueue();
+            queue.Submit(0, nullptr);
+        };
+
+        buffer.MapAsync(wgpu::MapMode::Write, 0, descriptor.size, callback, &callbackData);
+
+        WaitForAllOperations(device);
+    }
+
+    // Test that submitting in a render pipeline creation callback doesn't cause re-entrance
+    // problems.
+    TEST_F(QueueSubmitValidationTest, SubmitInCreateRenderPipelineAsyncCallback) {
+        struct CallbackData {
+            wgpu::Device device;
+        } callbackData = {device};
+
+        const auto callback = [](WGPUCreatePipelineAsyncStatus status, WGPURenderPipeline pipeline,
+                                 char const* message, void* userdata) {
+            CallbackData* data = reinterpret_cast<CallbackData*>(userdata);
+
+            wgpuRenderPipelineRelease(pipeline);
+
+            wgpu::Queue queue = data->device.GetQueue();
+            queue.Submit(0, nullptr);
+        };
+
+        wgpu::ShaderModule vsModule = utils::CreateShaderModule(device, R"(
+            [[builtin(position)]] var<out> Position : vec4<f32>;
+            [[stage(vertex)]] fn main() -> void {
+                Position = vec4<f32>(0.0, 0.0, 0.0, 1.0);
+            })");
+
+        wgpu::ShaderModule fsModule = utils::CreateShaderModule(device, R"(
+            [[location(0)]] var<out> fragColor : vec4<f32>;
+            [[stage(fragment)]] fn main() -> void {
+                fragColor = vec4<f32>(0.0, 1.0, 0.0, 1.0);
+            })");
+
+        utils::ComboRenderPipelineDescriptor2 descriptor;
+        descriptor.vertex.module = vsModule;
+        descriptor.cFragment.module = fsModule;
+        device.CreateRenderPipelineAsync(&descriptor, callback, &callbackData);
+
+        WaitForAllOperations(device);
+    }
+
+    // Test that submitting in a compute pipeline creation callback doesn't cause re-entrance
+    // problems.
+    TEST_F(QueueSubmitValidationTest, SubmitInCreateComputePipelineAsyncCallback) {
+        struct CallbackData {
+            wgpu::Device device;
+        } callbackData = {device};
+
+        const auto callback = [](WGPUCreatePipelineAsyncStatus status, WGPUComputePipeline pipeline,
+                                 char const* message, void* userdata) {
+            CallbackData* data = reinterpret_cast<CallbackData*>(userdata);
+
+            wgpuComputePipelineRelease(pipeline);
+
+            wgpu::Queue queue = data->device.GetQueue();
+            queue.Submit(0, nullptr);
+        };
+
+        wgpu::ComputePipelineDescriptor descriptor;
+        descriptor.computeStage.module = utils::CreateShaderModule(device, R"(
+            [[stage(compute)]] fn main() -> void {
+            })");
+        descriptor.computeStage.entryPoint = "main";
+        device.CreateComputePipelineAsync(&descriptor, callback, &callbackData);
+
+        WaitForAllOperations(device);
+    }
+
 }  // anonymous namespace