From 3354bf0d91e941942f56930c48adddaa9e471560 Mon Sep 17 00:00:00 2001
From: Loko Kung <lokokung@google.com>
Date: Thu, 22 Sep 2022 23:45:36 +0000
Subject: [PATCH] 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>
---
 src/dawn/native/EncodingContext.cpp           |  7 +++--
 .../native/IndirectDrawValidationEncoder.cpp  |  6 ++++
 src/dawn/tests/end2end/DeviceLostTests.cpp    | 31 +++++++++++++++++++
 3 files changed, 42 insertions(+), 2 deletions(-)

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<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());