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>
diff --git a/src/dawn/tests/unittests/wire/WireBufferMappingTests.cpp b/src/dawn/tests/unittests/wire/WireBufferMappingTests.cpp
index 5a7bc23..20a7120 100644
--- a/src/dawn/tests/unittests/wire/WireBufferMappingTests.cpp
+++ b/src/dawn/tests/unittests/wire/WireBufferMappingTests.cpp
@@ -186,18 +186,17 @@
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.
+ // The callback should get called immediately with UnmappedBeforeCallback status
+ // 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 @@
.WillOnce(InvokeWithoutArgs(
[&]() { api.CallBufferMapAsyncCallback(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.
+ // The callback should get called immediately with UnmappedBeforeCallback status,
+ // 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 @@
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.
+ // The callback should get called immediately with DestroyedBeforeCallback status
+ // 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 @@
.WillOnce(InvokeWithoutArgs(
[&]() { api.CallBufferMapAsyncCallback(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.
+ // The callback should be called with the server-side error and not the
+ // 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 @@
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 @@
FlushClient();
- EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_DeviceLost, this))
- .Times(1 + testData.numRequests);
+ EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_Error, this))
+ .Times(testData.numRequests);
+ EXPECT_CALL(*mockBufferMapCallback, Call(WGPUBufferMapAsyncStatus_DeviceLost, this)).Times(1);
GetWireClient()->Disconnect();
}
@@ -806,9 +815,11 @@
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);
}
diff --git a/src/dawn/wire/client/Buffer.cpp b/src/dawn/wire/client/Buffer.cpp
index 43335ef..9c7e969 100644
--- a/src/dawn/wire/client/Buffer.cpp
+++ b/src/dawn/wire/client/Buffer.cpp
@@ -159,20 +159,23 @@
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) {
- mRequests.CloseAll([status](MapRequestData* request) {
- if (request->callback != nullptr) {
- request->callback(status, request->userdata);
- }
- });
+void Buffer::InvokeAndClearCallback(WGPUBufferMapAsyncStatus status) {
+ WGPUBufferMapCallback callback = mRequest.callback;
+ void* userdata = mRequest.userdata;
+ mRequest.callback = nullptr;
+ mRequest.userdata = nullptr;
+ if (callback != nullptr) {
+ callback(status, userdata);
+ }
+ mPendingMap = false;
}
void Buffer::MapAsync(WGPUMapModeFlags mode,
@@ -180,6 +183,10 @@
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 @@
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 = {};
- request.callback = callback;
- request.userdata = userdata;
- request.offset = offset;
- request.size = size;
+ mRequest.callback = callback;
+ mRequest.userdata = userdata;
+ mRequest.offset = offset;
+ mRequest.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 @@
uint32_t status,
uint64_t readDataUpdateInfoLength,
const uint8_t* readDataUpdateInfo) {
- MapRequestData request;
- if (!mRequests.Acquire(requestSerial, &request)) {
- return false;
+ // If requestSerial doesn't match mSerial the corresponding request must have
+ // already been rejected by unmap or destroy and another MapAsync request must
+ // have been issued.
+ if (mSerial != requestSerial) {
+ return true;
}
- auto FailRequest = [&request]() -> bool {
- if (request.callback != nullptr) {
- request.callback(WGPUBufferMapAsyncStatus_DeviceLost, request.userdata);
- }
+ // If mPendingMap is false the request must have been already rejected
+ // by unmap or destroy.
+ 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;
- }
-
- if (status == WGPUBufferMapAsyncStatus_Success) {
- switch (request.type) {
+ switch (mRequest.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 @@
// 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 @@
UNREACHABLE();
}
- mMapOffset = request.offset;
- mMapSize = request.size;
+ mMapOffset = mRequest.offset;
+ mMapSize = mRequest.size;
}
- if (request.callback) {
- request.callback(static_cast<WGPUBufferMapAsyncStatus>(status), request.userdata);
- }
+ InvokeAndClearCallback(static_cast<WGPUBufferMapAsyncStatus>(status));
return true;
}
@@ -354,12 +358,7 @@
mMapOffset = 0;
mMapSize = 0;
- // Tag all mapping requests still in flight as unmapped before callback.
- mRequests.ForAll([](MapRequestData* request) {
- if (request->clientStatus == WGPUBufferMapAsyncStatus_Success) {
- request->clientStatus = WGPUBufferMapAsyncStatus_UnmappedBeforeCallback;
- }
- });
+ InvokeAndClearCallback(WGPUBufferMapAsyncStatus_UnmappedBeforeCallback);
BufferUnmapCmd cmd;
cmd.self = ToAPI(this);
@@ -372,12 +371,7 @@
// Remove the current mapping and destroy Read/WriteHandles.
FreeMappedData();
- // Tag all mapping requests still in flight as destroyed before callback.
- mRequests.ForAll([](MapRequestData* request) {
- if (request->clientStatus == WGPUBufferMapAsyncStatus_Success) {
- request->clientStatus = WGPUBufferMapAsyncStatus_DestroyedBeforeCallback;
- }
- });
+ InvokeAndClearCallback(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
BufferDestroyCmd cmd;
cmd.self = ToAPI(this);
diff --git a/src/dawn/wire/client/Buffer.h b/src/dawn/wire/client/Buffer.h
index 8040016..1784c0c 100644
--- a/src/dawn/wire/client/Buffer.h
+++ b/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 @@
private:
void CancelCallbacksForDisconnect() override;
- void ClearAllCallbacks(WGPUBufferMapAsyncStatus status);
+ void InvokeAndClearCallback(WGPUBufferMapAsyncStatus status);
bool IsMappedForReading() const;
bool IsMappedForWriting() const;
@@ -72,28 +71,22 @@
MappedAtCreation,
};
- // We want to defer all the validation to the server, which means we could have multiple
- // map request in flight at a single time and need to track them separately.
- // On well-behaved applications, only one request should exist at a single time.
+ // Up to only one request can exist at a single time.
+ // Other requests are rejected.
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
- // requests.
+ // Only one mapped pointer can be active at a time
// TODO(enga): Use a tagged pointer to save space.
std::unique_ptr<MemoryTransferService::ReadHandle> mReadHandle = nullptr;
std::unique_ptr<MemoryTransferService::WriteHandle> mWriteHandle = nullptr;
diff --git a/webgpu-cts/expectations.txt b/webgpu-cts/expectations.txt
index d3320ee..dd51e5d 100644
--- a/webgpu-cts/expectations.txt
+++ b/webgpu-cts/expectations.txt
@@ -466,6 +466,16 @@
################################################################################
################################################################################
+# 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
################################################################################
