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>
diff --git a/dawn_wire.json b/dawn_wire.json
index 5c66ed6..58ef049 100644
--- a/dawn_wire.json
+++ b/dawn_wire.json
@@ -209,8 +209,7 @@
"BufferUnmap",
"DeviceCreateErrorBuffer",
"DeviceGetQueue",
- "DeviceInjectError",
- "DevicePushErrorScope"
+ "DeviceInjectError"
],
"client_special_objects": [
"Adapter",
diff --git a/src/dawn/native/Device.cpp b/src/dawn/native/Device.cpp
index d03a73f..0855ebe 100644
--- a/src/dawn/native/Device.cpp
+++ b/src/dawn/native/Device.cpp
@@ -543,17 +543,25 @@
}
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 true;
+ callback(static_cast<WGPUErrorType>(scope.GetErrorType()), scope.GetErrorMessage(),
+ userdata);
+ return returnValue;
}
PersistentCache* DeviceBase::GetPersistentCache() {
diff --git a/src/dawn/tests/unittests/validation/ErrorScopeValidationTests.cpp b/src/dawn/tests/unittests/validation/ErrorScopeValidationTests.cpp
index ca2dfe9..808eba4 100644
--- a/src/dawn/tests/unittests/validation/ErrorScopeValidationTests.cpp
+++ b/src/dawn/tests/unittests/validation/ErrorScopeValidationTests.cpp
@@ -138,8 +138,11 @@
// 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 @@
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 @@
}
}
+// 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) {
diff --git a/src/dawn/tests/unittests/wire/WireErrorCallbackTests.cpp b/src/dawn/tests/unittests/wire/WireErrorCallbackTests.cpp
index 6ec0485..a9c5522 100644
--- a/src/dawn/tests/unittests/wire/WireErrorCallbackTests.cpp
+++ b/src/dawn/tests/unittests/wire/WireErrorCallbackTests.cpp
@@ -139,25 +139,21 @@
// 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, 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 @@
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 @@
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) {
- wgpuDevicePushErrorScope(device, WGPUErrorFilter_Validation);
+TEST_F(WireErrorCallbackTests, PopErrorScopeDeviceInFlightDestroy) {
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 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 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 @@
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
diff --git a/src/dawn/wire/client/Device.cpp b/src/dawn/wire/client/Device.cpp
index c3866a3..9378bd5 100644
--- a/src/dawn/wire/client/Device.cpp
+++ b/src/dawn/wire/client/Device.cpp
@@ -145,35 +145,18 @@
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) {
- return false;
- }
- mErrorScopeStackSize--;
-
+ // TODO(crbug.com/dawn/1324) Replace bool return with void when users are updated.
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;
}
diff --git a/src/dawn/wire/client/Device.h b/src/dawn/wire/client/Device.h
index bb6a96d..56e2af2 100644
--- a/src/dawn/wire/client/Device.h
+++ b/src/dawn/wire/client/Device.h
@@ -84,7 +84,6 @@
void* userdata = nullptr;
};
RequestTracker<ErrorScopeData> mErrorScopes;
- uint64_t mErrorScopeStackSize = 0;
struct CreatePipelineAsyncRequest {
WGPUCreateComputePipelineAsyncCallback createComputePipelineAsyncCallback = nullptr;
diff --git a/src/dawn/wire/server/ServerDevice.cpp b/src/dawn/wire/server/ServerDevice.cpp
index b04afc5..45fb6b8 100644
--- a/src/dawn/wire/server/ServerDevice.cpp
+++ b/src/dawn/wire/server/ServerDevice.cpp
@@ -89,13 +89,9 @@
userdata->requestSerial = requestSerial;
userdata->device = ObjectHandle{deviceId, device->generation};
- ErrorScopeUserdata* unownedUserdata = userdata.release();
- bool success = mProcs.devicePopErrorScope(
- device->handle, ForwardToServer<&Server::OnDevicePopErrorScope>, unownedUserdata);
- if (!success) {
- delete unownedUserdata;
- }
- return success;
+ mProcs.devicePopErrorScope(device->handle, ForwardToServer<&Server::OnDevicePopErrorScope>,
+ userdata.release());
+ return true;
}
void Server::OnDevicePopErrorScope(ErrorScopeUserdata* userdata,
