dawn_wire: Correctly handle buffer state changes on Destroy()

The server didn't take intercept the destroy() call which meant the
buffer could be unmapped by dawn_native without the status updated in
ServerBuffer. This caused crash when a subsequent UpdateMappedData
command was handled and tried to write into the mapped buffer.

The client needs to also track destroy() otherwise it could sent an
UpdateMappedData to a destroyed buffer which is a fatal error.

Tests are added that cover the client-server interaction for this, but
the pattern that the following is unfortunately not tested directly
against the wire server:

 - CreateBufferMapped
 - Destroy
 - UpdateMappedData

Bug: chromium:1068466
Change-Id: If5185d4a8a81cd5f6bb41c9888a18c44c14b2de4
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/18961
Commit-Queue: Corentin Wallez <cwallez@chromium.org>
Reviewed-by: Kai Ninomiya <kainino@chromium.org>

dawn_wire.json

@@ -106,6 +106,7 @@
             "FenceOnCompletion"
         ],
         "client_handwritten_commands": [
+            "BufferDestroy",
             "BufferUnmap",
             "DeviceCreateBuffer",
             "DeviceCreateBufferMapped",
@@ -119,6 +120,7 @@
             "Fence"
         ],
         "server_custom_pre_handler_commands": [
+            "BufferDestroy",
             "BufferUnmap"
         ],
         "server_handwritten_commands": [

src/dawn_wire/client/ApiProcs.cpp

@@ -368,6 +368,22 @@ namespace dawn_wire { namespace client {
         cmd.Serialize(allocatedBuffer, *buffer->device->GetClient());
     }
 
+    void ClientBufferDestroy(WGPUBuffer cBuffer) {
+        Buffer* buffer = reinterpret_cast<Buffer*>(cBuffer);
+
+        // Cancel or remove all mappings
+        buffer->writeHandle = nullptr;
+        buffer->readHandle = nullptr;
+        buffer->ClearMapRequests(WGPUBufferMapAsyncStatus_Unknown);
+
+        BufferDestroyCmd cmd;
+        cmd.self = cBuffer;
+        size_t requiredSize = cmd.GetRequiredSize();
+        char* allocatedBuffer =
+            static_cast<char*>(buffer->device->GetClient()->GetCmdSpace(requiredSize));
+        cmd.Serialize(allocatedBuffer, *buffer->device->GetClient());
+    }
+
     WGPUFence ClientQueueCreateFence(WGPUQueue cSelf, WGPUFenceDescriptor const* descriptor) {
         Queue* queue = reinterpret_cast<Queue*>(cSelf);
         Device* device = queue->device;

src/dawn_wire/server/ServerBuffer.cpp

@@ -31,6 +31,19 @@ namespace dawn_wire { namespace server {
         return true;
     }
 
+    bool Server::PreHandleBufferDestroy(const BufferDestroyCmd& cmd) {
+        // Destroying a buffer does an implicit unmapping.
+        auto* buffer = BufferObjects().Get(cmd.selfId);
+        DAWN_ASSERT(buffer != nullptr);
+
+        // The buffer was destroyed. Clear the Read/WriteHandle.
+        buffer->readHandle = nullptr;
+        buffer->writeHandle = nullptr;
+        buffer->mapWriteState = BufferMapWriteState::Unmapped;
+
+        return true;
+    }
+
     bool Server::DoBufferMapAsync(ObjectId bufferId,
                                   uint32_t requestSerial,
                                   bool isWrite,

src/tests/unittests/wire/WireMemoryTransferServiceTests.cpp

@@ -588,6 +588,61 @@ TEST_F(WireMemoryTransferServiceTests, BufferMapReadDeserializeInitialDataFailur
     EXPECT_CALL(serverMemoryTransferService, OnReadHandleDestroy(serverHandle)).Times(1);
 }
 
+// Test MapRead destroying the buffer before unmapping on the client side.
+TEST_F(WireMemoryTransferServiceTests, BufferMapReadDestroyBeforeUnmap) {
+    WGPUBuffer buffer;
+    WGPUBuffer apiBuffer;
+    std::tie(apiBuffer, buffer) = CreateBuffer();
+    FlushClient();
+
+    // The client should create and serialize a ReadHandle on mapReadAsync.
+    ClientReadHandle* clientHandle = ExpectReadHandleCreation();
+    ExpectReadHandleSerialization(clientHandle);
+
+    wgpuBufferMapReadAsync(buffer, ToMockBufferMapReadCallback, nullptr);
+
+    // The server should deserialize the MapRead handle from the client and then serialize
+    // an initialization message.
+    ServerReadHandle* serverHandle = ExpectServerReadHandleDeserialize();
+    ExpectServerReadHandleInitialize(serverHandle);
+
+    // Mock a successful callback
+    EXPECT_CALL(api, OnBufferMapReadAsyncCallback(apiBuffer, _, _))
+        .WillOnce(InvokeWithoutArgs([&]() {
+            api.CallMapReadCallback(apiBuffer, WGPUBufferMapAsyncStatus_Success, &mBufferContent,
+                                    sizeof(mBufferContent));
+        }));
+
+    FlushClient();
+
+    // The client receives a successful callback.
+    EXPECT_CALL(*mockBufferMapReadCallback,
+                Call(WGPUBufferMapAsyncStatus_Success, &mBufferContent, sizeof(mBufferContent), _))
+        .Times(1);
+
+    // The client should receive the handle initialization message from the server.
+    ExpectClientReadHandleDeserializeInitialize(clientHandle, &mBufferContent);
+
+    FlushServer();
+
+    // THIS IS THE TEST: destroy the buffer before unmapping and check it destroyed the mapping
+    // immediately, both in the client and server side.
+    {
+        EXPECT_CALL(clientMemoryTransferService, OnReadHandleDestroy(clientHandle)).Times(1);
+        wgpuBufferDestroy(buffer);
+
+        EXPECT_CALL(serverMemoryTransferService, OnReadHandleDestroy(serverHandle)).Times(1);
+        EXPECT_CALL(api, BufferDestroy(apiBuffer)).Times(1);
+        FlushClient();
+
+        // The handle is already destroyed so unmap only results in a server unmap call.
+        wgpuBufferUnmap(buffer);
+
+        EXPECT_CALL(api, BufferUnmap(apiBuffer)).Times(1);
+        FlushClient();
+    }
+}
+
 // Test successful MapWrite.
 TEST_F(WireMemoryTransferServiceTests, BufferMapWriteSuccess) {
     WGPUBuffer buffer;
@@ -823,6 +878,62 @@ TEST_F(WireMemoryTransferServiceTests, BufferMapWriteDeserializeFlushFailure) {
     EXPECT_CALL(serverMemoryTransferService, OnWriteHandleDestroy(serverHandle)).Times(1);
 }
 
+// Test MapWrite destroying the buffer before unmapping on the client side.
+TEST_F(WireMemoryTransferServiceTests, BufferMapWriteDestroyBeforeUnmap) {
+    WGPUBuffer buffer;
+    WGPUBuffer apiBuffer;
+    std::tie(apiBuffer, buffer) = CreateBuffer();
+    FlushClient();
+
+    ClientWriteHandle* clientHandle = ExpectWriteHandleCreation();
+    ExpectWriteHandleSerialization(clientHandle);
+
+    wgpuBufferMapWriteAsync(buffer, ToMockBufferMapWriteCallback, nullptr);
+
+    // The server should then deserialize the WriteHandle from the client.
+    ServerWriteHandle* serverHandle = ExpectServerWriteHandleDeserialization();
+
+    // Mock a successful callback.
+    EXPECT_CALL(api, OnBufferMapWriteAsyncCallback(apiBuffer, _, _))
+        .WillOnce(InvokeWithoutArgs([&]() {
+            api.CallMapWriteCallback(apiBuffer, WGPUBufferMapAsyncStatus_Success,
+                                     &mMappedBufferContent, sizeof(mMappedBufferContent));
+        }));
+
+    FlushClient();
+
+    // The client receives a successful callback.
+    EXPECT_CALL(*mockBufferMapWriteCallback,
+                Call(WGPUBufferMapAsyncStatus_Success, &mMappedBufferContent,
+                     sizeof(mMappedBufferContent), _))
+        .Times(1);
+
+    // Since the mapping succeeds, the client opens the WriteHandle.
+    ExpectClientWriteHandleOpen(clientHandle, &mMappedBufferContent);
+
+    FlushServer();
+
+    // The client writes to the handle contents.
+    mMappedBufferContent = mUpdatedBufferContent;
+
+    // THIS IS THE TEST: destroy the buffer before unmapping and check it destroyed the mapping
+    // immediately, both in the client and server side.
+    {
+        EXPECT_CALL(clientMemoryTransferService, OnWriteHandleDestroy(clientHandle)).Times(1);
+        wgpuBufferDestroy(buffer);
+
+        EXPECT_CALL(serverMemoryTransferService, OnWriteHandleDestroy(serverHandle)).Times(1);
+        EXPECT_CALL(api, BufferDestroy(apiBuffer)).Times(1);
+        FlushClient();
+
+        // The handle is already destroyed so unmap only results in a server unmap call.
+        wgpuBufferUnmap(buffer);
+
+        EXPECT_CALL(api, BufferUnmap(apiBuffer)).Times(1);
+        FlushClient();
+    }
+}
+
 // Test successful CreateBufferMappedAsync.
 TEST_F(WireMemoryTransferServiceTests, CreateBufferMappedAsyncSuccess) {
     // The client should create and serialize a WriteHandle on createBufferMappedAsync
@@ -1115,3 +1226,42 @@ TEST_F(WireMemoryTransferServiceTests, CreateBufferMappedDeserializeFlushFailure
 
     EXPECT_CALL(serverMemoryTransferService, OnWriteHandleDestroy(serverHandle)).Times(1);
 }
+
+// Test CreateBufferMapped destroying the buffer before unmapping on the client side.
+TEST_F(WireMemoryTransferServiceTests, CreateBufferMappedDestroyBeforeUnmap) {
+    // The client should create and serialize a WriteHandle on createBufferMapped.
+    ClientWriteHandle* clientHandle = ExpectWriteHandleCreation();
+
+    // Staging data is immediately available so the handle is Opened.
+    ExpectClientWriteHandleOpen(clientHandle, &mMappedBufferContent);
+
+    ExpectWriteHandleSerialization(clientHandle);
+
+    // The server should then deserialize the WriteHandle from the client.
+    ServerWriteHandle* serverHandle = ExpectServerWriteHandleDeserialization();
+
+    WGPUCreateBufferMappedResult result;
+    WGPUCreateBufferMappedResult apiResult;
+    std::tie(apiResult, result) = CreateBufferMapped();
+    FlushClient();
+
+    // Update the mapped contents.
+    mMappedBufferContent = mUpdatedBufferContent;
+
+    // THIS IS THE TEST: destroy the buffer before unmapping and check it destroyed the mapping
+    // immediately, both in the client and server side.
+    {
+        EXPECT_CALL(clientMemoryTransferService, OnWriteHandleDestroy(clientHandle)).Times(1);
+        wgpuBufferDestroy(result.buffer);
+
+        EXPECT_CALL(serverMemoryTransferService, OnWriteHandleDestroy(serverHandle)).Times(1);
+        EXPECT_CALL(api, BufferDestroy(apiResult.buffer)).Times(1);
+        FlushClient();
+
+        // The handle is already destroyed so unmap only results in a server unmap call.
+        wgpuBufferUnmap(result.buffer);
+
+        EXPECT_CALL(api, BufferUnmap(apiResult.buffer)).Times(1);
+        FlushClient();
+    }
+}