diff --git a/src/dawn_wire/client/Buffer.cpp b/src/dawn_wire/client/Buffer.cpp index 7db4749ed2..5fc7fbb20c 100644 --- a/src/dawn_wire/client/Buffer.cpp +++ b/src/dawn_wire/client/Buffer.cpp @@ -103,13 +103,9 @@ namespace dawn_wire { namespace client { Buffer::~Buffer() { // Callbacks need to be fired in all cases, as they can handle freeing resources // so we call them with "DestroyedBeforeCallback" status. - ClearMapRequests(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback); - } - - void Buffer::ClearMapRequests(WGPUBufferMapAsyncStatus status) { for (auto& it : mRequests) { if (it.second.callback) { - it.second.callback(status, it.second.userdata); + it.second.callback(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback, it.second.userdata); } } mRequests.clear(); @@ -200,10 +196,9 @@ namespace dawn_wire { namespace client { uint32_t status, uint64_t readInitialDataInfoLength, const uint8_t* readInitialDataInfo) { - // The requests can have been deleted via an Unmap so this isn't an error. auto requestIt = mRequests.find(requestSerial); if (requestIt == mRequests.end()) { - return true; + return false; } auto request = std::move(requestIt->second); @@ -222,6 +217,11 @@ namespace dawn_wire { namespace client { bool isWrite = request.writeHandle != nullptr; ASSERT(isRead != isWrite); + // Take into account the client-side status of the request if the server says it is a success. + if (status == WGPUBufferMapAsyncStatus_Success) { + status = request.clientStatus; + } + size_t mappedDataLength = 0; const void* mappedData = nullptr; if (status == WGPUBufferMapAsyncStatus_Success) { @@ -326,7 +326,13 @@ namespace dawn_wire { namespace client { mMappedData = nullptr; mMapOffset = 0; mMapSize = 0; - ClearMapRequests(WGPUBufferMapAsyncStatus_UnmappedBeforeCallback); + + // Tag all mapping requests still in flight as unmapped before callback. + for (auto& it : mRequests) { + if (it.second.clientStatus == WGPUBufferMapAsyncStatus_Success) { + it.second.clientStatus = WGPUBufferMapAsyncStatus_UnmappedBeforeCallback; + } + } BufferUnmapCmd cmd; cmd.self = ToAPI(this); @@ -334,11 +340,17 @@ namespace dawn_wire { namespace client { } void Buffer::Destroy() { - // Cancel or remove all mappings + // Remove the current mapping. mWriteHandle = nullptr; mReadHandle = nullptr; mMappedData = nullptr; - ClearMapRequests(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback); + + // Tag all mapping requests still in flight as destroyed before callback. + for (auto& it : mRequests) { + if (it.second.clientStatus == WGPUBufferMapAsyncStatus_Success) { + it.second.clientStatus = WGPUBufferMapAsyncStatus_DestroyedBeforeCallback; + } + } BufferDestroyCmd cmd; cmd.self = ToAPI(this); diff --git a/src/dawn_wire/client/Buffer.h b/src/dawn_wire/client/Buffer.h index f419583913..9268a6495f 100644 --- a/src/dawn_wire/client/Buffer.h +++ b/src/dawn_wire/client/Buffer.h @@ -32,7 +32,6 @@ namespace dawn_wire { namespace client { static WGPUBuffer CreateError(Device* device); ~Buffer(); - void ClearMapRequests(WGPUBufferMapAsyncStatus status); bool OnMapAsyncCallback(uint32_t requestSerial, uint32_t status, @@ -62,6 +61,12 @@ namespace dawn_wire { namespace client { void* userdata = nullptr; size_t offset = 0; size_t size = 0; + + // When the buffer is destroyed or unmapped too early, the unmappedBeforeX status takes + // precedence over the success value returned from the server. However Error statuses + // from the server take precedence over the client-side status. + WGPUBufferMapAsyncStatus clientStatus = WGPUBufferMapAsyncStatus_Success; + // TODO(enga): Use a tagged pointer to save space. std::unique_ptr readHandle = nullptr; std::unique_ptr writeHandle = nullptr; diff --git a/src/tests/unittests/wire/WireBufferMappingTests.cpp b/src/tests/unittests/wire/WireBufferMappingTests.cpp index c763ac012b..8b948aef79 100644 --- a/src/tests/unittests/wire/WireBufferMappingTests.cpp +++ b/src/tests/unittests/wire/WireBufferMappingTests.cpp @@ -65,9 +65,13 @@ class WireBufferMappingTests : public WireTest { mockBufferMapCallback = nullptr; } + void FlushClient() { + WireTest::FlushClient(); + Mock::VerifyAndClearExpectations(&mockBufferMapCallback); + } + void FlushServer() { WireTest::FlushServer(); - Mock::VerifyAndClearExpectations(&mockBufferMapCallback); } @@ -160,14 +164,91 @@ TEST_F(WireBufferMappingTests, UnmapCalledTooEarlyForRead) { EXPECT_CALL(api, BufferGetConstMappedRange(apiBuffer, 0, kBufferSize)) .WillOnce(Return(&bufferContent)); + // Oh no! We are calling Unmap too early! However the callback gets fired only after we get + // an answer from the server. + wgpuBufferUnmap(buffer); + EXPECT_CALL(api, BufferUnmap(apiBuffer)); + FlushClient(); - // Oh no! We are calling Unmap too early! + // The callback shouldn't get called with success, even when the request succeeded on the + // server side EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_UnmappedBeforeCallback, _)) .Times(1); - wgpuBufferUnmap(buffer); - // The callback shouldn't get called, even when the request succeeded on the server side + FlushServer(); +} + +// Check that even if Unmap() was called early client-side, we correctly surface server-side +// validation errors. +TEST_F(WireBufferMappingTests, UnmapCalledTooEarlyForReadButServerSideError) { + wgpuBufferMapAsync(buffer, WGPUMapMode_Read, 0, kBufferSize, ToMockBufferMapCallback, nullptr); + + EXPECT_CALL(api, OnBufferMapAsyncCallback(apiBuffer, _, _)).WillOnce(InvokeWithoutArgs([&]() { + api.CallMapAsyncCallback(apiBuffer, WGPUBufferMapAsyncStatus_Error); + })); + + // Oh no! We are calling Unmap too early! However the callback gets fired only after we get + // an answer from the server that the mapAsync call was an error. + wgpuBufferUnmap(buffer); + EXPECT_CALL(api, BufferUnmap(apiBuffer)); + + FlushClient(); + + // The callback should be called with the server-side error and not the UnmappedBeforeCallback. + EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_Error, _)) + .Times(1); + + FlushServer(); +} + +// Check the map read callback is called with "DestroyedBeforeCallback" when the map request would +// have worked, but Destroy was called +TEST_F(WireBufferMappingTests, DestroyCalledTooEarlyForRead) { + wgpuBufferMapAsync(buffer, WGPUMapMode_Read, 0, kBufferSize, ToMockBufferMapCallback, nullptr); + + uint32_t bufferContent = 31337; + EXPECT_CALL(api, OnBufferMapAsyncCallback(apiBuffer, _, _)).WillOnce(InvokeWithoutArgs([&]() { + api.CallMapAsyncCallback(apiBuffer, WGPUBufferMapAsyncStatus_Success); + })); + EXPECT_CALL(api, BufferGetConstMappedRange(apiBuffer, 0, kBufferSize)) + .WillOnce(Return(&bufferContent)); + + // Oh no! We are calling Unmap too early! However the callback gets fired only after we get + // an answer from the server. + wgpuBufferDestroy(buffer); + EXPECT_CALL(api, BufferDestroy(apiBuffer)); + + FlushClient(); + + // The callback shouldn't get called with success, even when the request succeeded on the + // server side + EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback, _)) + .Times(1); + + FlushServer(); +} + +// Check that even if Destroy() was called early client-side, we correctly surface server-side +// validation errors. +TEST_F(WireBufferMappingTests, DestroyCalledTooEarlyForReadButServerSideError) { + wgpuBufferMapAsync(buffer, WGPUMapMode_Read, 0, kBufferSize, ToMockBufferMapCallback, nullptr); + + EXPECT_CALL(api, OnBufferMapAsyncCallback(apiBuffer, _, _)).WillOnce(InvokeWithoutArgs([&]() { + api.CallMapAsyncCallback(apiBuffer, WGPUBufferMapAsyncStatus_Error); + })); + + // Oh no! We are calling Destroy too early! However the callback gets fired only after we get + // an answer from the server that the mapAsync call was an error. + wgpuBufferDestroy(buffer); + EXPECT_CALL(api, BufferDestroy(apiBuffer)); + + FlushClient(); + + // The callback should be called with the server-side error and not the DestroyedBeforCallback.. + EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_Error, _)) + .Times(1); + FlushServer(); }