diff --git a/src/dawn/native/Device.cpp b/src/dawn/native/Device.cpp index 5f920150ee..b8048c350e 100644 --- a/src/dawn/native/Device.cpp +++ b/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 - // 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 @@ namespace dawn::native { const std::lock_guard lock(objList.mutex); objList.objects.MoveInto(&objects); } - for (LinkNode* 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 e70682d040..47481c55cf 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 @@ 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 { + return vec4(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::