Immediate Buffer.MapAsync() rejection if pending map in Wire

This immediate rejection has been implemented in Native but
hasn't been yet in Wire. This commit adds the implementation
to Wire.

Also the commit changes the MapAsync callback firing timing
if pending map buffer is unmapped or destroyed. With this
commit the callback will be fired immediately Unmap or
Destroy is called to match the WebGPU spec. Currently the
callback is fired when the client receives a response from
server but it mismatches the spec.

Change-Id: Ia48d62be31912fd0384e23271e9de516f9d71d6c
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/113607
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Takahiro <hogehoge@gachapin.jp>
Reviewed-by: Austin Eng <enga@chromium.org>

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

@@ -186,18 +186,17 @@ TEST_F(WireBufferMappingReadTests, 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
+    // The callback should get called immediately with UnmappedBeforeCallback status
-    // an answer from the server.
+    // even if the request succeeds on the server side
+    EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_UnmappedBeforeCallback, _))
+        .Times(1);
+
+    // Oh no! We are calling Unmap too early! The callback should get fired immediately
+    // before we get an answer from the server.
     wgpuBufferUnmap(buffer);
     EXPECT_CALL(api, BufferUnmap(apiBuffer));
 
     FlushClient();
-
-    // 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);
-
     FlushServer();
 }
 
@@ -210,17 +209,17 @@ TEST_F(WireBufferMappingReadTests, UnmapCalledTooEarlyForReadButServerSideError)
         .WillOnce(InvokeWithoutArgs(
             [&]() { api.CallBufferMapAsyncCallback(apiBuffer, WGPUBufferMapAsyncStatus_Error); }));
 
-    // Oh no! We are calling Unmap too early! However the callback gets fired only after we get
+    // The callback should get called immediately with UnmappedBeforeCallback status,
-    // an answer from the server that the mapAsync call was an error.
+    // not server-side error, even if the request fails on the server side
+    EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_UnmappedBeforeCallback, _))
+        .Times(1);
+
+    // Oh no! We are calling Unmap too early! The callback should get fired immediately
+    // before 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();
 }
 
@@ -237,18 +236,17 @@ TEST_F(WireBufferMappingReadTests, DestroyCalledTooEarlyForRead) {
     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
+    // The callback should get called immediately with DestroyedBeforeCallback status
-    // an answer from the server.
+    // even if the request succeeds on the server side
+    EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback, _))
+        .Times(1);
+
+    // Oh no! We are calling Destroy too early! The callback should get fired immediately
+    // before 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();
 }
 
@@ -261,17 +259,17 @@ TEST_F(WireBufferMappingReadTests, DestroyCalledTooEarlyForReadButServerSideErro
         .WillOnce(InvokeWithoutArgs(
             [&]() { api.CallBufferMapAsyncCallback(apiBuffer, WGPUBufferMapAsyncStatus_Error); }));
 
-    // Oh no! We are calling Destroy too early! However the callback gets fired only after we
+    // The callback should be called with the server-side error and not the
-    // get an answer from the server that the mapAsync call was an error.
+    // DestroyedBeforCallback..
+    EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback, _))
+        .Times(1);
+
+    // Oh no! We are calling Destroy too early! The callback should get fired immediately
+    // before 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();
 }
 
@@ -748,6 +746,16 @@ TEST_F(WireBufferMappingTests, MapAfterDisconnect) {
     wgpuBufferMapAsync(buffer, WGPUMapMode_Read, 0, kBufferSize, ToMockBufferMapCallback, this);
 }
 
+// Test that mappinga again while pending map immediately cause an error
+TEST_F(WireBufferMappingTests, PendingMapImmediateError) {
+    SetupBuffer(WGPUMapMode_Read);
+
+    wgpuBufferMapAsync(buffer, WGPUMapMode_Read, 0, kBufferSize, nullptr, this);
+
+    EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_Error, this)).Times(1);
+    wgpuBufferMapAsync(buffer, WGPUMapMode_Read, 0, kBufferSize, ToMockBufferMapCallback, this);
+}
+
 // Hack to pass in test context into user callback
 struct TestData {
     WireBufferMappingTests* pTest;
@@ -787,8 +795,9 @@ TEST_F(WireBufferMappingTests, MapInsideCallbackBeforeDisconnect) {
 
     FlushClient();
 
-    EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_DeviceLost, this))
+    EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_Error, this))
-        .Times(1 + testData.numRequests);
+        .Times(testData.numRequests);
+    EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_DeviceLost, this)).Times(1);
     GetWireClient()->Disconnect();
 }
 
@@ -806,9 +815,11 @@ TEST_F(WireBufferMappingWriteTests, MapInsideCallbackBeforeDestruction) {
 
     FlushClient();
 
+    EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_Error, this))
+        .Times(testData.numRequests);
     EXPECT_CALL(*mockBufferMapCallback,
                 Call(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback, this))
-        .Times(1 + testData.numRequests);
+        .Times(1);
     wgpuBufferRelease(buffer);
 }
 

src/dawn/wire/client/Buffer.cpp

@@ -159,20 +159,23 @@ Buffer::Buffer(const ObjectBaseParams& params,
       mDeviceIsAlive(device->GetAliveWeakPtr()) {}
 
 Buffer::~Buffer() {
-    ClearAllCallbacks(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
+    InvokeAndClearCallback(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
     FreeMappedData();
 }
 
 void Buffer::CancelCallbacksForDisconnect() {
-    ClearAllCallbacks(WGPUBufferMapAsyncStatus_DeviceLost);
+    InvokeAndClearCallback(WGPUBufferMapAsyncStatus_DeviceLost);
 }
 
-void Buffer::ClearAllCallbacks(WGPUBufferMapAsyncStatus status) {
+void Buffer::InvokeAndClearCallback(WGPUBufferMapAsyncStatus status) {
-    mRequests.CloseAll([status](MapRequestData* request) {
+    WGPUBufferMapCallback callback = mRequest.callback;
-        if (request->callback != nullptr) {
+    void* userdata = mRequest.userdata;
-            request->callback(status, request->userdata);
+    mRequest.callback = nullptr;
-        }
+    mRequest.userdata = nullptr;
-    });
+    if (callback != nullptr) {
+        callback(status, userdata);
+    }
+    mPendingMap = false;
 }
 
 void Buffer::MapAsync(WGPUMapModeFlags mode,
@@ -180,6 +183,10 @@ void Buffer::MapAsync(WGPUMapModeFlags mode,
                       size_t size,
                       WGPUBufferMapCallback callback,
                       void* userdata) {
+    if (mPendingMap) {
+        return callback(WGPUBufferMapAsyncStatus_Error, userdata);
+    }
+
     Client* client = GetClient();
     if (client->IsDisconnected()) {
         return callback(WGPUBufferMapAsyncStatus_DeviceLost, userdata);
@@ -190,25 +197,24 @@ void Buffer::MapAsync(WGPUMapModeFlags mode,
         size = mSize - offset;
     }
 
-    // Create the request structure that will hold information while this mapping is
+    // Set up the request structure that will hold information while this mapping is
     // in flight.
-    MapRequestData request = {};
+    mRequest.callback = callback;
-    request.callback = callback;
+    mRequest.userdata = userdata;
-    request.userdata = userdata;
+    mRequest.offset = offset;
-    request.offset = offset;
+    mRequest.size = size;
-    request.size = size;
     if (mode & WGPUMapMode_Read) {
-        request.type = MapRequestType::Read;
+        mRequest.type = MapRequestType::Read;
     } else if (mode & WGPUMapMode_Write) {
-        request.type = MapRequestType::Write;
+        mRequest.type = MapRequestType::Write;
     }
 
-    uint64_t serial = mRequests.Add(std::move(request));
-
     // Serialize the command to send to the server.
+    mPendingMap = true;
+    mSerial++;
     BufferMapAsyncCmd cmd;
     cmd.bufferId = GetWireId();
-    cmd.requestSerial = serial;
+    cmd.requestSerial = mSerial;
     cmd.mode = mode;
     cmd.offset = offset;
     cmd.size = size;
@@ -220,26 +226,26 @@ bool Buffer::OnMapAsyncCallback(uint64_t requestSerial,
                                 uint32_t status,
                                 uint64_t readDataUpdateInfoLength,
                                 const uint8_t* readDataUpdateInfo) {
-    MapRequestData request;
+    // If requestSerial doesn't match mSerial the corresponding request must have
-    if (!mRequests.Acquire(requestSerial, &request)) {
+    // already been rejected by unmap or destroy and another MapAsync request must
-        return false;
+    // have been issued.
+    if (mSerial != requestSerial) {
+        return true;
     }
 
-    auto FailRequest = [&request]() -> bool {
+    // If mPendingMap is false the request must have been already rejected
-        if (request.callback != nullptr) {
+    // by unmap or destroy.
-            request.callback(WGPUBufferMapAsyncStatus_DeviceLost, request.userdata);
+    if (!mPendingMap) {
-        }
+        return true;
+    }
+
+    auto FailRequest = [this]() -> bool {
+        InvokeAndClearCallback(WGPUBufferMapAsyncStatus_DeviceLost);
         return false;
     };
 
-    // 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;
+        switch (mRequest.type) {
-    }
-
-    if (status == WGPUBufferMapAsyncStatus_Success) {
-        switch (request.type) {
             case MapRequestType::Read: {
                 if (readDataUpdateInfoLength > std::numeric_limits<size_t>::max()) {
                     // This is the size of data deserialized from the command stream, which must
@@ -254,7 +260,7 @@ bool Buffer::OnMapAsyncCallback(uint64_t requestSerial,
                 // Update user map data with server returned data
                 if (!mReadHandle->DeserializeDataUpdate(
                         readDataUpdateInfo, static_cast<size_t>(readDataUpdateInfoLength),
-                        request.offset, request.size)) {
+                        mRequest.offset, mRequest.size)) {
                     return FailRequest();
                 }
                 mMapState = MapState::MappedForRead;
@@ -273,13 +279,11 @@ bool Buffer::OnMapAsyncCallback(uint64_t requestSerial,
                 UNREACHABLE();
         }
 
-        mMapOffset = request.offset;
+        mMapOffset = mRequest.offset;
-        mMapSize = request.size;
+        mMapSize = mRequest.size;
     }
 
-    if (request.callback) {
+    InvokeAndClearCallback(static_cast<WGPUBufferMapAsyncStatus>(status));
-        request.callback(static_cast<WGPUBufferMapAsyncStatus>(status), request.userdata);
-    }
 
     return true;
 }
@@ -354,12 +358,7 @@ void Buffer::Unmap() {
     mMapOffset = 0;
     mMapSize = 0;
 
-    // Tag all mapping requests still in flight as unmapped before callback.
+    InvokeAndClearCallback(WGPUBufferMapAsyncStatus_UnmappedBeforeCallback);
-    mRequests.ForAll([](MapRequestData* request) {
-        if (request->clientStatus == WGPUBufferMapAsyncStatus_Success) {
-            request->clientStatus = WGPUBufferMapAsyncStatus_UnmappedBeforeCallback;
-        }
-    });
 
     BufferUnmapCmd cmd;
     cmd.self = ToAPI(this);
@@ -372,12 +371,7 @@ void Buffer::Destroy() {
     // Remove the current mapping and destroy Read/WriteHandles.
     FreeMappedData();
 
-    // Tag all mapping requests still in flight as destroyed before callback.
+    InvokeAndClearCallback(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
-    mRequests.ForAll([](MapRequestData* request) {
-        if (request->clientStatus == WGPUBufferMapAsyncStatus_Success) {
-            request->clientStatus = WGPUBufferMapAsyncStatus_DestroyedBeforeCallback;
-        }
-    });
 
     BufferDestroyCmd cmd;
     cmd.self = ToAPI(this);

src/dawn/wire/client/Buffer.h

@@ -20,7 +20,6 @@
 #include "dawn/webgpu.h"
 #include "dawn/wire/WireClient.h"
 #include "dawn/wire/client/ObjectBase.h"
-#include "dawn/wire/client/RequestTracker.h"
 
 namespace dawn::wire::client {
 
@@ -55,7 +54,7 @@ class Buffer final : public ObjectBase {
 
   private:
     void CancelCallbacksForDisconnect() override;
-    void ClearAllCallbacks(WGPUBufferMapAsyncStatus status);
+    void InvokeAndClearCallback(WGPUBufferMapAsyncStatus status);
 
     bool IsMappedForReading() const;
     bool IsMappedForWriting() const;
@@ -72,28 +71,22 @@ class Buffer final : public ObjectBase {
         MappedAtCreation,
     };
 
-    // We want to defer all the validation to the server, which means we could have multiple
+    // Up to only one request can exist at a single time.
-    // map request in flight at a single time and need to track them separately.
+    // Other requests are rejected.
-    // On well-behaved applications, only one request should exist at a single time.
     struct MapRequestData {
         WGPUBufferMapCallback callback = nullptr;
         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;
-
         MapRequestType type = MapRequestType::None;
     };
-    RequestTracker<MapRequestData> mRequests;
+    MapRequestData mRequest;
+    bool mPendingMap = false;
+    uint64_t mSerial = 0;
     uint64_t mSize = 0;
     WGPUBufferUsage mUsage;
 
-    // Only one mapped pointer can be active at a time because Unmap clears all the in-flight
+    // Only one mapped pointer can be active at a time
-    // requests.
     // TODO(enga): Use a tagged pointer to save space.
     std::unique_ptr<MemoryTransferService::ReadHandle> mReadHandle = nullptr;
     std::unique_ptr<MemoryTransferService::WriteHandle> mWriteHandle = nullptr;

webgpu-cts/expectations.txt

@@ -465,6 +465,16 @@ crbug.com/dawn/1613 worker_webgpu:api,validation,buffer,mapping:getMappedRange,s
 crbug.com/dawn/1613 worker_webgpu:api,validation,buffer,mapping:mapAsync,state,mappingPending:* [ Failure ]
 ################################################################################
 
+################################################################################
+# Buffer mapping tests need updates in CTS
+################################################################################
+webgpu:api,operation,buffers,map_oom:mapAsync:* [ Failure ]
+webgpu:api,validation,buffer,mapping:getMappedRange,state,mapped:* [ Failure ]
+webgpu:api,validation,buffer,mapping:getMappedRange,state,mappedAtCreation:* [ Failure ]
+worker_webgpu:api,validation,buffer,mapping:getMappedRange,state,mapped:* [ Failure ]
+worker_webgpu:api,validation,buffer,mapping:getMappedRange,state,mappedAtCreation:* [ Failure ]
+################################################################################
+
 ################################################################################
 # untriaged failures
 # KEEP