Fix freed memory access due to DestroyObjects.

- Use a while loop to pop the list instead of iterating it and deleting at the same time.
- Adds regression tests to verify that the fix works.

Bug: chromium:1318792
Change-Id: I84fb494c64b07d3e1bd2b5b3af7cb9f82eee28b0
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/88043
Reviewed-by: Austin Eng <enga@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Commit-Queue: Loko Kung <lokokung@google.com>

src/dawn/native/Device.cpp

@@ -287,10 +287,10 @@ namespace dawn::native {
 
     void DeviceBase::DestroyObjects() {
         // List of object types in reverse "dependency" order so we can iterate and delete the
-        // objects safely starting at leaf objects. We define dependent here such that if B has
+        // objects safely. We define dependent here such that if B has a ref to A, then B depends on
-        // a ref to A, then B depends on A. We therefore try to destroy B before destroying A. Note
+        // A. We therefore try to destroy B before destroying A. Note that this only considers the
-        // that this only considers the immediate frontend dependencies, while backend objects could
+        // immediate frontend dependencies, while backend objects could add complications and extra
-        // add complications and extra dependencies.
+        // dependencies.
         //
         // Note that AttachmentState is not an ApiObject so it cannot be eagerly destroyed. However,
         // since AttachmentStates are cached by the device, objects that hold references to
@@ -330,8 +330,9 @@ namespace dawn::native {
             const std::lock_guard<std::mutex> lock(objList.mutex);
             objList.objects.MoveInto(&objects);
         }
-        for (LinkNode<ApiObjectBase>* node : objects) {
+        while (!objects.empty()) {
-            node->value()->Destroy();
+            // The destroy call should also remove the object from the list.
+            objects.head()->value()->Destroy();
         }
     }
 

src/dawn/tests/unittests/native/DestroyObjectTests.cpp

@@ -17,6 +17,7 @@
 #include "dawn/native/Toggles.h"
 #include "dawn/tests/DawnNativeTest.h"
 #include "dawn/utils/ComboRenderPipelineDescriptor.h"
+#include "dawn/utils/WGPUHelpers.h"
 #include "mocks/BindGroupLayoutMock.h"
 #include "mocks/BindGroupMock.h"
 #include "mocks/BufferMock.h"
@@ -763,6 +764,64 @@ namespace dawn::native { namespace {
         EXPECT_FALSE(textureView->IsAlive());
     }
 
+    static constexpr std::string_view kComputeShader = R"(
+        @stage(compute) @workgroup_size(1) fn main() {}
+    )";
+
+    static constexpr std::string_view kVertexShader = R"(
+        @stage(vertex) fn main() -> @builtin(position) vec4<f32> {
+            return vec4<f32>(0.0, 0.0, 0.0, 0.0);
+        }
+    )";
+
+    static constexpr std::string_view kFragmentShader = R"(
+        @stage(fragment) fn main() {}
+    )";
+
+    class DestroyObjectRegressionTests : public DawnNativeTest {};
+
+    // LastRefInCommand* tests are regression test(s) for https://crbug.com/chromium/1318792. The
+    // regression tests here are not exhuastive. In order to have an exhuastive test case for this
+    // class of failures, we should test every possible command with the commands holding the last
+    // references (or as last as possible) of their needed objects. For now, including simple cases
+    // including a stripped-down case from the original bug.
+
+    // Tests that when a RenderPipeline's last reference is held in a command in an unfinished
+    // CommandEncoder, that destroying the device still works as expected (and does not cause
+    // double-free).
+    TEST_F(DestroyObjectRegressionTests, LastRefInCommandRenderPipeline) {
+        utils::BasicRenderPass pass = utils::CreateBasicRenderPass(device, 1, 1);
+
+        utils::ComboRenderPassDescriptor passDesc{};
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::RenderPassEncoder renderEncoder = encoder.BeginRenderPass(&pass.renderPassInfo);
+
+        utils::ComboRenderPipelineDescriptor pipelineDesc;
+        pipelineDesc.cTargets[0].writeMask = wgpu::ColorWriteMask::None;
+        pipelineDesc.vertex.module = utils::CreateShaderModule(device, kVertexShader.data());
+        pipelineDesc.vertex.entryPoint = "main";
+        pipelineDesc.cFragment.module = utils::CreateShaderModule(device, kFragmentShader.data());
+        pipelineDesc.cFragment.entryPoint = "main";
+        renderEncoder.SetPipeline(device.CreateRenderPipeline(&pipelineDesc));
+
+        device.Destroy();
+    }
+
+    // Tests that when a ComputePipelines's last reference is held in a command in an unfinished
+    // CommandEncoder, that destroying the device still works as expected (and does not cause
+    // double-free).
+    TEST_F(DestroyObjectRegressionTests, LastRefInCommandComputePipeline) {
+        wgpu::CommandEncoder encoder = device.CreateCommandEncoder();
+        wgpu::ComputePassEncoder computeEncoder = encoder.BeginComputePass();
+
+        wgpu::ComputePipelineDescriptor pipelineDesc;
+        pipelineDesc.compute.module = utils::CreateShaderModule(device, kComputeShader.data());
+        pipelineDesc.compute.entryPoint = "main";
+        computeEncoder.SetPipeline(device.CreateComputePipeline(&pipelineDesc));
+
+        device.Destroy();
+    }
+
     // TODO(https://crbug.com/dawn/1381) Remove when namespaces are not indented.
     // NOLINTNEXTLINE(readability/namespace)
 }}  // namespace dawn::native::