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>

src/dawn_native/Device.cpp

@@ -200,7 +200,11 @@ namespace dawn_native {
         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);

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<std::mutex> lock(*GetDevice()->GetObjectListMutex(GetType()));
+        {
-        if (!RemoveFromList()) {
+            const std::lock_guard<std::mutex> lock(*GetDevice()->GetObjectListMutex(GetType()));
-            return false;
+            if (!RemoveFromList()) {
+                return false;
+            }
         }
         DestroyApiObjectImpl();
         return true;

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();
 

src/dawn_native/d3d12/BindGroupLayoutD3D12.cpp

@@ -131,10 +131,6 @@ namespace dawn_native { namespace d3d12 {
             device->GetSamplerStagingDescriptorAllocator(GetSamplerDescriptorCount());
     }
 
-    BindGroupLayout::~BindGroupLayout() {
-        DestroyApiObject();
-    }
-
     ResultOrError<Ref<BindGroup>> BindGroupLayout::AllocateBindGroup(
         Device* device,
         const BindGroupDescriptor* descriptor) {

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

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<BindGroup> mBindGroupAllocator;
     };

src/dawn_native/metal/BindGroupLayoutMTL.mm

@@ -33,10 +33,6 @@ namespace dawn_native { namespace metal {
           mBindGroupAllocator(MakeFrontendBindGroupAllocator<BindGroup>(4096)) {
     }
 
-    BindGroupLayout::~BindGroupLayout() {
-        DestroyApiObject();
-    }
-
     Ref<BindGroup> BindGroupLayout::AllocateBindGroup(Device* device,
                                                       const BindGroupDescriptor* descriptor) {
         return AcquireRef(mBindGroupAllocator.Allocate(device, descriptor));

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)

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 {

src/dawn_native/opengl/BindGroupLayoutGL.cpp

@@ -25,10 +25,6 @@ namespace dawn_native { namespace opengl {
           mBindGroupAllocator(MakeFrontendBindGroupAllocator<BindGroup>(4096)) {
     }
 
-    BindGroupLayout::~BindGroupLayout() {
-        DestroyApiObject();
-    }
-
     Ref<BindGroup> BindGroupLayout::AllocateBindGroup(Device* device,
                                                       const BindGroupDescriptor* descriptor) {
         return AcquireRef(mBindGroupAllocator.Allocate(device, descriptor));

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<BindGroup> mBindGroupAllocator;
     };
 

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 {

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<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) {

src/tests/unittests/native/mocks/BindGroupLayoutMock.h

@@ -26,9 +26,7 @@ namespace dawn_native {
       public:
         BindGroupLayoutMock(DeviceBase* device) : BindGroupLayoutBase(device) {
         }
-        ~BindGroupLayoutMock() override {
+        ~BindGroupLayoutMock() override = default;
-            DestroyApiObject();
-        }
 
         MOCK_METHOD(void, DestroyApiObjectImpl, (), (override));
     };