diff --git a/src/dawn/native/EncodingContext.cpp b/src/dawn/native/EncodingContext.cpp index 9b7f36703d..3f2e796788 100644 --- a/src/dawn/native/EncodingContext.cpp +++ b/src/dawn/native/EncodingContext.cpp @@ -127,9 +127,12 @@ MaybeError EncodingContext::ExitRenderPass(const ApiObjectBase* passEncoder, // mPendingCommands contains only the commands from BeginRenderPassCmd to // EndRenderPassCmd, inclusive. Now we swap out this allocator with a fresh one to give // the validation encoder a chance to insert its commands first. + // Note: If encoding validation commands fails, no commands should be in mPendingCommands, + // so swap back the renderCommands to ensure that they are not leaked. CommandAllocator renderCommands = std::move(mPendingCommands); - DAWN_TRY(EncodeIndirectDrawValidationCommands(mDevice, commandEncoder, &usageTracker, - &indirectDrawMetadata)); + DAWN_TRY_WITH_CLEANUP(EncodeIndirectDrawValidationCommands( + mDevice, commandEncoder, &usageTracker, &indirectDrawMetadata), + { mPendingCommands = std::move(renderCommands); }); CommitCommands(std::move(mPendingCommands)); CommitCommands(std::move(renderCommands)); } diff --git a/src/dawn/native/IndirectDrawValidationEncoder.cpp b/src/dawn/native/IndirectDrawValidationEncoder.cpp index db940e798b..e8e4e1be6d 100644 --- a/src/dawn/native/IndirectDrawValidationEncoder.cpp +++ b/src/dawn/native/IndirectDrawValidationEncoder.cpp @@ -239,6 +239,12 @@ MaybeError EncodeIndirectDrawValidationCommands(DeviceBase* device, CommandEncoder* commandEncoder, RenderPassResourceUsageTracker* usageTracker, IndirectDrawMetadata* indirectDrawMetadata) { + // Since encoding validation commands may create new objects, verify that the device is alive. + // TODO(dawn:1199): This check is obsolete if device loss causes device.destroy(). + // - This function only happens within the context of a TryEncode which would catch the + // same issue if device loss implied device.destroy(). + DAWN_TRY(device->ValidateIsAlive()); + struct Batch { const IndirectDrawMetadata::IndirectValidationBatch* metadata; uint64_t numIndexBufferElements; diff --git a/src/dawn/tests/end2end/DeviceLostTests.cpp b/src/dawn/tests/end2end/DeviceLostTests.cpp index d9aa3d0541..4c2b01ed85 100644 --- a/src/dawn/tests/end2end/DeviceLostTests.cpp +++ b/src/dawn/tests/end2end/DeviceLostTests.cpp @@ -475,6 +475,37 @@ TEST_P(DeviceLostTest, FreeBindGroupAfterDeviceLossWithPendingCommands) { bg = nullptr; } +// This is a regression test for crbug.com/1365011 where ending a render pass with an indirect draw +// in it after the device is lost would cause render commands to be leaked. +TEST_P(DeviceLostTest, DeviceLostInRenderPassWithDrawIndirect) { + utils::BasicRenderPass renderPass = utils::CreateBasicRenderPass(device, 4u, 4u); + utils::ComboRenderPipelineDescriptor desc; + desc.vertex.module = utils::CreateShaderModule(device, R"( + @vertex fn main(@builtin(vertex_index) i : u32) -> @builtin(position) vec4 { + var pos = array, 3>( + vec2(-1.0, -1.0), + vec2(3.0, -1.0), + vec2(-1.0, 3.0)); + return vec4(pos[i], 0.0, 1.0); + } + )"); + desc.cFragment.module = utils::CreateShaderModule(device, R"( + @fragment fn main() -> @location(0) vec4 { + return vec4(0.0, 1.0, 0.0, 1.0); + } + )"); + desc.cTargets[0].format = renderPass.colorFormat; + wgpu::Buffer indirectBuffer = + utils::CreateBufferFromData(device, wgpu::BufferUsage::Indirect, {3, 1, 0, 0}); + wgpu::RenderPipeline pipeline = device.CreateRenderPipeline(&desc); + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + wgpu::RenderPassEncoder pass = encoder.BeginRenderPass(&renderPass.renderPassInfo); + pass.SetPipeline(pipeline); + pass.DrawIndirect(indirectBuffer, 0); + LoseDeviceForTesting(); + pass.End(); +} + // Attempting to set an object label after device loss should not cause an error. TEST_P(DeviceLostTest, SetLabelAfterDeviceLoss) { DAWN_TEST_UNSUPPORTED_IF(UsesWire());