[WebGPU] CTS test validation/buffer/mapping.html is failing

https://bugs.webkit.org/show_bug.cgi?id=266497
<radar://119731119>

Reviewed by Tadeu Zagallo.

Add correct validation for Buffer mapping.

With this change, all GPUBuffer CTS tests are passing.

* LayoutTests/http/tests/webgpu/webgpu/api/validation/buffer/mapping-expected.txt:
* Source/WebCore/Modules/WebGPU/GPUBuffer.cpp:
(WebCore::GPUBuffer::GPUBuffer):
(WebCore::GPUBuffer::mapAsync):
(WebCore::containsRange):
(WebCore::GPUBuffer::getMappedRange):
(WebCore::GPUBuffer::internalUnmap):
(WebCore::GPUBuffer::destroy):
* Source/WebCore/Modules/WebGPU/GPUBuffer.h:
* Source/WebGPU/WebGPU/Buffer.h:
* Source/WebGPU/WebGPU/Buffer.mm:
(WebGPU::Buffer::errorValidatingMapAsync const):
(WebGPU::Buffer::mapAsync):
(WebGPU::Buffer::validateUnmap const):
(WebGPU::Buffer::unmap):
(WebGPU::Buffer::validateMapAsync const): Deleted.
* Source/WebKit/GPUProcess/graphics/WebGPU/RemoteBuffer.cpp:
(WebKit::RemoteBuffer::mapAsync):
(WebKit::RemoteBuffer::unmap):
* Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteBufferProxy.cpp:
(WebKit::WebGPU::RemoteBufferProxy::unmap):

Canonical link: https://commits.webkit.org/272526@main

LayoutTests/http/tests/webgpu/webgpu/api/validation/buffer/mapping-expected.txt

@@ -1 +1,37 @@
-(Populate me when we're ready to investigate this test)
+
+PASS :mapAsync,usage:
+PASS :mapAsync,invalidBuffer:
+PASS :mapAsync,state,destroyed:
+PASS :mapAsync,state,mappedAtCreation:
+PASS :mapAsync,state,mapped:
+PASS :mapAsync,state,mappingPending:
+PASS :mapAsync,sizeUnspecifiedOOB:
+PASS :mapAsync,offsetAndSizeAlignment:
+PASS :mapAsync,offsetAndSizeOOB:
+PASS :mapAsync,earlyRejection:
+PASS :mapAsync,abort_over_invalid_error:
+PASS :getMappedRange,state,mapped:
+PASS :getMappedRange,state,mappedAtCreation:
+PASS :getMappedRange,state,invalid_mappedAtCreation:
+PASS :getMappedRange,state,mappedAgain:
+PASS :getMappedRange,state,unmapped:
+PASS :getMappedRange,state,destroyed:
+PASS :getMappedRange,state,mappingPending:
+PASS :getMappedRange,subrange,mapped:mapMode=1
+PASS :getMappedRange,subrange,mapped:mapMode=2
+PASS :getMappedRange,subrange,mappedAtCreation:
+PASS :getMappedRange,offsetAndSizeAlignment,mapped:mapMode=1
+PASS :getMappedRange,offsetAndSizeAlignment,mapped:mapMode=2
+PASS :getMappedRange,offsetAndSizeAlignment,mappedAtCreation:
+PASS :getMappedRange,sizeAndOffsetOOB,mappedAtCreation:
+PASS :getMappedRange,sizeAndOffsetOOB,mapped:
+PASS :getMappedRange,disjointRanges:
+PASS :getMappedRange,disjoinRanges_many:
+PASS :unmap,state,unmapped:
+PASS :unmap,state,destroyed:
+PASS :unmap,state,mappedAtCreation:
+PASS :unmap,state,mapped:
+PASS :unmap,state,mappingPending:
+PASS :gc_behavior,mappedAtCreation:
+PASS :gc_behavior,mapAsync:
+

Source/WebCore/Modules/WebGPU/GPUBuffer.cpp

@@ -45,7 +45,10 @@ GPUBuffer::GPUBuffer(Ref<WebGPU::Buffer>&& backing, size_t bufferSize, GPUBuffer
     , m_usage(usage)
     , m_mapState(mappedAtCreation ? GPUBufferMapState::Mapped : GPUBufferMapState::Unmapped)
     , m_device(device)
+    , m_mappedAtCreation(mappedAtCreation)
 {
+    if (mappedAtCreation)
+        m_mappedRangeSize = m_bufferSize;
 }
 
 String GPUBuffer::label() const
@@ -60,13 +63,8 @@ void GPUBuffer::setLabel(String&& label)
 
 void GPUBuffer::mapAsync(GPUMapModeFlags mode, std::optional<GPUSize64> offset, std::optional<GPUSize64> size, MapAsyncPromise&& promise)
 {
-    if (!m_bufferSize || (size.has_value() && !size.value())) {
-        promise.resolve(nullptr);
-        return;
-    }
-
     if (m_pendingMapPromise) {
-        promise.reject(Exception { ExceptionCode::OperationError });
+        promise.reject(Exception { ExceptionCode::OperationError, "pendingMapPromise"_s });
         return;
     }
 
@@ -75,20 +73,26 @@ void GPUBuffer::mapAsync(GPUMapModeFlags mode, std::optional<GPUSize64> offset,
 
     m_pendingMapPromise = promise;
     // FIXME: Should this capture a weak pointer to |this| instead?
-    m_backing->mapAsync(convertMapModeFlagsToBacking(mode), offset.value_or(0), size, [promise = WTFMove(promise), protectedThis = Ref { *this }](bool success) mutable {
+    m_backing->mapAsync(convertMapModeFlagsToBacking(mode), offset.value_or(0), size, [promise = WTFMove(promise), protectedThis = Ref { *this }, offset, size](bool success) mutable {
         if (!protectedThis->m_pendingMapPromise) {
-            promise.resolve(nullptr);
+            if (protectedThis->m_destroyed)
+                promise.reject(Exception { ExceptionCode::OperationError, "buffer destroyed during mapAsync"_s });
+            else
+                promise.resolve(nullptr);
             return;
         }
 
         protectedThis->m_pendingMapPromise = std::nullopt;
         if (success) {
             protectedThis->m_mapState = GPUBufferMapState::Mapped;
+            protectedThis->m_mappedRangeOffset = offset.value_or(0);
+            protectedThis->m_mappedRangeSize = size.value_or(protectedThis->m_bufferSize - protectedThis->m_mappedRangeOffset);
             promise.resolve(nullptr);
         } else {
             if (protectedThis->m_mapState == GPUBufferMapState::Pending)
                 protectedThis->m_mapState = GPUBufferMapState::Unmapped;
-            promise.reject(Exception { ExceptionCode::OperationError });
+
+            promise.reject(Exception { ExceptionCode::OperationError, "map async was not successful"_s });
         }
     });
 }
@@ -102,16 +106,77 @@ static auto makeArrayBuffer(auto source, auto byteLength, auto& cachedArrayBuffe
     return arrayBuffer;
 }
 
-ExceptionOr<Ref<JSC::ArrayBuffer>> GPUBuffer::getMappedRange(std::optional<GPUSize64> offset, std::optional<GPUSize64> size)
+static bool containsRange(size_t offset, size_t endOffset, const auto& mappedRanges, const auto& mappedPoints)
+{
+    if (offset == endOffset) {
+        if (mappedPoints.contains(offset))
+            return true;
+
+        for (auto& range : mappedRanges) {
+            if (range.begin() < offset && offset < range.end())
+                return true;
+        }
+        return false;
+    }
+
+    if (mappedRanges.overlaps({ offset, endOffset }))
+        return true;
+
+    for (auto& i : mappedPoints) {
+        if (offset < i && i < endOffset)
+            return true;
+    }
+
+    return false;
+}
+
+ExceptionOr<Ref<JSC::ArrayBuffer>> GPUBuffer::getMappedRange(std::optional<GPUSize64> optionalOffset, std::optional<GPUSize64> optionalSize)
 {
-    if (!m_bufferSize || (size.has_value() && !size.value()))
-        return makeArrayBuffer(static_cast<size_t>(0U), 1, m_arrayBuffer, m_device, *this);
+    if (m_mapState != GPUBufferMapState::Mapped || m_destroyed)
+        return Exception { ExceptionCode::OperationError, "not mapped or destroyed"_s };
 
-    // size is <= the size of the buffer is validated in WebGPU.framework
-    m_mappedRange = m_backing->getMappedRange(offset.value_or(0), size);
+    auto offset = optionalOffset.value_or(0);
+    if (offset > m_bufferSize)
+        return Exception { ExceptionCode::OperationError, "offset > bufferSize"_s };
+
+    auto size = optionalSize.value_or(m_bufferSize - offset);
+    auto checkedEndOffset = checkedSum<uint64_t>(offset, size);
+    if (checkedEndOffset.hasOverflowed())
+        return Exception { ExceptionCode::OperationError, "has overflowed"_s };
+
+    auto endOffset = checkedEndOffset.value();
+    if (offset % 8)
+        return Exception { ExceptionCode::OperationError, "validation failed offset % 8"_s };
+
+    if (size % 4)
+        return Exception { ExceptionCode::OperationError, "validation failed size % 4"_s };
+
+    if (offset < m_mappedRangeOffset)
+        return Exception { ExceptionCode::OperationError, "validation failed offset < m_mappedRangeOffset"_s };
+
+    if (endOffset > m_mappedRangeSize + m_mappedRangeOffset)
+        return Exception { ExceptionCode::OperationError, "getMappedRangeFailed because offset + size > mappedRangeSize + mappedRangeOffset"_s };
+
+    if (endOffset > m_bufferSize)
+        return Exception { ExceptionCode::OperationError, "validation failed endOffset > bufferSie"_s };
+
+    if (containsRange(offset, endOffset, m_mappedRanges, m_mappedPoints))
+        return Exception { ExceptionCode::OperationError, "validation failed - containsRange"_s };
+
+    if (offset == endOffset)
+        m_mappedPoints.add(offset);
+    else {
+        m_mappedRanges.add({ static_cast<size_t>(offset), static_cast<size_t>(endOffset) });
+        m_mappedRanges.compact();
+    }
+
+    m_mappedRange = m_backing->getMappedRange(offset, size);
     if (!m_mappedRange.source) {
         m_arrayBuffer = nullptr;
-        return Exception { ExceptionCode::OperationError };
+        if (m_mappedAtCreation || !size)
+            return makeArrayBuffer(static_cast<size_t>(0U), 1, m_arrayBuffer, m_device, *this);
+
+        return Exception { ExceptionCode::OperationError, "getMappedRange failed"_s };
     }
 
     return makeArrayBuffer(m_mappedRange.source, m_mappedRange.byteLength, m_arrayBuffer, m_device, *this);
@@ -125,16 +190,19 @@ void GPUBuffer::unmap(ScriptExecutionContext& scriptExecutionContext)
 
 void GPUBuffer::internalUnmap(ScriptExecutionContext& scriptExecutionContext)
 {
+    m_mappedAtCreation = false;
+    m_mappedRangeOffset = 0;
+    m_mappedRangeSize = 0;
+    m_mappedRanges.clear();
+    m_mappedPoints.clear();
     if (m_pendingMapPromise) {
         m_pendingMapPromise->reject(Exception { ExceptionCode::AbortError });
         m_pendingMapPromise = std::nullopt;
     }
 
     m_mapState = GPUBufferMapState::Unmapped;
-    if (!m_bufferSize)
-        return;
 
-    if (m_arrayBuffer && m_arrayBuffer->data()) {
+    if (m_arrayBuffer && m_arrayBuffer->data() && m_mappedRange.byteLength) {
         memcpy(m_mappedRange.source, m_arrayBuffer->data(), m_mappedRange.byteLength);
         JSC::ArrayBufferContents emptyBuffer;
         m_arrayBuffer->unpin();
@@ -143,13 +211,12 @@ void GPUBuffer::internalUnmap(ScriptExecutionContext& scriptExecutionContext)
 
     m_arrayBuffer = nullptr;
     m_backing->unmap();
+    m_mappedRange = { nullptr, 0 };
 }
 
 void GPUBuffer::destroy(ScriptExecutionContext& scriptExecutionContext)
 {
-    if (!m_bufferSize)
-        return;
-
+    m_destroyed = true;
     internalUnmap(scriptExecutionContext);
     m_bufferSize = 0;
     m_backing->destroy();

Source/WebCore/Modules/WebGPU/GPUBuffer.h

@@ -35,6 +35,9 @@
 #include <JavaScriptCore/ArrayBuffer.h>
 #include <cstdint>
 #include <optional>
+#include <wtf/HashSet.h>
+#include <wtf/Range.h>
+#include <wtf/RangeSet.h>
 #include <wtf/Ref.h>
 #include <wtf/RefCounted.h>
 #include <wtf/text/WTFString.h>
@@ -77,10 +80,17 @@ class GPUBuffer : public RefCounted<GPUBuffer> {
     WebGPU::Buffer::MappedRange m_mappedRange;
     JSC::ArrayBuffer* m_arrayBuffer { nullptr };
     size_t m_bufferSize { 0 };
+    size_t m_mappedRangeOffset { 0 };
+    size_t m_mappedRangeSize { 0 };
     const GPUBufferUsageFlags m_usage { 0 };
     GPUBufferMapState m_mapState { GPUBufferMapState::Unmapped };
     std::optional<MapAsyncPromise> m_pendingMapPromise;
     GPUDevice& m_device;
+    using MappedRanges = WTF::RangeSet<WTF::Range<size_t>>;
+    MappedRanges m_mappedRanges;
+    HashSet<size_t, DefaultHash<size_t>, WTF::UnsignedWithZeroKeyHashTraits<size_t>> m_mappedPoints;
+    bool m_destroyed { false };
+    bool m_mappedAtCreation { false };
 };
 
 }

Source/WebGPU/WebGPU/Buffer.h

@@ -92,7 +92,7 @@ class Buffer : public WGPUBufferImpl, public RefCounted<Buffer> {
     Buffer(Device&);
 
     bool validateGetMappedRange(size_t offset, size_t rangeSize) const;
-    bool validateMapAsync(WGPUMapModeFlags, size_t offset, size_t rangeSize) const;
+    NSString* errorValidatingMapAsync(WGPUMapModeFlags, size_t offset, size_t rangeSize) const;
     bool validateUnmap() const;
 
     id<MTLBuffer> m_buffer { nil };

Source/WebGPU/WebGPU/Buffer.mm

@@ -229,35 +229,37 @@ static size_t computeRangeSize(uint64_t size, size_t offset)
     return static_cast<char*>(m_buffer.contents) + offset;
 }
 
-bool Buffer::validateMapAsync(WGPUMapModeFlags mode, size_t offset, size_t rangeSize) const
+NSString* Buffer::errorValidatingMapAsync(WGPUMapModeFlags mode, size_t offset, size_t rangeSize) const
 {
+#define ERROR_STRING(x) (@"GPUTexture.mapAsync: " x)
     if (!isValid())
-        return false;
+        return ERROR_STRING(@"Buffer is not valid");
 
     if (offset % 8)
-        return false;
+        return ERROR_STRING(@"Offset is not divisible by 8");
 
     if (rangeSize % 4)
-        return false;
+        return ERROR_STRING(@"range size is not divisible by 4");
 
     auto end = checkedSum<uint64_t>(offset, rangeSize);
     if (end.hasOverflowed() || end.value() > m_size)
-        return false;
+        return ERROR_STRING(@"offset and rangeSize overflowed");
 
-    if (m_state != State::Unmapped && m_state != State::MappedAtCreation)
-        return false;
+    if (m_state != State::Unmapped)
+        return ERROR_STRING(@"state != Unmapped");
 
     auto readWriteModeFlags = mode & (WGPUMapMode_Read | WGPUMapMode_Write);
     if (readWriteModeFlags != WGPUMapMode_Read && readWriteModeFlags != WGPUMapMode_Write)
-        return false;
+        return ERROR_STRING(@"readWriteModeFlags != Read && readWriteModeFlags != Write");
 
     if ((mode & WGPUMapMode_Read) && !(m_usage & WGPUBufferUsage_MapRead))
-        return false;
+        return ERROR_STRING(@"(mode & Read) && !(usage & Read)");
 
     if ((mode & WGPUMapMode_Write) && !(m_usage & WGPUBufferUsage_MapWrite))
-        return false;
+        return ERROR_STRING(@"(mode & Write) && !(usage & Write)");
 
-    return true;
+#undef ERROR_STRING
+    return nil;
 }
 
 void Buffer::mapAsync(WGPUMapModeFlags mode, size_t offset, size_t size, CompletionHandler<void(WGPUBufferMapAsyncStatus)>&& callback)
@@ -268,12 +270,10 @@ static size_t computeRangeSize(uint64_t size, size_t offset)
     if (size == WGPU_WHOLE_MAP_SIZE)
         rangeSize = computeRangeSize(m_size, offset);
 
-    if (!validateMapAsync(mode, offset, rangeSize)) {
-        m_device->generateAValidationError("Validation failure."_s);
+    if (NSString* error = errorValidatingMapAsync(mode, offset, rangeSize)) {
+        m_device->generateAValidationError(error);
 
-        m_device->instance().scheduleWork([callback = WTFMove(callback)]() mutable {
-            callback(WGPUBufferMapAsyncStatus_ValidationError);
-        });
+        callback(WGPUBufferMapAsyncStatus_ValidationError);
         return;
     }
 
@@ -313,19 +313,14 @@ static size_t computeRangeSize(uint64_t size, size_t offset)
 
 bool Buffer::validateUnmap() const
 {
-    if (m_state != State::MappedAtCreation
-        && m_state != State::MappingPending
-        && m_state != State::Mapped)
-        return false;
-
     return true;
 }
 
 void Buffer::unmap()
 {
     // https://gpuweb.github.io/gpuweb/#dom-gpubuffer-unmap
 
-    if (!validateUnmap())
+    if (!validateUnmap() && !m_device->isValid())
         return;
 
     // FIXME: "If this.[[state]] is mapping pending: Reject [[mapping]] with an AbortError."

Source/WebKit/GPUProcess/graphics/WebGPU/RemoteBuffer.cpp

@@ -55,13 +55,13 @@ void RemoteBuffer::mapAsync(WebCore::WebGPU::MapModeFlags mapModeFlags, WebCore:
     m_isMapped = true;
     m_mapModeFlags = mapModeFlags;
 
-    m_backing->mapAsync(mapModeFlags, offset, size, [offset, size, protectedThis = Ref<RemoteBuffer>(*this), callback = WTFMove(callback)] (bool success) mutable {
+    m_backing->mapAsync(mapModeFlags, offset, size, [protectedThis = Ref<RemoteBuffer>(*this), callback = WTFMove(callback)] (bool success) mutable {
         if (!success) {
             callback(std::nullopt);
             return;
         }
 
-        auto mappedRange = protectedThis->m_backing->getMappedRange(offset, size);
+        auto mappedRange = protectedThis->m_backing->getMappedRange(0, std::nullopt);
         protectedThis->m_mappedRange = mappedRange;
         callback(Vector<uint8_t>(static_cast<const uint8_t*>(mappedRange.source), mappedRange.byteLength));
     });
@@ -79,11 +79,7 @@ void RemoteBuffer::getMappedRange(WebCore::WebGPU::Size64 offset, std::optional<
 
 void RemoteBuffer::unmap(Vector<uint8_t>&& data)
 {
-    if (!m_mappedRange || m_mappedRange->byteLength < data.size())
-        return;
-    ASSERT(m_isMapped);
-
-    if (m_mapModeFlags.contains(WebCore::WebGPU::MapMode::Write))
+    if (m_isMapped && m_mappedRange && m_mappedRange->byteLength >= data.size() && m_mapModeFlags.contains(WebCore::WebGPU::MapMode::Write))
         memcpy(m_mappedRange->source, data.data(), data.size());
 
     m_backing->unmap();

Source/WebKit/WebProcess/GPU/graphics/WebGPU/RemoteBufferProxy.cpp

@@ -91,12 +91,8 @@ auto RemoteBufferProxy::getMappedRange(WebCore::WebGPU::Size64 offset, std::opti
 
 void RemoteBufferProxy::unmap()
 {
-    // FIXME: Implement error handling.
-    if (!m_data)
-        return;
-
     Vector<uint8_t> data;
-    if (m_mapModeFlags.contains(WebCore::WebGPU::MapMode::Write))
+    if (m_data && m_mapModeFlags.contains(WebCore::WebGPU::MapMode::Write))
         data = WTFMove(*m_data);
     auto sendResult = send(Messages::RemoteBuffer::Unmap(WTFMove(data)));
     UNUSED_VARIABLE(sendResult);