wire::client: Make Buffer::MapAsync surface server-side errors.

In the WebGPU specification, validation errors for mapAsync take
precedence over the early-unmap or early-destroy promise resolution.

Change the client to wait for the mapAsync status from the server before
sending the cancelation through the callback. If the server sends back
an error, then it takes precedence over the client-side status.

Also adds tests for the updated semantic.

Bug: dawn:445

Change-Id: I7bf1d8bbb3cb62d73ab19ecdf0aad2963e854964
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/29300
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Stephen White <senorblanco@chromium.org>

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);

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<MemoryTransferService::ReadHandle> readHandle = nullptr;
             std::unique_ptr<MemoryTransferService::WriteHandle> writeHandle = nullptr;

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();
 }