Add cleanup when encoding indirect draw validations.
- Cleanup is necessary because otherwise encoded render commands may be
leaked if the validation encoding fails. (The leaked render commands
can then trigger an assert in ~Device::Cache because the commands can
hold a ref to an AttachmentState that was not destroyed, and hence
still be in the device cache.
- Added explicit check in EncoderIndirectDrawValidationCommands for
device 'alive-ness' since it may create new objects later and hit the
same error later on anyways.
- Added regression test.
Fixed: chromium:1365011
Change-Id: I342479a4227fc43d82ea35f662d049e6db2b1740
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/103340
Reviewed-by: Austin Eng <enga@chromium.org>
Commit-Queue: Loko Kung <lokokung@google.com>
Kokoro: Kokoro <noreply+kokoro@google.com>
diff --git a/src/dawn/native/EncodingContext.cpp b/src/dawn/native/EncodingContext.cpp
index 9b7f367..3f2e796 100644
--- a/src/dawn/native/EncodingContext.cpp
+++ b/src/dawn/native/EncodingContext.cpp
@@ -127,9 +127,12 @@
// 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 db940e7..e8e4e1b 100644
--- a/src/dawn/native/IndirectDrawValidationEncoder.cpp
+++ b/src/dawn/native/IndirectDrawValidationEncoder.cpp
@@ -239,6 +239,12 @@
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 d9aa3d0..4c2b01ed85 100644
--- a/src/dawn/tests/end2end/DeviceLostTests.cpp
+++ b/src/dawn/tests/end2end/DeviceLostTests.cpp
@@ -475,6 +475,37 @@
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<f32> {
+ var pos = array<vec2<f32>, 3>(
+ vec2<f32>(-1.0, -1.0),
+ vec2<f32>(3.0, -1.0),
+ vec2<f32>(-1.0, 3.0));
+ return vec4<f32>(pos[i], 0.0, 1.0);
+ }
+ )");
+ desc.cFragment.module = utils::CreateShaderModule(device, R"(
+ @fragment fn main() -> @location(0) vec4<f32> {
+ return vec4<f32>(0.0, 1.0, 0.0, 1.0);
+ }
+ )");
+ desc.cTargets[0].format = renderPass.colorFormat;
+ wgpu::Buffer indirectBuffer =
+ utils::CreateBufferFromData<uint32_t>(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());
