Also reject mapAsync if the buffer is being mapped

To reflect the WebGPU spec change at
https://github.com/gpuweb/gpuweb/pull/3348

Bug: chromium:1355994
Change-Id: I0159cbd9e1977a05453e3c562a2b0649a0ff930f
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/110448
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Takahiro <hogehoge@gachapin.jp>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>

src/dawn/native/Buffer.cpp

@@ -37,25 +37,24 @@ namespace dawn::native {
 
 namespace {
 struct MapRequestTask : TrackTaskCallback {
-    MapRequestTask(dawn::platform::Platform* platform, Ref<BufferBase> buffer, MapRequestID id)
+    MapRequestTask(dawn::platform::Platform* platform, Ref<BufferBase> buffer)
-        : TrackTaskCallback(platform), buffer(std::move(buffer)), id(id) {}
+        : TrackTaskCallback(platform), buffer(std::move(buffer)) {}
     void Finish() override {
         ASSERT(mSerial != kMaxExecutionSerial);
         TRACE_EVENT1(mPlatform, General, "Buffer::TaskInFlight::Finished", "serial",
                      uint64_t(mSerial));
-        buffer->OnMapRequestCompleted(id, WGPUBufferMapAsyncStatus_Success);
+        buffer->OnMapRequestCompleted(WGPUBufferMapAsyncStatus_Success);
     }
     void HandleDeviceLoss() override {
-        buffer->OnMapRequestCompleted(id, WGPUBufferMapAsyncStatus_DeviceLost);
+        buffer->OnMapRequestCompleted(WGPUBufferMapAsyncStatus_DeviceLost);
     }
     void HandleShutDown() override {
-        buffer->OnMapRequestCompleted(id, WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
+        buffer->OnMapRequestCompleted(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
     }
     ~MapRequestTask() override = default;
 
   private:
     Ref<BufferBase> buffer;
-    MapRequestID id;
 };
 
 class ErrorBuffer final : public BufferBase {
@@ -194,7 +193,7 @@ BufferBase::~BufferBase() {
 }
 
 void BufferBase::DestroyImpl() {
-    if (mState == BufferState::Mapped) {
+    if (mState == BufferState::Mapped || mState == BufferState::PendingMap) {
         UnmapInternal(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
     } else if (mState == BufferState::MappedAtCreation) {
         if (mStagingBuffer != nullptr) {
@@ -311,15 +310,17 @@ MaybeError BufferBase::ValidateCanUseOnQueueNow() const {
         case BufferState::Mapped:
         case BufferState::MappedAtCreation:
             return DAWN_VALIDATION_ERROR("%s used in submit while mapped.", this);
+        case BufferState::PendingMap:
+            return DAWN_VALIDATION_ERROR("%s used in submit while pending map.", this);
         case BufferState::Unmapped:
             return {};
     }
     UNREACHABLE();
 }
 
-void BufferBase::CallMapCallback(MapRequestID mapID, WGPUBufferMapAsyncStatus status) {
+void BufferBase::CallMapCallback(WGPUBufferMapAsyncStatus status) {
     ASSERT(!IsError());
-    if (mMapCallback != nullptr && mapID == mLastMapID) {
+    if (mMapCallback != nullptr) {
         // Tag the callback as fired before firing it, otherwise it could fire a second time if
         // for example buffer.Unmap() is called inside the application-provided callback.
         WGPUBufferMapCallback callback = mMapCallback;
@@ -330,6 +331,8 @@ void BufferBase::CallMapCallback(MapRequestID mapID, WGPUBufferMapAsyncStatus st
         } else {
             callback(status, mMapUserdata);
         }
+
+        mMapUserdata = 0;
     }
 }
 
@@ -356,20 +359,19 @@ void BufferBase::APIMapAsync(wgpu::MapMode mode,
     }
     ASSERT(!IsError());
 
-    mLastMapID++;
     mMapMode = mode;
     mMapOffset = offset;
     mMapSize = size;
     mMapCallback = callback;
     mMapUserdata = userdata;
-    mState = BufferState::Mapped;
+    mState = BufferState::PendingMap;
 
     if (GetDevice()->ConsumedError(MapAsyncImpl(mode, offset, size))) {
-        CallMapCallback(mLastMapID, WGPUBufferMapAsyncStatus_DeviceLost);
+        CallMapCallback(WGPUBufferMapAsyncStatus_DeviceLost);
         return;
     }
     std::unique_ptr<MapRequestTask> request =
-        std::make_unique<MapRequestTask>(GetDevice()->GetPlatform(), this, mLastMapID);
+        std::make_unique<MapRequestTask>(GetDevice()->GetPlatform(), this);
     TRACE_EVENT1(GetDevice()->GetPlatform(), General, "Buffer::APIMapAsync", "serial",
                  uint64_t(GetDevice()->GetPendingCommandSerial()));
     GetDevice()->GetQueue()->TrackTask(std::move(request));
@@ -438,16 +440,11 @@ void BufferBase::Unmap() {
 }
 
 void BufferBase::UnmapInternal(WGPUBufferMapAsyncStatus callbackStatus) {
-    if (mState == BufferState::Mapped) {
+    if (mState == BufferState::PendingMap) {
-        // A map request can only be called once, so this will fire only if the request wasn't
+        CallMapCallback(callbackStatus);
-        // completed before the Unmap.
+        UnmapImpl();
-        // Callbacks are not fired if there is no callback registered, so this is correct for
+    } else if (mState == BufferState::Mapped) {
-        // mappedAtCreation = true.
-        CallMapCallback(mLastMapID, callbackStatus);
         UnmapImpl();
-
-        mMapCallback = nullptr;
-        mMapUserdata = 0;
     } else if (mState == BufferState::MappedAtCreation) {
         if (mStagingBuffer != nullptr) {
             GetDevice()->ConsumedError(CopyFromStagingBuffer());
@@ -483,6 +480,8 @@ MaybeError BufferBase::ValidateMapAsync(wgpu::MapMode mode,
         case BufferState::Mapped:
         case BufferState::MappedAtCreation:
             return DAWN_VALIDATION_ERROR("%s is already mapped.", this);
+        case BufferState::PendingMap:
+            return DAWN_VALIDATION_ERROR("%s is pending map.", this);
         case BufferState::Destroyed:
             return DAWN_VALIDATION_ERROR("%s is destroyed.", this);
         case BufferState::Unmapped:
@@ -542,6 +541,7 @@ bool BufferBase::CanGetMappedRange(bool writable, size_t offset, size_t size) co
             ASSERT(bool{mMapMode & wgpu::MapMode::Read} ^ bool{mMapMode & wgpu::MapMode::Write});
             return !writable || (mMapMode & wgpu::MapMode::Write);
 
+        case BufferState::PendingMap:
         case BufferState::Unmapped:
         case BufferState::Destroyed:
             return false;
@@ -554,8 +554,11 @@ MaybeError BufferBase::ValidateUnmap() const {
     return {};
 }
 
-void BufferBase::OnMapRequestCompleted(MapRequestID mapID, WGPUBufferMapAsyncStatus status) {
+void BufferBase::OnMapRequestCompleted(WGPUBufferMapAsyncStatus status) {
-    CallMapCallback(mapID, status);
+    if (status == WGPUBufferMapAsyncStatus_Success && mState == BufferState::PendingMap) {
+        mState = BufferState::Mapped;
+    }
+    CallMapCallback(status);
 }
 
 bool BufferBase::NeedsInitialization() const {

src/dawn/native/Buffer.h

@@ -44,6 +44,7 @@ class BufferBase : public ApiObjectBase {
   public:
     enum class BufferState {
         Unmapped,
+        PendingMap,
         Mapped,
         MappedAtCreation,
         Destroyed,
@@ -62,7 +63,7 @@ class BufferBase : public ApiObjectBase {
     wgpu::BufferUsage GetUsageExternalOnly() const;
 
     MaybeError MapAtCreation();
-    void OnMapRequestCompleted(MapRequestID mapID, WGPUBufferMapAsyncStatus status);
+    void OnMapRequestCompleted(WGPUBufferMapAsyncStatus status);
 
     MaybeError ValidateCanUseOnQueueNow() const;
 
@@ -109,7 +110,7 @@ class BufferBase : public ApiObjectBase {
 
     virtual bool IsCPUWritableAtCreation() const = 0;
     MaybeError CopyFromStagingBuffer();
-    void CallMapCallback(MapRequestID mapID, WGPUBufferMapAsyncStatus status);
+    void CallMapCallback(WGPUBufferMapAsyncStatus status);
 
     MaybeError ValidateMapAsync(wgpu::MapMode mode,
                                 size_t offset,
@@ -128,7 +129,6 @@ class BufferBase : public ApiObjectBase {
 
     WGPUBufferMapCallback mMapCallback = nullptr;
     void* mMapUserdata = 0;
-    MapRequestID mLastMapID = MapRequestID(0);
     wgpu::MapMode mMapMode = wgpu::MapMode::None;
     size_t mMapOffset = 0;
     size_t mMapSize = 0;

src/dawn/tests/end2end/DeviceLostTests.cpp

@@ -55,6 +55,24 @@ class DeviceLostTest : public DawnTest {
         EXPECT_EQ(WGPUBufferMapAsyncStatus_DeviceLost, status);
         EXPECT_EQ(&fakeUserData, userdata);
     }
+
+    void MapAsyncAndWait(const wgpu::Buffer& buffer,
+                         wgpu::MapMode mode,
+                         size_t offset,
+                         size_t size) {
+        bool done = false;
+        buffer.MapAsync(
+            mode, offset, size,
+            [](WGPUBufferMapAsyncStatus status, void* userdata) {
+                ASSERT_EQ(WGPUBufferMapAsyncStatus_Success, status);
+                *static_cast<bool*>(userdata) = true;
+            },
+            &done);
+
+        while (!done) {
+            WaitABit();
+        }
+    }
 };
 
 // Test that DeviceLostCallback is invoked when LostForTestimg is called
@@ -338,7 +356,7 @@ TEST_P(DeviceLostTest, GetMappedRange_MapAsyncReading) {
     desc.usage = wgpu::BufferUsage::MapRead | wgpu::BufferUsage::CopyDst;
     wgpu::Buffer buffer = device.CreateBuffer(&desc);
 
-    buffer.MapAsync(wgpu::MapMode::Read, 0, 4, nullptr, nullptr);
+    MapAsyncAndWait(buffer, wgpu::MapMode::Read, 0, 4);
     queue.Submit(0, nullptr);
 
     const void* rangeBeforeLoss = buffer.GetConstMappedRange();
@@ -355,7 +373,7 @@ TEST_P(DeviceLostTest, GetMappedRange_MapAsyncWriting) {
     desc.usage = wgpu::BufferUsage::MapWrite | wgpu::BufferUsage::CopySrc;
     wgpu::Buffer buffer = device.CreateBuffer(&desc);
 
-    buffer.MapAsync(wgpu::MapMode::Write, 0, 4, nullptr, nullptr);
+    MapAsyncAndWait(buffer, wgpu::MapMode::Write, 0, 4);
     queue.Submit(0, nullptr);
 
     const void* rangeBeforeLoss = buffer.GetConstMappedRange();

src/dawn/tests/unittests/validation/BufferValidationTests.cpp

@@ -294,6 +294,7 @@ TEST_F(BufferValidationTest, MapAsync_AlreadyMapped) {
     {
         wgpu::Buffer buffer = CreateMapReadBuffer(4);
         buffer.MapAsync(wgpu::MapMode::Read, 0, 4, nullptr, nullptr);
+        WaitForAllOperations(device);
         AssertMapAsyncError(buffer, wgpu::MapMode::Read, 0, 4);
     }
     {
@@ -303,6 +304,7 @@ TEST_F(BufferValidationTest, MapAsync_AlreadyMapped) {
     {
         wgpu::Buffer buffer = CreateMapWriteBuffer(4);
         buffer.MapAsync(wgpu::MapMode::Write, 0, 4, nullptr, nullptr);
+        WaitForAllOperations(device);
         AssertMapAsyncError(buffer, wgpu::MapMode::Write, 0, 4);
     }
     {
@@ -311,6 +313,53 @@ TEST_F(BufferValidationTest, MapAsync_AlreadyMapped) {
     }
 }
 
+// Test map async with a buffer that's pending map
+TEST_F(BufferValidationTest, MapAsync_PendingMap) {
+    // Read + overlapping range
+    {
+        wgpu::Buffer buffer = CreateMapReadBuffer(4);
+        // The first map async call should succeed while the second one should fail
+        buffer.MapAsync(wgpu::MapMode::Read, 0, 4, ToMockBufferMapAsyncCallback, nullptr);
+        AssertMapAsyncError(buffer, wgpu::MapMode::Read, 0, 4);
+        EXPECT_CALL(*mockBufferMapAsyncCallback, Call(WGPUBufferMapAsyncStatus_Success, nullptr))
+            .Times(1);
+        WaitForAllOperations(device);
+    }
+
+    // Read + non-overlapping range
+    {
+        wgpu::Buffer buffer = CreateMapReadBuffer(16);
+        // The first map async call should succeed while the second one should fail
+        buffer.MapAsync(wgpu::MapMode::Read, 0, 8, ToMockBufferMapAsyncCallback, nullptr);
+        AssertMapAsyncError(buffer, wgpu::MapMode::Read, 8, 8);
+        EXPECT_CALL(*mockBufferMapAsyncCallback, Call(WGPUBufferMapAsyncStatus_Success, nullptr))
+            .Times(1);
+        WaitForAllOperations(device);
+    }
+
+    // Write + overlapping range
+    {
+        wgpu::Buffer buffer = CreateMapWriteBuffer(4);
+        // The first map async call should succeed while the second one should fail
+        buffer.MapAsync(wgpu::MapMode::Write, 0, 4, ToMockBufferMapAsyncCallback, nullptr);
+        AssertMapAsyncError(buffer, wgpu::MapMode::Write, 0, 4);
+        EXPECT_CALL(*mockBufferMapAsyncCallback, Call(WGPUBufferMapAsyncStatus_Success, nullptr))
+            .Times(1);
+        WaitForAllOperations(device);
+    }
+
+    // Write + non-overlapping range
+    {
+        wgpu::Buffer buffer = CreateMapWriteBuffer(16);
+        // The first map async call should succeed while the second one should fail
+        buffer.MapAsync(wgpu::MapMode::Write, 0, 8, ToMockBufferMapAsyncCallback, nullptr);
+        AssertMapAsyncError(buffer, wgpu::MapMode::Write, 8, 8);
+        EXPECT_CALL(*mockBufferMapAsyncCallback, Call(WGPUBufferMapAsyncStatus_Success, nullptr))
+            .Times(1);
+        WaitForAllOperations(device);
+    }
+}
+
 // Test map async with a buffer that's destroyed
 TEST_F(BufferValidationTest, MapAsync_Destroy) {
     {