Reject callbacks if the device is destroyed before completion

Callbacks should all reject instead of waiting for the device
to idle on shutdown.

Bug: dawn:652
Change-Id: Id4a9ab2560aa34b8ea574271f61f8a499e15ab3a
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/40360
Reviewed-by: Stephen White <senorblanco@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Austin Eng <enga@chromium.org>

src/dawn_native/Device.cpp

@@ -144,6 +144,15 @@ namespace dawn_native {
     }
 
     void DeviceBase::ShutDownBase() {
+        // Skip handling device facilities if they haven't even been created (or failed doing so)
+        if (mState != State::BeingCreated) {
+            // Reject all error scope callbacks.
+            mErrorScopeTracker->ClearForShutDown();
+
+            // Reject all async pipeline creations.
+            mCreateReadyPipelineTracker->ClearForShutDown();
+        }
+
         // Disconnect the device, depending on which state we are currently in.
         switch (mState) {
             case State::BeingCreated:
@@ -171,18 +180,13 @@ namespace dawn_native {
         ASSERT(mCompletedSerial == mLastSubmittedSerial);
         ASSERT(mFutureSerial <= mCompletedSerial);
 
-        // Skip handling device facilities if they haven't even been created (or failed doing so)
         if (mState != State::BeingCreated) {
-            // The GPU timeline is finished so all services can be freed immediately. They need to
+            // The GPU timeline is finished.
-            // be freed before ShutDownImpl() because they might relinquish resources that will be
+            // Tick the queue-related tasks since they should be complete. This must be done before
-            // freed by backends in the ShutDownImpl() call. Still tick the ones that might have
+            // ShutDownImpl() it may relinquish resources that will be freed by backends in the
-            // pending callbacks.
+            // ShutDownImpl() call.
-            mErrorScopeTracker->Tick(GetCompletedCommandSerial());
             GetQueue()->Tick(GetCompletedCommandSerial());
-
+            // Call TickImpl once last time to clean up resources
-            mCreateReadyPipelineTracker->ClearForShutDown();
-
-            // call TickImpl once last time to clean up resources
             // Ignore errors so that we can continue with destruction
             IgnoreErrors(TickImpl());
         }

src/dawn_native/ErrorScopeTracker.cpp

@@ -25,13 +25,7 @@ namespace dawn_native {
     }
 
     ErrorScopeTracker::~ErrorScopeTracker() {
-        // The tracker is destroyed when the Device is destroyed. We need to
+        ASSERT(mScopesInFlight.Empty());
-        // call Destroy on all in-flight error scopes so they resolve their callbacks
-        // with UNKNOWN.
-        for (Ref<ErrorScope>& scope : mScopesInFlight.IterateUpTo(kMaxExecutionSerial)) {
-            scope->UnlinkForShutdown();
-        }
-        Tick(kMaxExecutionSerial);
     }
 
     void ErrorScopeTracker::TrackUntilLastSubmitComplete(ErrorScope* scope) {
@@ -43,4 +37,11 @@ namespace dawn_native {
         mScopesInFlight.ClearUpTo(completedSerial);
     }
 
+    void ErrorScopeTracker::ClearForShutDown() {
+        for (Ref<ErrorScope>& scope : mScopesInFlight.IterateAll()) {
+            scope->UnlinkForShutdown();
+        }
+        mScopesInFlight.Clear();
+    }
+
 }  // namespace dawn_native

src/dawn_native/ErrorScopeTracker.h

@@ -32,6 +32,7 @@ namespace dawn_native {
         void TrackUntilLastSubmitComplete(ErrorScope* scope);
 
         void Tick(ExecutionSerial completedSerial);
+        void ClearForShutDown();
 
       protected:
         DeviceBase* mDevice;

src/dawn_wire/client/ClientDoers.cpp

@@ -38,6 +38,10 @@ namespace dawn_wire { namespace client {
     }
 
     bool Client::DoDeviceLostCallback(Device* device, char const* message) {
+        if (device == nullptr) {
+            // The device might have been deleted or recreated so this isn't an error.
+            return true;
+        }
         device->HandleDeviceLost(message);
         return true;
     }
@@ -46,6 +50,10 @@ namespace dawn_wire { namespace client {
                                                uint64_t requestSerial,
                                                WGPUErrorType errorType,
                                                const char* message) {
+        if (device == nullptr) {
+            // The device might have been deleted or recreated so this isn't an error.
+            return true;
+        }
         return device->OnPopErrorScopeCallback(requestSerial, errorType, message);
     }
 

src/tests/unittests/validation/ErrorScopeValidationTests.cpp

@@ -199,16 +199,15 @@ TEST_F(ErrorScopeValidationTest, AsynchronousThenSynchronous) {
 // Test that if the device is destroyed before the callback occurs, it is called with NoError
 // because all previous operations are waited upon before the destruction returns.
 TEST_F(ErrorScopeValidationTest, DeviceDestroyedBeforeCallback) {
-    // TODO(crbug.com/dawn/652): This has different behavior on the wire and should be consistent.
-    DAWN_SKIP_TEST_IF(UsesWire());
-
-    wgpu::Queue queue = device.GetQueue();
-
     device.PushErrorScope(wgpu::ErrorFilter::OutOfMemory);
+    {
+        // Note: this is in its own scope to be clear the queue does not outlive the device.
+        wgpu::Queue queue = device.GetQueue();
         queue.Submit(0, nullptr);
+    }
     device.PopErrorScope(ToMockDevicePopErrorScopeCallback, this);
 
-    EXPECT_CALL(*mockDevicePopErrorScopeCallback, Call(WGPUErrorType_NoError, _, this)).Times(1);
+    EXPECT_CALL(*mockDevicePopErrorScopeCallback, Call(WGPUErrorType_Unknown, _, this)).Times(1);
     device = nullptr;
 }