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>
diff --git a/src/dawn/native/Device.cpp b/src/dawn/native/Device.cpp
index 5f92015..b8048c3 100644
--- a/src/dawn/native/Device.cpp
+++ b/src/dawn/native/Device.cpp
@@ -287,10 +287,10 @@
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
- // a ref to A, then B depends on A. We therefore try to destroy B before destroying A. Note
- // that this only considers the immediate frontend dependencies, while backend objects could
- // add complications and extra dependencies.
+ // objects safely. We define dependent here such that if B has a ref to A, then B depends on
+ // A. We therefore try to destroy B before destroying A. Note that this only considers the
+ // immediate frontend dependencies, while backend objects could add complications and extra
+ // 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 @@
const std::lock_guard<std::mutex> lock(objList.mutex);
objList.objects.MoveInto(&objects);
}
- for (LinkNode<ApiObjectBase>* node : objects) {
- node->value()->Destroy();
+ while (!objects.empty()) {
+ // The destroy call should also remove the object from the list.
+ objects.head()->value()->Destroy();
}
}
diff --git a/src/dawn/tests/unittests/native/DestroyObjectTests.cpp b/src/dawn/tests/unittests/native/DestroyObjectTests.cpp
index e70682d0..47481c5 100644
--- a/src/dawn/tests/unittests/native/DestroyObjectTests.cpp
+++ b/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 @@
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::
