diff --git a/src/dawn_native/Device.cpp b/src/dawn_native/Device.cpp index 0c948ded9a..d5ae480426 100644 --- a/src/dawn_native/Device.cpp +++ b/src/dawn_native/Device.cpp @@ -200,7 +200,11 @@ namespace dawn_native { mCaches = std::make_unique(); } - DeviceBase::~DeviceBase() = default; + DeviceBase::~DeviceBase() { + // We need to explicitly release the Queue before we complete the destructor so that the + // Queue does not get destroyed after the Device. + mQueue = nullptr; + } MaybeError DeviceBase::Initialize(QueueBase* defaultQueue) { mQueue = AcquireRef(defaultQueue); diff --git a/src/dawn_native/ObjectBase.cpp b/src/dawn_native/ObjectBase.cpp index d458334b51..2a1d495a8d 100644 --- a/src/dawn_native/ObjectBase.cpp +++ b/src/dawn_native/ObjectBase.cpp @@ -70,15 +70,22 @@ namespace dawn_native { return IsInList(); } + void ApiObjectBase::DeleteThis() { + DestroyApiObject(); + RefCounted::DeleteThis(); + } + void ApiObjectBase::TrackInDevice() { ASSERT(GetDevice() != nullptr); GetDevice()->TrackObject(this); } bool ApiObjectBase::DestroyApiObject() { - const std::lock_guard lock(*GetDevice()->GetObjectListMutex(GetType())); - if (!RemoveFromList()) { - return false; + { + const std::lock_guard lock(*GetDevice()->GetObjectListMutex(GetType())); + if (!RemoveFromList()) { + return false; + } } DestroyApiObjectImpl(); return true; diff --git a/src/dawn_native/ObjectBase.h b/src/dawn_native/ObjectBase.h index 6dd18245a8..d05a56e0e6 100644 --- a/src/dawn_native/ObjectBase.h +++ b/src/dawn_native/ObjectBase.h @@ -71,6 +71,17 @@ namespace dawn_native { void APISetLabel(const char* label); protected: + // Overriding of the RefCounted's DeleteThis function ensures that instances of objects + // always call their derived class implementation of DestroyApiObject prior to the derived + // class being destroyed. This guarantees that when ApiObjects' reference counts drop to 0, + // then the underlying backend's Destroy calls are executed. We cannot naively put the call + // to DestroyApiObject in the destructor of this class because it calls DestroyApiObjectImpl + // which is a virtual function often implemented in the Derived class which would already + // have been destroyed by the time ApiObject's destructor is called by C++'s destruction + // order. Note that some classes like BindGroup may override the DeleteThis function again, + // and they should ensure that their overriding versions call this underlying version + // somewhere. + void DeleteThis() override; void TrackInDevice(); virtual void DestroyApiObjectImpl(); diff --git a/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp b/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp index 02fb9f17d2..761b8f74a7 100644 --- a/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp +++ b/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp @@ -131,10 +131,6 @@ namespace dawn_native { namespace d3d12 { device->GetSamplerStagingDescriptorAllocator(GetSamplerDescriptorCount()); } - BindGroupLayout::~BindGroupLayout() { - DestroyApiObject(); - } - ResultOrError> BindGroupLayout::AllocateBindGroup( Device* device, const BindGroupDescriptor* descriptor) { diff --git a/src/dawn_native/d3d12/BindGroupLayoutD3D12.h b/src/dawn_native/d3d12/BindGroupLayoutD3D12.h index e55c3df28c..abf67021a0 100644 --- a/src/dawn_native/d3d12/BindGroupLayoutD3D12.h +++ b/src/dawn_native/d3d12/BindGroupLayoutD3D12.h @@ -64,7 +64,7 @@ namespace dawn_native { namespace d3d12 { BindGroupLayout(Device* device, const BindGroupLayoutDescriptor* descriptor, PipelineCompatibilityToken pipelineCompatibilityToken); - ~BindGroupLayout() override; + ~BindGroupLayout() override = default; // Contains the offset into the descriptor heap for the given resource view. Samplers and // non-samplers are stored in separate descriptor heaps, so the offsets should be unique diff --git a/src/dawn_native/metal/BindGroupLayoutMTL.h b/src/dawn_native/metal/BindGroupLayoutMTL.h index bbbc959b58..1d2c2a9334 100644 --- a/src/dawn_native/metal/BindGroupLayoutMTL.h +++ b/src/dawn_native/metal/BindGroupLayoutMTL.h @@ -36,7 +36,7 @@ namespace dawn_native { namespace metal { BindGroupLayout(DeviceBase* device, const BindGroupLayoutDescriptor* descriptor, PipelineCompatibilityToken pipelineCompatibilityToken); - ~BindGroupLayout() override; + ~BindGroupLayout() override = default; SlabAllocator mBindGroupAllocator; }; diff --git a/src/dawn_native/metal/BindGroupLayoutMTL.mm b/src/dawn_native/metal/BindGroupLayoutMTL.mm index a1c8255c39..5d748c1f78 100644 --- a/src/dawn_native/metal/BindGroupLayoutMTL.mm +++ b/src/dawn_native/metal/BindGroupLayoutMTL.mm @@ -33,10 +33,6 @@ namespace dawn_native { namespace metal { mBindGroupAllocator(MakeFrontendBindGroupAllocator(4096)) { } - BindGroupLayout::~BindGroupLayout() { - DestroyApiObject(); - } - Ref BindGroupLayout::AllocateBindGroup(Device* device, const BindGroupDescriptor* descriptor) { return AcquireRef(mBindGroupAllocator.Allocate(device, descriptor)); diff --git a/src/dawn_native/null/DeviceNull.cpp b/src/dawn_native/null/DeviceNull.cpp index 44552267f7..e9b23eba39 100644 --- a/src/dawn_native/null/DeviceNull.cpp +++ b/src/dawn_native/null/DeviceNull.cpp @@ -271,10 +271,6 @@ namespace dawn_native { namespace null { : BindGroupLayoutBase(device, descriptor, pipelineCompatibilityToken) { } - BindGroupLayout::~BindGroupLayout() { - DestroyApiObject(); - } - // Buffer Buffer::Buffer(Device* device, const BufferDescriptor* descriptor) diff --git a/src/dawn_native/null/DeviceNull.h b/src/dawn_native/null/DeviceNull.h index 9e9200c280..5192819a78 100644 --- a/src/dawn_native/null/DeviceNull.h +++ b/src/dawn_native/null/DeviceNull.h @@ -207,7 +207,7 @@ namespace dawn_native { namespace null { PipelineCompatibilityToken pipelineCompatibilityToken); private: - ~BindGroupLayout() override; + ~BindGroupLayout() override = default; }; class Buffer final : public BufferBase { diff --git a/src/dawn_native/opengl/BindGroupLayoutGL.cpp b/src/dawn_native/opengl/BindGroupLayoutGL.cpp index 99cd5c2345..d008b1d48a 100644 --- a/src/dawn_native/opengl/BindGroupLayoutGL.cpp +++ b/src/dawn_native/opengl/BindGroupLayoutGL.cpp @@ -25,10 +25,6 @@ namespace dawn_native { namespace opengl { mBindGroupAllocator(MakeFrontendBindGroupAllocator(4096)) { } - BindGroupLayout::~BindGroupLayout() { - DestroyApiObject(); - } - Ref BindGroupLayout::AllocateBindGroup(Device* device, const BindGroupDescriptor* descriptor) { return AcquireRef(mBindGroupAllocator.Allocate(device, descriptor)); diff --git a/src/dawn_native/opengl/BindGroupLayoutGL.h b/src/dawn_native/opengl/BindGroupLayoutGL.h index 5061b02012..136bd0a7e5 100644 --- a/src/dawn_native/opengl/BindGroupLayoutGL.h +++ b/src/dawn_native/opengl/BindGroupLayoutGL.h @@ -33,7 +33,7 @@ namespace dawn_native { namespace opengl { void DeallocateBindGroup(BindGroup* bindGroup); private: - ~BindGroupLayout() override; + ~BindGroupLayout() override = default; SlabAllocator mBindGroupAllocator; }; diff --git a/src/dawn_native/vulkan/BindGroupLayoutVk.cpp b/src/dawn_native/vulkan/BindGroupLayoutVk.cpp index eb5282294c..b4647582e9 100644 --- a/src/dawn_native/vulkan/BindGroupLayoutVk.cpp +++ b/src/dawn_native/vulkan/BindGroupLayoutVk.cpp @@ -161,7 +161,6 @@ namespace dawn_native { namespace vulkan { device->fn.DestroyDescriptorSetLayout(device->GetVkDevice(), mHandle, nullptr); mHandle = VK_NULL_HANDLE; } - DestroyApiObject(); } VkDescriptorSetLayout BindGroupLayout::GetHandle() const { diff --git a/src/tests/unittests/native/DestroyObjectTests.cpp b/src/tests/unittests/native/DestroyObjectTests.cpp index d1974035ec..6e9c2835a3 100644 --- a/src/tests/unittests/native/DestroyObjectTests.cpp +++ b/src/tests/unittests/native/DestroyObjectTests.cpp @@ -25,7 +25,7 @@ namespace dawn_native { namespace { using ::testing::InSequence; using ::testing::Return; - TEST(DestroyObjectTests, BindGroupLayout) { + TEST(DestroyObjectTests, BindGroupLayoutExplicit) { // Skipping validation on descriptors as coverage for validation is already present. DeviceMock device; device.SetToggle(Toggle::SkipValidation, true); @@ -46,6 +46,28 @@ namespace dawn_native { namespace { EXPECT_FALSE(bindGroupLayout->IsAlive()); } + // If the reference count on API objects reach 0, they should delete themselves. Note that GTest + // will also complain if there is a memory leak. + TEST(DestroyObjectTests, BindGroupLayoutImplicit) { + // Skipping validation on descriptors as coverage for validation is already present. + DeviceMock device; + device.SetToggle(Toggle::SkipValidation, true); + + BindGroupLayoutMock* bindGroupLayoutMock = new BindGroupLayoutMock(&device); + EXPECT_CALL(*bindGroupLayoutMock, DestroyApiObjectImpl).Times(1); + + { + BindGroupLayoutDescriptor desc = {}; + Ref bindGroupLayout; + EXPECT_CALL(device, CreateBindGroupLayoutImpl) + .WillOnce(Return(ByMove(AcquireRef(bindGroupLayoutMock)))); + DAWN_ASSERT_AND_ASSIGN(bindGroupLayout, device.CreateBindGroupLayout(&desc)); + + EXPECT_TRUE(bindGroupLayout->IsAlive()); + EXPECT_TRUE(bindGroupLayout->IsCachedReference()); + } + } + // Destroying the objects on the device should result in all created objects being destroyed in // order. TEST(DestroyObjectTests, DestroyObjects) { diff --git a/src/tests/unittests/native/mocks/BindGroupLayoutMock.h b/src/tests/unittests/native/mocks/BindGroupLayoutMock.h index 6f8dba53b2..dcd8692d10 100644 --- a/src/tests/unittests/native/mocks/BindGroupLayoutMock.h +++ b/src/tests/unittests/native/mocks/BindGroupLayoutMock.h @@ -26,9 +26,7 @@ namespace dawn_native { public: BindGroupLayoutMock(DeviceBase* device) : BindGroupLayoutBase(device) { } - ~BindGroupLayoutMock() override { - DestroyApiObject(); - } + ~BindGroupLayoutMock() override = default; MOCK_METHOD(void, DestroyApiObjectImpl, (), (override)); };