Fixes popErrorScope to match the specs.

- Prepares for removal of unnecessary bool return, and just call callbacks appropriately. For now always returns true until all users are updated.
- Removes PushErrorScope from handwritten commands now that we no longer need to do anything special.
- Updates tests to reflect the change and make sure to set EXPECTs before calling functions to make tests easier to follow.

Bug: dawn:1324, dawn:526
Change-Id: I90b09c54f9adbf2d6d50ad20dcedf68b5ed0b1fa
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/83942
Reviewed-by: Kai Ninomiya <kainino@chromium.org>
Commit-Queue: Austin Eng <enga@chromium.org>

dawn_wire.json

@@ -209,8 +209,7 @@
             "BufferUnmap",
             "DeviceCreateErrorBuffer",
             "DeviceGetQueue",
-            "DeviceInjectError",
+            "DeviceInjectError"
-            "DevicePushErrorScope"
         ],
         "client_special_objects": [
             "Adapter",

src/dawn/native/Device.cpp

@@ -543,17 +543,25 @@ namespace dawn::native {
     }
 
     bool DeviceBase::APIPopErrorScope(wgpu::ErrorCallback callback, void* userdata) {
+        // TODO(crbug.com/dawn/1324) Remove return and make function void when users are updated.
+        bool returnValue = true;
+        if (callback == nullptr) {
+            static wgpu::ErrorCallback defaultCallback = [](WGPUErrorType, char const*, void*) {};
+            callback = defaultCallback;
+        }
+        // TODO(crbug.com/dawn/1122): Call callbacks only on wgpuInstanceProcessEvents
+        if (IsLost()) {
+            callback(WGPUErrorType_DeviceLost, "GPU device disconnected", userdata);
+            return returnValue;
+        }
         if (mErrorScopeStack->Empty()) {
-            return false;
+            callback(WGPUErrorType_Unknown, "No error scopes to pop", userdata);
+            return returnValue;
         }
         ErrorScope scope = mErrorScopeStack->Pop();
-        if (callback != nullptr) {
-            // TODO(crbug.com/dawn/1122): Call callbacks only on wgpuInstanceProcessEvents
         callback(static_cast<WGPUErrorType>(scope.GetErrorType()), scope.GetErrorMessage(),
                  userdata);
-        }
+        return returnValue;
-
-        return true;
     }
 
     PersistentCache* DeviceBase::GetPersistentCache() {

src/dawn/tests/unittests/validation/ErrorScopeValidationTests.cpp

@@ -138,8 +138,11 @@ TEST_F(ErrorScopeValidationTest, UnhandledErrorsMatchUncapturedErrorCallback) {
 // Check that push/popping error scopes must be balanced.
 TEST_F(ErrorScopeValidationTest, PushPopBalanced) {
     // No error scopes to pop.
-    { EXPECT_FALSE(device.PopErrorScope(ToMockDevicePopErrorScopeCallback, this)); }
+    {
-
+        EXPECT_CALL(*mockDevicePopErrorScopeCallback, Call(WGPUErrorType_Unknown, _, this))
+            .Times(1);
+        device.PopErrorScope(ToMockDevicePopErrorScopeCallback, this);
+    }
     // Too many pops
     {
         device.PushErrorScope(wgpu::ErrorFilter::Validation);
@@ -149,7 +152,9 @@ TEST_F(ErrorScopeValidationTest, PushPopBalanced) {
         device.PopErrorScope(ToMockDevicePopErrorScopeCallback, this + 1);
         FlushWire();
 
-        EXPECT_FALSE(device.PopErrorScope(ToMockDevicePopErrorScopeCallback, this + 2));
+        EXPECT_CALL(*mockDevicePopErrorScopeCallback, Call(WGPUErrorType_Unknown, _, this + 2))
+            .Times(1);
+        device.PopErrorScope(ToMockDevicePopErrorScopeCallback, this + 2);
     }
 }
 
@@ -208,6 +213,17 @@ TEST_F(ErrorScopeValidationTest, DeviceDestroyedBeforeCallback) {
     }
 }
 
+// If the device is destroyed, pop error scope should callback with device lost.
+TEST_F(ErrorScopeValidationTest, DeviceDestroyedBeforePop) {
+    device.PushErrorScope(wgpu::ErrorFilter::Validation);
+    ExpectDeviceDestruction();
+    device.Destroy();
+    FlushWire();
+
+    EXPECT_CALL(*mockDevicePopErrorScopeCallback, Call(WGPUErrorType_DeviceLost, _, this)).Times(1);
+    device.PopErrorScope(ToMockDevicePopErrorScopeCallback, this);
+}
+
 // Regression test that on device shutdown, we don't get a recursion in O(pushed error scope) that
 // would lead to a stack overflow
 TEST_F(ErrorScopeValidationTest, ShutdownStackOverflow) {

src/dawn/tests/unittests/wire/WireErrorCallbackTests.cpp

@@ -139,25 +139,21 @@ TEST_F(WireErrorCallbackTests, DeviceLoggingCallback) {
 
 // Test the return wire for error scopes.
 TEST_F(WireErrorCallbackTests, PushPopErrorScopeCallback) {
-    wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation);
     EXPECT_CALL(api, DevicePushErrorScope(apiDevice, WGPUErrorFilter_Validation)).Times(1);
-
+    wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation);
     FlushClient();
 
-    wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this);
-
     WGPUErrorCallback callback;
     void* userdata;
     EXPECT_CALL(api, OnDevicePopErrorScope(apiDevice, _, _))
         .WillOnce(DoAll(SaveArg<1>(&callback), SaveArg<2>(&userdata), Return(true)));
-
+    wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this);
     FlushClient();
 
-    callback(WGPUErrorType_Validation, "Some error message", userdata);
     EXPECT_CALL(*mockDevicePopErrorScopeCallback,
                 Call(WGPUErrorType_Validation, StrEq("Some error message"), this))
         .Times(1);
-
+    callback(WGPUErrorType_Validation, "Some error message", userdata);
     FlushServer();
 }
 
@@ -165,15 +161,11 @@ TEST_F(WireErrorCallbackTests, PushPopErrorScopeCallback) {
 TEST_F(WireErrorCallbackTests, PopErrorScopeCallbackOrdering) {
     // Two error scopes are popped, and the first one returns first.
     {
-        wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation);
-        wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation);
         EXPECT_CALL(api, DevicePushErrorScope(apiDevice, WGPUErrorFilter_Validation)).Times(2);
-
+        wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation);
+        wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation);
         FlushClient();
 
-        wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this);
-        wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this + 1);
-
         WGPUErrorCallback callback1;
         WGPUErrorCallback callback2;
         void* userdata1;
@@ -181,33 +173,30 @@ TEST_F(WireErrorCallbackTests, PopErrorScopeCallbackOrdering) {
         EXPECT_CALL(api, OnDevicePopErrorScope(apiDevice, _, _))
             .WillOnce(DoAll(SaveArg<1>(&callback1), SaveArg<2>(&userdata1), Return(true)))
             .WillOnce(DoAll(SaveArg<1>(&callback2), SaveArg<2>(&userdata2), Return(true)));
-
+        wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this);
+        wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this + 1);
         FlushClient();
 
-        callback1(WGPUErrorType_Validation, "First error message", userdata1);
         EXPECT_CALL(*mockDevicePopErrorScopeCallback,
                     Call(WGPUErrorType_Validation, StrEq("First error message"), this))
             .Times(1);
+        callback1(WGPUErrorType_Validation, "First error message", userdata1);
         FlushServer();
 
-        callback2(WGPUErrorType_Validation, "Second error message", userdata2);
         EXPECT_CALL(*mockDevicePopErrorScopeCallback,
                     Call(WGPUErrorType_Validation, StrEq("Second error message"), this + 1))
             .Times(1);
+        callback2(WGPUErrorType_Validation, "Second error message", userdata2);
         FlushServer();
     }
 
     // Two error scopes are popped, and the second one returns first.
     {
-        wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation);
-        wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation);
         EXPECT_CALL(api, DevicePushErrorScope(apiDevice, WGPUErrorFilter_Validation)).Times(2);
-
+        wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation);
+        wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation);
         FlushClient();
 
-        wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this);
-        wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this + 1);
-
         WGPUErrorCallback callback1;
         WGPUErrorCallback callback2;
         void* userdata1;
@@ -215,36 +204,35 @@ TEST_F(WireErrorCallbackTests, PopErrorScopeCallbackOrdering) {
         EXPECT_CALL(api, OnDevicePopErrorScope(apiDevice, _, _))
             .WillOnce(DoAll(SaveArg<1>(&callback1), SaveArg<2>(&userdata1), Return(true)))
             .WillOnce(DoAll(SaveArg<1>(&callback2), SaveArg<2>(&userdata2), Return(true)));
-
+        wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this);
+        wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this + 1);
         FlushClient();
 
-        callback2(WGPUErrorType_Validation, "Second error message", userdata2);
         EXPECT_CALL(*mockDevicePopErrorScopeCallback,
                     Call(WGPUErrorType_Validation, StrEq("Second error message"), this + 1))
             .Times(1);
+        callback2(WGPUErrorType_Validation, "Second error message", userdata2);
         FlushServer();
 
-        callback1(WGPUErrorType_Validation, "First error message", userdata1);
         EXPECT_CALL(*mockDevicePopErrorScopeCallback,
                     Call(WGPUErrorType_Validation, StrEq("First error message"), this))
             .Times(1);
+        callback1(WGPUErrorType_Validation, "First error message", userdata1);
         FlushServer();
     }
 }
 
 // Test the return wire for error scopes in flight when the device is destroyed.
-TEST_F(WireErrorCallbackTests, PopErrorScopeDeviceDestroyed) {
+TEST_F(WireErrorCallbackTests, PopErrorScopeDeviceInFlightDestroy) {
-    wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation);
     EXPECT_CALL(api, DevicePushErrorScope(apiDevice, WGPUErrorFilter_Validation)).Times(1);
-
+    wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation);
     FlushClient();
 
-    EXPECT_TRUE(wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this));
-
     EXPECT_CALL(api, OnDevicePopErrorScope(apiDevice, _, _)).WillOnce(Return(true));
+    wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this);
     FlushClient();
 
-    // Incomplete callback called in Device destructor.
+    // Incomplete callback called in Device destructor. This is resolved after the end of this test.
     EXPECT_CALL(*mockDevicePopErrorScopeCallback,
                 Call(WGPUErrorType_Unknown, ValidStringMessage(), this))
         .Times(1);
@@ -253,12 +241,11 @@ TEST_F(WireErrorCallbackTests, PopErrorScopeDeviceDestroyed) {
 // Test that registering a callback then wire disconnect calls the callback with
 // DeviceLost.
 TEST_F(WireErrorCallbackTests, PopErrorScopeThenDisconnect) {
-    wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation);
     EXPECT_CALL(api, DevicePushErrorScope(apiDevice, WGPUErrorFilter_Validation)).Times(1);
+    wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation);
 
-    EXPECT_TRUE(wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this));
     EXPECT_CALL(api, OnDevicePopErrorScope(apiDevice, _, _)).WillOnce(Return(true));
-
+    wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this);
     FlushClient();
 
     EXPECT_CALL(*mockDevicePopErrorScopeCallback,
@@ -270,9 +257,8 @@ TEST_F(WireErrorCallbackTests, PopErrorScopeThenDisconnect) {
 // Test that registering a callback after wire disconnect calls the callback with
 // DeviceLost.
 TEST_F(WireErrorCallbackTests, PopErrorScopeAfterDisconnect) {
-    wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation);
     EXPECT_CALL(api, DevicePushErrorScope(apiDevice, WGPUErrorFilter_Validation)).Times(1);
-
+    wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation);
     FlushClient();
 
     GetWireClient()->Disconnect();
@@ -280,36 +266,23 @@ TEST_F(WireErrorCallbackTests, PopErrorScopeAfterDisconnect) {
     EXPECT_CALL(*mockDevicePopErrorScopeCallback,
                 Call(WGPUErrorType_DeviceLost, ValidStringMessage(), this))
         .Times(1);
-    EXPECT_TRUE(wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this));
+    wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this);
 }
 
-// Test that PopErrorScope returns false if there are no error scopes.
+// Empty stack (We are emulating the errors that would be callback-ed from native).
 TEST_F(WireErrorCallbackTests, PopErrorScopeEmptyStack) {
-    // Empty stack
-    { EXPECT_FALSE(wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this)); }
-
-    // Pop too many times
-    {
-        wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation);
-        EXPECT_CALL(api, DevicePushErrorScope(apiDevice, WGPUErrorFilter_Validation)).Times(1);
-
-        EXPECT_TRUE(wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this));
-        EXPECT_FALSE(wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this + 1));
-
         WGPUErrorCallback callback;
         void* userdata;
         EXPECT_CALL(api, OnDevicePopErrorScope(apiDevice, _, _))
             .WillOnce(DoAll(SaveArg<1>(&callback), SaveArg<2>(&userdata), Return(true)));
-
+        wgpuDevicePopErrorScope(device, ToMockDevicePopErrorScopeCallback, this);
         FlushClient();
 
-        callback(WGPUErrorType_Validation, "Some error message", userdata);
         EXPECT_CALL(*mockDevicePopErrorScopeCallback,
-                    Call(WGPUErrorType_Validation, StrEq("Some error message"), this))
+                    Call(WGPUErrorType_Validation, StrEq("No error scopes to pop"), this))
             .Times(1);
-
+        callback(WGPUErrorType_Validation, "No error scopes to pop", userdata);
         FlushServer();
-    }
 }
 
 // Test the return wire for device lost callback

src/dawn/wire/client/Device.cpp

@@ -145,35 +145,18 @@ namespace dawn::wire::client {
         mDeviceLostUserdata = userdata;
     }
 
-    void Device::PushErrorScope(WGPUErrorFilter filter) {
-        mErrorScopeStackSize++;
-
-        DevicePushErrorScopeCmd cmd;
-        cmd.self = ToAPI(this);
-        cmd.filter = filter;
-
-        client->SerializeCommand(cmd);
-    }
-
     bool Device::PopErrorScope(WGPUErrorCallback callback, void* userdata) {
-        if (mErrorScopeStackSize == 0) {
+        // TODO(crbug.com/dawn/1324) Replace bool return with void when users are updated.
-            return false;
-        }
-        mErrorScopeStackSize--;
-
         if (client->IsDisconnected()) {
             callback(WGPUErrorType_DeviceLost, "GPU device disconnected", userdata);
             return true;
         }
 
         uint64_t serial = mErrorScopes.Add({callback, userdata});
-
         DevicePopErrorScopeCmd cmd;
         cmd.deviceId = this->id;
         cmd.requestSerial = serial;
-
         client->SerializeCommand(cmd);
-
         return true;
     }
 

src/dawn/wire/client/Device.h

@@ -84,7 +84,6 @@ namespace dawn::wire::client {
             void* userdata = nullptr;
         };
         RequestTracker<ErrorScopeData> mErrorScopes;
-        uint64_t mErrorScopeStackSize = 0;
 
         struct CreatePipelineAsyncRequest {
             WGPUCreateComputePipelineAsyncCallback createComputePipelineAsyncCallback = nullptr;

src/dawn/wire/server/ServerDevice.cpp

@@ -89,13 +89,9 @@ namespace dawn::wire::server {
         userdata->requestSerial = requestSerial;
         userdata->device = ObjectHandle{deviceId, device->generation};
 
-        ErrorScopeUserdata* unownedUserdata = userdata.release();
+        mProcs.devicePopErrorScope(device->handle, ForwardToServer<&Server::OnDevicePopErrorScope>,
-        bool success = mProcs.devicePopErrorScope(
+                                   userdata.release());
-            device->handle, ForwardToServer<&Server::OnDevicePopErrorScope>, unownedUserdata);
+        return true;
-        if (!success) {
-            delete unownedUserdata;
-        }
-        return success;
     }
 
     void Server::OnDevicePopErrorScope(ErrorScopeUserdata* userdata,