From f601680b9e0600c4469d84f876375fc6f2ae2028 Mon Sep 17 00:00:00 2001 From: Vivek Raj Date: Fri, 18 Jul 2025 12:19:31 +0530 Subject: [PATCH 1/3] Add early-exit validation to MapAsync for already mapped buffers and test case --- src/dawn/native/Buffer.cpp | 31 ++++++++++++++++++++ src/dawn/tests/end2end/BufferTests.cpp | 39 ++++++++++++++++++++++++++ 2 files changed, 70 insertions(+) diff --git a/src/dawn/native/Buffer.cpp b/src/dawn/native/Buffer.cpp index 9b85cb99e43..76134736d61 100644 --- a/src/dawn/native/Buffer.cpp +++ b/src/dawn/native/Buffer.cpp @@ -574,6 +574,34 @@ Future BufferBase::APIMapAsync(wgpu::MapMode mode, { auto deviceGuard = GetDevice()->GetGuard(); + // Early-exit validation: prevent double-mapping of the same buffer. + // + // According to the WebGPU specification, a buffer must not be mapped more than + // once at a time. If a buffer is already in a Mapped or MappedAtCreation state, + // calling MapAsync() again is invalid. + // + // Instead of deferring to the full validation path (ValidateMapAsync), + // we return early here with a warning and an error callback to improve + // developer feedback and reduce unnecessary work. + + BufferState currentState = mState.load(std::memory_order::acquire); + if (currentState == BufferState::Mapped || currentState == BufferState::MappedAtCreation) { + // Log a warning for developer visibility. This does not raise a device error. + dawn::EmitWarning(GetDevice(), + "Attempted to map a buffer that is already mapped. " + "Call Unmap() before calling MapAsync again."); + + // Trigger the callback immediately with an error status. + // This provides synchronous feedback for invalid usage. + if (callbackInfo.callback != nullptr) { + callbackInfo.callback(WGPUMapAsyncStatus_Error, callbackInfo.userdata); + } + + // Return a null future. No event is tracked for this invalid operation. + return Future::MakeEmpty(); + } + + // Handle the defaulting of size required by WebGPU, even if in webgpu_cpp.h it is not // possible to default the function argument (because there is the callback later in the // argument list) @@ -614,6 +642,9 @@ Future BufferBase::APIMapAsync(wgpu::MapMode mode, return {futureID}; } + +//****************************************************************************************************** */ + void* BufferBase::APIGetMappedRange(size_t offset, size_t size) { return GetMappedRange(offset, size, true); } diff --git a/src/dawn/tests/end2end/BufferTests.cpp b/src/dawn/tests/end2end/BufferTests.cpp index 05186b5ff26..4027cac74f2 100644 --- a/src/dawn/tests/end2end/BufferTests.cpp +++ b/src/dawn/tests/end2end/BufferTests.cpp @@ -1230,6 +1230,45 @@ TEST_P(BufferTests, CreateErrorBuffer) { ASSERT_EQ(buffer, nullptr); } +// Test that MapAsync fails if called twice on the same buffer without unmapping. +TEST_P(BufferValidationTests, MapAsyncFailsWhenAlreadyMapped) { + // Create a writable buffer with MapWrite and CopyDst usage. + wgpu::BufferDescriptor desc; + desc.size = 64; + desc.usage = wgpu::BufferUsage::MapWrite | wgpu::BufferUsage::CopyDst; + + wgpu::Buffer buffer = device.CreateBuffer(&desc); + + // First call to MapAsync should succeed. + bool callback1Called = false; + buffer.MapAsync(wgpu::MapMode::Write, 0, 64, + [&](WGPUMapAsyncStatus status) { + // Expect the first mapping to succeed. + EXPECT_EQ(status, WGPUMapAsyncStatus_Success); + callback1Called = true; + }); + + // Immediately call MapAsync again without unmapping. + bool callback2Called = false; + buffer.MapAsync(wgpu::MapMode::Write, 0, 64, + [&](WGPUMapAsyncStatus status) { + // The second mapping should fail and return an error. + EXPECT_EQ(status, WGPUMapAsyncStatus_Error); + callback2Called = true; + }); + + // Process the commands and trigger callbacks. + device.Tick(); + + // Ensure both callbacks were actually called. + EXPECT_TRUE(callback1Called); + EXPECT_TRUE(callback2Called); + + // Cleanup: Unmap the buffer after use. + buffer.Unmap(); +} + + // Test that mapping an OOM buffer fails gracefully TEST_P(BufferTests, CreateBufferOOMMapAsync) { // TODO(crbug.com/346377856): fails on ANGLE/D3D11, but is likely a Dawn/GL bug that only From 8d95bee1cbef599b6078f1d0c5d9d438d0c065ba Mon Sep 17 00:00:00 2001 From: Vivek Raj Date: Fri, 18 Jul 2025 15:46:00 +0530 Subject: [PATCH 2/3] Fix: Disable Dawn install to fix CI export target errors --- .github/workflows/dawn-ci.cmake | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/.github/workflows/dawn-ci.cmake b/.github/workflows/dawn-ci.cmake index 90c36d751fd..3e39337fd53 100644 --- a/.github/workflows/dawn-ci.cmake +++ b/.github/workflows/dawn-ci.cmake @@ -6,7 +6,9 @@ if (WIN32) set(CMAKE_VS_WINDOWS_TARGET_PLATFORM_VERSION "$ENV{WIN10_SDK_VERSION}" CACHE STRING "") endif () set(DAWN_FETCH_DEPENDENCIES ON CACHE BOOL "") -set(DAWN_ENABLE_INSTALL ON CACHE BOOL "") +# set(DAWN_ENABLE_INSTALL ON CACHE BOOL "") +set(DAWN_ENABLE_INSTALL OFF CACHE BOOL "") + if (CMAKE_SYTEM_NAME STREQUAL "Linux") # `sccache` seems effective only on linux. From ba69d220cb688c81bb7270b7f59cb6182ca60ae3 Mon Sep 17 00:00:00 2001 From: Vivek Raj Date: Mon, 11 Aug 2025 15:20:31 -0400 Subject: [PATCH 3/3] Remove redundant validation to avoid breaking API semantics --- src/dawn/native/Buffer.cpp | 46 +++++++++++--------------------------- 1 file changed, 13 insertions(+), 33 deletions(-) diff --git a/src/dawn/native/Buffer.cpp b/src/dawn/native/Buffer.cpp index 76134736d61..3b338f0af79 100644 --- a/src/dawn/native/Buffer.cpp +++ b/src/dawn/native/Buffer.cpp @@ -566,45 +566,24 @@ Future BufferBase::APIMapAsync(wgpu::MapMode mode, size_t offset, size_t size, const WGPUBufferMapCallbackInfo& callbackInfo) { - // TODO(crbug.com/dawn/2052): Once we always return a future, change this to log to the instance - // (note, not raise a validation error to the device) and return the null future. DAWN_ASSERT(callbackInfo.nextInChain == nullptr); Ref event; { auto deviceGuard = GetDevice()->GetGuard(); - // Early-exit validation: prevent double-mapping of the same buffer. - // - // According to the WebGPU specification, a buffer must not be mapped more than - // once at a time. If a buffer is already in a Mapped or MappedAtCreation state, - // calling MapAsync() again is invalid. - // - // Instead of deferring to the full validation path (ValidateMapAsync), - // we return early here with a warning and an error callback to improve - // developer feedback and reduce unnecessary work. - - BufferState currentState = mState.load(std::memory_order::acquire); - if (currentState == BufferState::Mapped || currentState == BufferState::MappedAtCreation) { - // Log a warning for developer visibility. This does not raise a device error. - dawn::EmitWarning(GetDevice(), - "Attempted to map a buffer that is already mapped. " - "Call Unmap() before calling MapAsync again."); - - // Trigger the callback immediately with an error status. - // This provides synchronous feedback for invalid usage. - if (callbackInfo.callback != nullptr) { - callbackInfo.callback(WGPUMapAsyncStatus_Error, callbackInfo.userdata); - } - - // Return a null future. No event is tracked for this invalid operation. - return Future::MakeEmpty(); - } - + // Remove redundant early-exit validation. + // Let ValidateMapAsync and MapAsyncImpl handle invalid states, + // so errors are reported through the device's uncaptured error mechanism. + // We can still log a warning for developer visibility. + BufferState currentState = mState.load(std::memory_order::acquire); + if (currentState == BufferState::Mapped || currentState == BufferState::MappedAtCreation) { + dawn::EmitWarning(GetDevice(), + "Attempted to map a buffer that is already mapped. " + "Call Unmap() before calling MapAsync again."); + } - // Handle the defaulting of size required by WebGPU, even if in webgpu_cpp.h it is not - // possible to default the function argument (because there is the callback later in the - // argument list) + // Handle the defaulting of size required by WebGPU if ((size == wgpu::kWholeMapSize) && (offset <= mSize)) { size = mSize - offset; } @@ -639,10 +618,11 @@ Future BufferBase::APIMapAsync(wgpu::MapMode mode, DAWN_ASSERT(event); FutureID futureID = GetInstance()->GetEventManager()->TrackEvent(std::move(event)); - return {futureID}; + return {futureID}; // Always return a valid FutureID } + //****************************************************************************************************** */ void* BufferBase::APIGetMappedRange(size_t offset, size_t size) {