[WebGPU] Fix flaky crashing tests

https://bugs.webkit.org/show_bug.cgi?id=268628 <radar://122189307> Reviewed by Tadeu Zagallo. The member variable, m_device, was being accessed off a single thread, in scheduleWork(), so it should be ThreadSafeWeakPtr instead of WeakPtr. * Source/WebGPU/WebGPU/Queue.h: * Source/WebGPU/WebGPU/Queue.mm: (WebGPU::Queue::commitMTLCommandBuffer): (WebGPU::Queue::submit): (WebGPU::Queue::writeBuffer): (WebGPU::Queue::device const): (WebGPU::Queue::clearTexture): (WebGPU::Queue::writeTexture): (WebGPU::Queue::scheduleWork): * Source/WebGPU/WebGPU/PipelineLayout.h: * Source/WebGPU/WebGPU/PipelineLayout.mm: (WebGPU::PipelineLayout::errorValidatingBindGroupCompatibility const): * Source/WebGPU/WebGPU/RenderBundleEncoder.mm: (WebGPU::RenderBundleEncoder::executePreDrawCommands): * Source/WebGPU/WebGPU/RenderPassEncoder.mm: (WebGPU::RenderPassEncoder::executePreDrawCommands): * Source/WebGPU/WebGPU/RenderPipeline.h: * Source/WebGPU/WebGPU/RenderPipeline.mm: (WebGPU::RenderPipeline::vertexStageInBufferCount const): Fix improper validation logic introduced with https://bugs.webkit.org/show_bug.cgi?id=267285 Canonical link: https://commits.webkit.org/274001@main
WebKit · Feb 2, 2024 · f24a54e · f24a54e
1 parent 1cb5156
commit f24a54e
Show file tree

Hide file tree

Showing 8 changed files with 34 additions and 17 deletions.
diff --git a/Source/WebGPU/WebGPU/PipelineLayout.h b/Source/WebGPU/WebGPU/PipelineLayout.h
@@ -80,7 +80,7 @@ class PipelineLayout : public WGPUPipelineLayoutImpl, public RefCounted<Pipeline
     const Vector<uint32_t>* fragmentOffsets(uint32_t, const Vector<uint32_t>&);
     const Vector<uint32_t>* computeOffsets(uint32_t, const Vector<uint32_t>&);
     using BindGroupHashMap = HashMap<uint32_t, WeakPtr<BindGroup>, DefaultHash<uint32_t>, WTF::UnsignedWithZeroKeyHashTraits<uint32_t>>;
-    NSString* errorValidatingBindGroupCompatibility(const BindGroupHashMap&) const;
+    NSString* errorValidatingBindGroupCompatibility(const BindGroupHashMap&, size_t vertexStageInBufferCount = 0) const;
 private:
     PipelineLayout(std::optional<Vector<Ref<BindGroupLayout>>>&&, Device&);
     PipelineLayout(Device&);

diff --git a/Source/WebGPU/WebGPU/PipelineLayout.mm b/Source/WebGPU/WebGPU/PipelineLayout.mm
@@ -246,18 +246,21 @@ static size_t returnOffsetOfGroup0(auto& v, auto groupIndex)
     return offsetVectorForBindGroup(bindGroupIndex, m_computeOffsets, dynamicOffsets, WGPUShaderStage_Compute);
 }
 
-NSString* PipelineLayout::errorValidatingBindGroupCompatibility(const PipelineLayout::BindGroupHashMap& bindGroups) const
+NSString* PipelineLayout::errorValidatingBindGroupCompatibility(const PipelineLayout::BindGroupHashMap& bindGroups, size_t vertexStageInBufferCount) const
 {
     auto setBindGroupsSize = bindGroups.size();
     if (!m_bindGroupLayouts)
-        return !setBindGroupsSize ? nil : @"bind groups were set but layout has no bind groups";
+        return (!setBindGroupsSize && !vertexStageInBufferCount) ? nil : @"bind groups were set but layout has no bind groups";
 
     auto& bindGroupLayouts = *m_bindGroupLayouts;
     auto numberOfBindGroupsInPipeline = bindGroupLayouts.size();
-    if (setBindGroupsSize < numberOfBindGroupsInPipeline)
+    if (setBindGroupsSize + vertexStageInBufferCount < numberOfBindGroupsInPipeline) {
+        if (numberOfBindGroupsInPipeline == 1 && !bindGroupLayouts[0]->entries().size())
+            return nil;
         return [NSString stringWithFormat:@"number of bind groups set(%u) is less than the pipeline uses(%zu)", setBindGroupsSize, numberOfBindGroupsInPipeline];
+    }
 
-    for (size_t bindGroupIndex = 0; bindGroupIndex < numberOfBindGroupsInPipeline; ++bindGroupIndex) {
+    for (size_t bindGroupIndex = vertexStageInBufferCount; bindGroupIndex < numberOfBindGroupsInPipeline; ++bindGroupIndex) {
         auto it = bindGroups.find(bindGroupIndex);
         if (it == bindGroups.end())
             return [NSString stringWithFormat:@"can not find bind group in pipeline for bindGroup index %zu", bindGroupIndex];

diff --git a/Source/WebGPU/WebGPU/Queue.h b/Source/WebGPU/WebGPU/Queue.h
@@ -98,7 +98,7 @@ class Queue : public WGPUQueueImpl, public ThreadSafeRefCounted<Queue> {
     id<MTLCommandQueue> m_commandQueue { nil };
     id<MTLCommandBuffer> m_commandBuffer { nil };
     id<MTLBlitCommandEncoder> m_blitCommandEncoder { nil };
-    WeakPtr<Device> m_device; // The only kind of queues that exist right now are default queues, which are owned by Devices.
+    ThreadSafeWeakPtr<Device> m_device; // The only kind of queues that exist right now are default queues, which are owned by Devices.
 
     uint64_t m_submittedCommandBufferCount { 0 };
     uint64_t m_completedCommandBufferCount { 0 };

diff --git a/Source/WebGPU/WebGPU/Queue.mm b/Source/WebGPU/WebGPU/Queue.mm
@@ -185,13 +185,19 @@
 
     ASSERT(commandBuffer.commandQueue == m_commandQueue);
     [commandBuffer addScheduledHandler:[protectedThis = Ref { *this }](id<MTLCommandBuffer>) {
+        auto device = protectedThis->m_device.get();
+        if (!device || !device->device())
+            return;
         protectedThis->scheduleWork(CompletionHandler<void(void)>([protectedThis = protectedThis.copyRef()]() {
             ++(protectedThis->m_scheduledCommandBufferCount);
             for (auto& callback : protectedThis->m_onSubmittedWorkScheduledCallbacks.take(protectedThis->m_scheduledCommandBufferCount))
                 callback();
         }, CompletionHandlerCallThread::AnyThread));
     }];
     [commandBuffer addCompletedHandler:[protectedThis = Ref { *this }](id<MTLCommandBuffer> commandBuffer) {
+        auto device = protectedThis->m_device.get();
+        if (!device || !device->device())
+            return;
         protectedThis->scheduleWork(CompletionHandler<void(void)>([commandBuffer, protectedThis = protectedThis.copyRef()]() {
             ++(protectedThis->m_completedCommandBufferCount);
             [protectedThis->m_pendingCommandBuffers removeObject:commandBuffer];
@@ -209,7 +215,7 @@
 
 void Queue::submit(Vector<std::reference_wrapper<CommandBuffer>>&& commands)
 {
-    auto* device = m_device.get();
+    auto device = m_device.get();
     if (!device)
         return;
 
@@ -274,7 +280,7 @@
 
 void Queue::writeBuffer(const Buffer& buffer, uint64_t bufferOffset, void* data, size_t size)
 {
-    auto* device = m_device.get();
+    auto device = m_device.get();
     if (!device)
         return;
 
@@ -315,7 +321,7 @@
 
 void Queue::writeBuffer(id<MTLBuffer> buffer, uint64_t bufferOffset, void* data, size_t size)
 {
-    auto* device = m_device.get();
+    auto device = m_device.get();
     if (!device)
         return;
 
@@ -379,23 +385,24 @@ static bool validateWriteTexture(const WGPUImageCopyTexture& destination, const
 
 const Device& Queue::device() const
 {
-    auto* device = m_device.get();
+    auto device = m_device.get();
     RELEASE_ASSERT(device);
     return *device;
 }
 
 void Queue::clearTexture(const WGPUImageCopyTexture& destination, NSUInteger slice)
 {
-    if (!m_device.get())
+    auto device = m_device.get();
+    if (!device)
         return;
 
     ensureBlitCommandEncoder();
-    CommandEncoder::clearTexture(destination, slice, m_device.get()->device(), m_blitCommandEncoder);
+    CommandEncoder::clearTexture(destination, slice, device->device(), m_blitCommandEncoder);
 }
 
 void Queue::writeTexture(const WGPUImageCopyTexture& destination, void* data, size_t dataSize, const WGPUTextureDataLayout& dataLayout, const WGPUExtent3D& size)
 {
-    auto* device = m_device.get();
+    auto device = m_device.get();
     if (destination.nextInChain || dataLayout.nextInChain || !device)
         return;
 
@@ -701,10 +708,11 @@ static bool validateWriteTexture(const WGPUImageCopyTexture& destination, const
 
 void Queue::scheduleWork(Instance::WorkItem&& workItem)
 {
-    if (!m_device.get())
+    auto device = m_device.get();
+    if (!device)
         return;
 
-    m_device.get()->instance().scheduleWork(WTFMove(workItem));
+    device->instance().scheduleWork(WTFMove(workItem));
 }
 
 } // namespace WebGPU

diff --git a/Source/WebGPU/WebGPU/RenderBundleEncoder.mm b/Source/WebGPU/WebGPU/RenderBundleEncoder.mm
@@ -263,7 +263,7 @@ - (instancetype)initWithICB:(id<MTLIndirectCommandBuffer>)icb pipelineState:(id<
     if (!icbCommand)
         return true;
 
-    if (NSString* error = m_pipeline->pipelineLayout().errorValidatingBindGroupCompatibility(m_bindGroups)) {
+    if (NSString* error = m_pipeline->pipelineLayout().errorValidatingBindGroupCompatibility(m_bindGroups, m_pipeline->vertexStageInBufferCount())) {
         makeInvalid(error);
         return false;
     }

diff --git a/Source/WebGPU/WebGPU/RenderPassEncoder.mm b/Source/WebGPU/WebGPU/RenderPassEncoder.mm
@@ -376,7 +376,7 @@ static void setViewportMinMaxDepthIntoBuffer(auto& fragmentDynamicOffsets, float
         return false;
     }
 
-    if (NSString* error = m_pipeline->pipelineLayout().errorValidatingBindGroupCompatibility(m_bindGroups)) {
+    if (NSString* error = m_pipeline->pipelineLayout().errorValidatingBindGroupCompatibility(m_bindGroups, m_pipeline->vertexStageInBufferCount())) {
         makeInvalid(error);
         return false;
     }

diff --git a/Source/WebGPU/WebGPU/RenderPipeline.h b/Source/WebGPU/WebGPU/RenderPipeline.h
@@ -92,6 +92,7 @@ class RenderPipeline : public WGPURenderPipelineImpl, public RefCounted<RenderPi
     const RequiredBufferIndicesContainer& requiredBufferIndices() const { return m_requiredBufferIndices; }
     WGPUPrimitiveTopology primitiveTopology() const;
     MTLIndexType stripIndexFormat() const;
+    size_t vertexStageInBufferCount() const;
 
 private:
     RenderPipeline(id<MTLRenderPipelineState>, MTLPrimitiveType, std::optional<MTLIndexType>, MTLWinding, MTLCullMode, MTLDepthClipMode, MTLDepthStencilDescriptor *, Ref<PipelineLayout>&&, float depthBias, float depthBiasSlopeScale, float depthBiasClamp, uint32_t sampleMask, MTLRenderPipelineDescriptor*, uint32_t colorAttachmentCount, const WGPURenderPipelineDescriptor&, RequiredBufferIndicesContainer&&, Device&);

diff --git a/Source/WebGPU/WebGPU/RenderPipeline.mm b/Source/WebGPU/WebGPU/RenderPipeline.mm
@@ -1512,6 +1512,11 @@ static uint32_t componentsForDataType(MTLDataType dataType)
     });
 }
 
+size_t RenderPipeline::vertexStageInBufferCount() const
+{
+    return m_descriptor.vertex.bufferCount;
+}
+
 RenderPipeline::RenderPipeline(id<MTLRenderPipelineState> renderPipelineState, MTLPrimitiveType primitiveType, std::optional<MTLIndexType> indexType, MTLWinding frontFace, MTLCullMode cullMode, MTLDepthClipMode clipMode, MTLDepthStencilDescriptor *depthStencilDescriptor, Ref<PipelineLayout>&& pipelineLayout, float depthBias, float depthBiasSlopeScale, float depthBiasClamp, uint32_t sampleMask, MTLRenderPipelineDescriptor* renderPipelineDescriptor, uint32_t colorAttachmentCount, const WGPURenderPipelineDescriptor& descriptor, RequiredBufferIndicesContainer&& requiredBufferIndices, Device& device)
     : m_renderPipelineState(renderPipelineState)
     , m_device(device)