Expose device.destroy upwards and add end2end tests

Bug: dawn:628
Change-Id: I0820d6855ac928c25f5720a2ccf0f21ae3f88d79
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/68120
Commit-Queue: Loko Kung <lokokung@google.com>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Austin Eng <enga@chromium.org>

dawn.json

@@ -1016,9 +1016,7 @@
                 ]
             },
             {
-                "name": "destroy",
+                "name": "destroy"
-                "_TODO": "crbug.com/dawn/628: Implement in Dawn",
-                "tags": ["upstream"]
             },
             {
                 "name": "get limits",

src/dawn_native/Device.cpp

@@ -312,8 +312,20 @@ namespace dawn_native {
     }
 
     void DeviceBase::Destroy() {
+        // Skip if we are already destroyed.
+        if (mState == State::Destroyed) {
+            return;
+        }
+
         // Skip handling device facilities if they haven't even been created (or failed doing so)
         if (mState != State::BeingCreated) {
+            // The device is being destroyed so it will be lost, call the application callback.
+            if (mDeviceLostCallback != nullptr) {
+                mDeviceLostCallback(WGPUDeviceLostReason_Destroyed, "Device was destroyed.",
+                                    mDeviceLostUserdata);
+                mDeviceLostCallback = nullptr;
+            }
+
             // Call all the callbacks immediately as the device is about to shut down.
             // TODO(crbug.com/dawn/826): Cancel the tasks that are in flight if possible.
             mAsyncTaskManager->WaitAllPendingTasks();
@@ -346,15 +358,21 @@ namespace dawn_native {
 
             case State::Disconnected:
                 break;
+
+            case State::Destroyed:
+                // If we are already destroyed we should've skipped this work entirely.
+                UNREACHABLE();
+                break;
         }
         ASSERT(mCompletedSerial == mLastSubmittedSerial);
         ASSERT(mFutureSerial <= mCompletedSerial);
 
         if (mState != State::BeingCreated) {
             // The GPU timeline is finished.
-            // Tick the queue-related tasks since they should be complete. This must be done before
+            // Finish destroying all objects owned by the device and tick the queue-related tasks
-            // DestroyImpl() it may relinquish resources that will be freed by backends in the
+            // since they should be complete. This must be done before DestroyImpl() it may
-            // DestroyImpl() call.
+            // relinquish resources that will be freed by backends in the DestroyImpl() call.
+            DestroyObjects();
             mQueue->Tick(GetCompletedCommandSerial());
             // Call TickImpl once last time to clean up resources
             // Ignore errors so that we can continue with destruction
@@ -362,6 +380,8 @@ namespace dawn_native {
         }
 
         // At this point GPU operations are always finished, so we are in the disconnected state.
+        // Note that currently this state change is required because some of the backend
+        // implementations of DestroyImpl checks that we are disconnected before doing work.
         mState = State::Disconnected;
 
         mDynamicUploader = nullptr;
@@ -373,12 +393,15 @@ namespace dawn_native {
 
         AssumeCommandsComplete();
 
-        // Now that the GPU timeline is empty, destroy all objects owned by the device, and then the
+        // Now that the GPU timeline is empty, destroy the backend device.
-        // backend device.
-        DestroyObjects();
         DestroyImpl();
 
         mCaches = nullptr;
+        mState = State::Destroyed;
+    }
+
+    void DeviceBase::APIDestroy() {
+        Destroy();
     }
 
     void DeviceBase::HandleError(InternalErrorType type, const char* message) {
@@ -421,8 +444,6 @@ namespace dawn_native {
         if (type == InternalErrorType::DeviceLost) {
             // The device was lost, call the application callback.
             if (mDeviceLostCallback != nullptr) {
-                // TODO(crbug.com/dawn/628): Make sure the "Destroyed" reason is passed if
-                // the device was destroyed.
                 mDeviceLostCallback(WGPUDeviceLostReason_Undefined, message, mDeviceLostUserdata);
                 mDeviceLostCallback = nullptr;
             }
@@ -461,6 +482,9 @@ namespace dawn_native {
         // resetting) the resources pointed by such pointer may be freed. Flush all deferred
         // callback tasks to guarantee we are never going to use the previous callback after
         // this call.
+        if (IsLost()) {
+            return;
+        }
         FlushCallbackTaskQueue();
         mLoggingCallback = callback;
         mLoggingUserdata = userdata;
@@ -472,6 +496,9 @@ namespace dawn_native {
         // resetting) the resources pointed by such pointer may be freed. Flush all deferred
         // callback tasks to guarantee we are never going to use the previous callback after
         // this call.
+        if (IsLost()) {
+            return;
+        }
         FlushCallbackTaskQueue();
         mUncapturedErrorCallback = callback;
         mUncapturedErrorUserdata = userdata;
@@ -483,6 +510,9 @@ namespace dawn_native {
         // resetting) the resources pointed by such pointer may be freed. Flush all deferred
         // callback tasks to guarantee we are never going to use the previous callback after
         // this call.
+        if (IsLost()) {
+            return;
+        }
         FlushCallbackTaskQueue();
         mDeviceLostCallback = callback;
         mDeviceLostUserdata = userdata;

src/dawn_native/Device.h

@@ -297,11 +297,13 @@ namespace dawn_native {
         //     Disconnected)
         //   - Disconnected: there is no longer work happening on the GPU timeline and the CPU data
         //     structures can be safely destroyed without additional synchronization.
+        //   - Destroyed: the device is disconnected and resources have been reclaimed.
         enum class State {
             BeingCreated,
             Alive,
             BeingDisconnected,
             Disconnected,
+            Destroyed,
         };
         State GetState() const;
         bool IsLost() const;
@@ -365,6 +367,7 @@ namespace dawn_native {
 
         const std::string& GetLabel() const;
         void APISetLabel(const char* label);
+        void APIDestroy();
 
       protected:
         // Constructor used only for mocking and testing.

src/tests/DawnTest.cpp

@@ -1007,6 +1007,9 @@ void DawnTestBase::TearDown() {
         EXPECT_EQ(mLastWarningCount,
                   dawn_native::GetDeprecationWarningCountForTesting(device.Get()));
     }
+
+    // The device will be destroyed soon after, so we want to set the expectation.
+    ExpectDeviceDestruction();
 }
 
 void DawnTestBase::StartExpectDeviceError() {
@@ -1018,6 +1021,10 @@ bool DawnTestBase::EndExpectDeviceError() {
     return mError;
 }
 
+void DawnTestBase::ExpectDeviceDestruction() {
+    mExpectDestruction = true;
+}
+
 // static
 void DawnTestBase::OnDeviceError(WGPUErrorType type, const char* message, void* userdata) {
     ASSERT(type != WGPUErrorType_NoError);
@@ -1029,9 +1036,14 @@ void DawnTestBase::OnDeviceError(WGPUErrorType type, const char* message, void*
 }
 
 void DawnTestBase::OnDeviceLost(WGPUDeviceLostReason reason, const char* message, void* userdata) {
+    DawnTestBase* self = static_cast<DawnTestBase*>(userdata);
+    if (self->mExpectDestruction) {
+        EXPECT_EQ(reason, WGPUDeviceLostReason_Destroyed);
+        return;
+    }
     // Using ADD_FAILURE + ASSERT instead of FAIL to prevent the current test from continuing with a
     // corrupt state.
-    ADD_FAILURE() << "Device Lost during test: " << message;
+    ADD_FAILURE() << "Device lost during test: " << message;
     ASSERT(false);
 }
 

src/tests/DawnTest.h

@@ -308,6 +308,8 @@ class DawnTestBase {
     void StartExpectDeviceError();
     bool EndExpectDeviceError();
 
+    void ExpectDeviceDestruction();
+
     bool HasVendorIdFilter() const;
     uint32_t GetVendorIdFilter() const;
 
@@ -505,6 +507,7 @@ class DawnTestBase {
     static void OnDeviceLost(WGPUDeviceLostReason reason, const char* message, void* userdata);
     bool mExpectError = false;
     bool mError = false;
+    bool mExpectDestruction = false;
 
     std::ostringstream& AddTextureExpectationImpl(const char* file,
                                                   int line,

src/tests/end2end/CreatePipelineAsyncTests.cpp

@@ -417,6 +417,66 @@ TEST_P(CreatePipelineAsyncTest, ReleaseDeviceBeforeCallbackOfCreateRenderPipelin
         &task);
 }
 
+// Verify there is no error when the device is destroyed before the callback of
+// CreateComputePipelineAsync() is called.
+TEST_P(CreatePipelineAsyncTest, DestroyDeviceBeforeCallbackOfCreateComputePipelineAsync) {
+    wgpu::ComputePipelineDescriptor csDesc;
+    csDesc.compute.module = utils::CreateShaderModule(device, R"(
+        [[stage(compute), workgroup_size(1)]] fn main() {
+        })");
+    csDesc.compute.entryPoint = "main";
+
+    device.CreateComputePipelineAsync(
+        &csDesc,
+        [](WGPUCreatePipelineAsyncStatus status, WGPUComputePipeline returnPipeline,
+           const char* message, void* userdata) {
+            EXPECT_EQ(WGPUCreatePipelineAsyncStatus::WGPUCreatePipelineAsyncStatus_DeviceDestroyed,
+                      status);
+
+            CreatePipelineAsyncTask* task = static_cast<CreatePipelineAsyncTask*>(userdata);
+            task->computePipeline = wgpu::ComputePipeline::Acquire(returnPipeline);
+            task->isCompleted = true;
+            task->message = message;
+        },
+        &task);
+    ExpectDeviceDestruction();
+    device.Destroy();
+}
+
+// Verify there is no error when the device is destroyed before the callback of
+// CreateRenderPipelineAsync() is called.
+TEST_P(CreatePipelineAsyncTest, DestroyDeviceBeforeCallbackOfCreateRenderPipelineAsync) {
+    utils::ComboRenderPipelineDescriptor renderPipelineDescriptor;
+    wgpu::ShaderModule vsModule = utils::CreateShaderModule(device, R"(
+        [[stage(vertex)]] fn main() -> [[builtin(position)]] vec4<f32> {
+            return vec4<f32>(0.0, 0.0, 0.0, 1.0);
+        })");
+    wgpu::ShaderModule fsModule = utils::CreateShaderModule(device, R"(
+        [[stage(fragment)]] fn main() -> [[location(0)]] vec4<f32> {
+            return vec4<f32>(0.0, 1.0, 0.0, 1.0);
+        })");
+    renderPipelineDescriptor.vertex.module = vsModule;
+    renderPipelineDescriptor.cFragment.module = fsModule;
+    renderPipelineDescriptor.cTargets[0].format = wgpu::TextureFormat::RGBA8Unorm;
+    renderPipelineDescriptor.primitive.topology = wgpu::PrimitiveTopology::PointList;
+
+    device.CreateRenderPipelineAsync(
+        &renderPipelineDescriptor,
+        [](WGPUCreatePipelineAsyncStatus status, WGPURenderPipeline returnPipeline,
+           const char* message, void* userdata) {
+            EXPECT_EQ(WGPUCreatePipelineAsyncStatus::WGPUCreatePipelineAsyncStatus_DeviceDestroyed,
+                      status);
+
+            CreatePipelineAsyncTask* task = static_cast<CreatePipelineAsyncTask*>(userdata);
+            task->renderPipeline = wgpu::RenderPipeline::Acquire(returnPipeline);
+            task->isCompleted = true;
+            task->message = message;
+        },
+        &task);
+    ExpectDeviceDestruction();
+    device.Destroy();
+}
+
 // Verify the code path of CreateComputePipelineAsync() to directly return the compute pipeline
 // object from cache works correctly.
 TEST_P(CreatePipelineAsyncTest, CreateSameComputePipelineTwice) {

src/tests/end2end/DestroyTests.cpp

@@ -164,6 +164,19 @@ TEST_P(DestroyTest, DestroyThenSetLabel) {
     buffer.SetLabel(label.c_str());
 }
 
+// Device destroy before buffer submit will result in error.
+TEST_P(DestroyTest, DestroyDeviceBeforeSubmit) {
+    // TODO(crbug.com/dawn/628) Add more comprehensive tests with destroy and backends.
+    DAWN_TEST_UNSUPPORTED_IF(UsesWire());
+    wgpu::CommandBuffer commands = CreateTriangleCommandBuffer();
+
+    // Tests normally don't expect a device lost error, but since we are destroying the device, we
+    // actually do, so we need to override the default device lost callback.
+    ExpectDeviceDestruction();
+    device.Destroy();
+    ASSERT_DEVICE_ERROR(queue.Submit(1, &commands));
+}
+
 DAWN_INSTANTIATE_TEST(DestroyTest,
                       D3D12Backend(),
                       MetalBackend(),