Moves DestroyApiObject call into ApiObjectBase::DeleteThis
- Moving the call into DeleteThis should make it so that derived classes don't need to explicitly implement a destructor that calls DestroyApiObject.
Bug: dawn:628
Change-Id: I145f42e7e4c144cc0d2d7c7f609744399d514fe1
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/66840
Commit-Queue: Loko Kung <lokokung@google.com>
Reviewed-by: Austin Eng <enga@chromium.org>
diff --git a/src/dawn_native/Device.cpp b/src/dawn_native/Device.cpp
index 0c948de..d5ae480 100644
--- a/src/dawn_native/Device.cpp
+++ b/src/dawn_native/Device.cpp
@@ -200,7 +200,11 @@
mCaches = std::make_unique<DeviceBase::Caches>();
}
- 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 d458334..2a1d495 100644
--- a/src/dawn_native/ObjectBase.cpp
+++ b/src/dawn_native/ObjectBase.cpp
@@ -70,15 +70,22 @@
return IsInList();
}
+ void ApiObjectBase::DeleteThis() {
+ DestroyApiObject();
+ RefCounted::DeleteThis();
+ }
+
void ApiObjectBase::TrackInDevice() {
ASSERT(GetDevice() != nullptr);
GetDevice()->TrackObject(this);
}
bool ApiObjectBase::DestroyApiObject() {
- const std::lock_guard<std::mutex> lock(*GetDevice()->GetObjectListMutex(GetType()));
- if (!RemoveFromList()) {
- return false;
+ {
+ const std::lock_guard<std::mutex> 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 6dd1824..d05a56e 100644
--- a/src/dawn_native/ObjectBase.h
+++ b/src/dawn_native/ObjectBase.h
@@ -71,6 +71,17 @@
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 02fb9f1..761b8f7 100644
--- a/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp
+++ b/src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp
@@ -131,10 +131,6 @@
device->GetSamplerStagingDescriptorAllocator(GetSamplerDescriptorCount());
}
- BindGroupLayout::~BindGroupLayout() {
- DestroyApiObject();
- }
-
ResultOrError<Ref<BindGroup>> BindGroupLayout::AllocateBindGroup(
Device* device,
const BindGroupDescriptor* descriptor) {
diff --git a/src/dawn_native/d3d12/BindGroupLayoutD3D12.h b/src/dawn_native/d3d12/BindGroupLayoutD3D12.h
index e55c3df..abf6702 100644
--- a/src/dawn_native/d3d12/BindGroupLayoutD3D12.h
+++ b/src/dawn_native/d3d12/BindGroupLayoutD3D12.h
@@ -64,7 +64,7 @@
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 bbbc959..1d2c2a9 100644
--- a/src/dawn_native/metal/BindGroupLayoutMTL.h
+++ b/src/dawn_native/metal/BindGroupLayoutMTL.h
@@ -36,7 +36,7 @@
BindGroupLayout(DeviceBase* device,
const BindGroupLayoutDescriptor* descriptor,
PipelineCompatibilityToken pipelineCompatibilityToken);
- ~BindGroupLayout() override;
+ ~BindGroupLayout() override = default;
SlabAllocator<BindGroup> mBindGroupAllocator;
};
diff --git a/src/dawn_native/metal/BindGroupLayoutMTL.mm b/src/dawn_native/metal/BindGroupLayoutMTL.mm
index a1c8255..5d748c1 100644
--- a/src/dawn_native/metal/BindGroupLayoutMTL.mm
+++ b/src/dawn_native/metal/BindGroupLayoutMTL.mm
@@ -33,10 +33,6 @@
mBindGroupAllocator(MakeFrontendBindGroupAllocator<BindGroup>(4096)) {
}
- BindGroupLayout::~BindGroupLayout() {
- DestroyApiObject();
- }
-
Ref<BindGroup> 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 4455226..e9b23eb 100644
--- a/src/dawn_native/null/DeviceNull.cpp
+++ b/src/dawn_native/null/DeviceNull.cpp
@@ -271,10 +271,6 @@
: 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 9e9200c..5192819 100644
--- a/src/dawn_native/null/DeviceNull.h
+++ b/src/dawn_native/null/DeviceNull.h
@@ -207,7 +207,7 @@
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 99cd5c23..d008b1d 100644
--- a/src/dawn_native/opengl/BindGroupLayoutGL.cpp
+++ b/src/dawn_native/opengl/BindGroupLayoutGL.cpp
@@ -25,10 +25,6 @@
mBindGroupAllocator(MakeFrontendBindGroupAllocator<BindGroup>(4096)) {
}
- BindGroupLayout::~BindGroupLayout() {
- DestroyApiObject();
- }
-
Ref<BindGroup> 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 5061b02..136bd0a 100644
--- a/src/dawn_native/opengl/BindGroupLayoutGL.h
+++ b/src/dawn_native/opengl/BindGroupLayoutGL.h
@@ -33,7 +33,7 @@
void DeallocateBindGroup(BindGroup* bindGroup);
private:
- ~BindGroupLayout() override;
+ ~BindGroupLayout() override = default;
SlabAllocator<BindGroup> mBindGroupAllocator;
};
diff --git a/src/dawn_native/vulkan/BindGroupLayoutVk.cpp b/src/dawn_native/vulkan/BindGroupLayoutVk.cpp
index eb52822..b464758 100644
--- a/src/dawn_native/vulkan/BindGroupLayoutVk.cpp
+++ b/src/dawn_native/vulkan/BindGroupLayoutVk.cpp
@@ -161,7 +161,6 @@
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 d197403..6e9c283 100644
--- a/src/tests/unittests/native/DestroyObjectTests.cpp
+++ b/src/tests/unittests/native/DestroyObjectTests.cpp
@@ -25,7 +25,7 @@
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 @@
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<BindGroupLayoutBase> 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 6f8dba5..dcd8692 100644
--- a/src/tests/unittests/native/mocks/BindGroupLayoutMock.h
+++ b/src/tests/unittests/native/mocks/BindGroupLayoutMock.h
@@ -26,9 +26,7 @@
public:
BindGroupLayoutMock(DeviceBase* device) : BindGroupLayoutBase(device) {
}
- ~BindGroupLayoutMock() override {
- DestroyApiObject();
- }
+ ~BindGroupLayoutMock() override = default;
MOCK_METHOD(void, DestroyApiObjectImpl, (), (override));
};
