Adds/refactors destroy handling for Buffer and QuerySet.

The changes should pass through the destroy changes such that when the device is destroyed, the respective destroy functionality currently existing in the backends should be called.

For buffers, destroy no longer causes validation errors since even error buffers may need to be destroyed in the case of mappedAtCreation.

Bug: dawn:628, dawn:1002
Change-Id: I42a475af5d67cc60f86d95ac53c2b377a9fd2e82
Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/65863
Reviewed-by: Austin Eng <enga@chromium.org>
Reviewed-by: Corentin Wallez <cwallez@chromium.org>
Commit-Queue: Loko Kung <lokokung@google.com>

src/dawn_native/Buffer.cpp

@@ -64,13 +64,12 @@ namespace dawn_native {
                         mFakeMappedData =
                             std::unique_ptr<uint8_t[]>(AllocNoThrow<uint8_t>(descriptor->size));
                     }
+                    // Since error buffers in this case may allocate memory, we need to track them
+                    // for destruction on the device.
+                    TrackInDevice();
                 }
             }
 
-            void ClearMappedData() {
-                mFakeMappedData.reset();
-            }
-
           private:
             bool IsCPUWritableAtCreation() const override {
                 UNREACHABLE();
@@ -83,14 +82,17 @@ namespace dawn_native {
             MaybeError MapAsyncImpl(wgpu::MapMode mode, size_t offset, size_t size) override {
                 UNREACHABLE();
             }
+
             void* GetMappedPointerImpl() override {
                 return mFakeMappedData.get();
             }
+
             void UnmapImpl() override {
-                UNREACHABLE();
+                mFakeMappedData.reset();
             }
-            void DestroyImpl() override {
+
-                UNREACHABLE();
+            void DestroyApiObjectImpl() override {
+                mFakeMappedData.reset();
             }
 
             std::unique_ptr<uint8_t[]> mFakeMappedData;
@@ -153,6 +155,8 @@ namespace dawn_native {
         if ((mUsage & wgpu::BufferUsage::Indirect) && device->IsValidationEnabled()) {
             mUsage |= kInternalStorageBuffer;
         }
+
+        TrackInDevice();
     }
 
     BufferBase::BufferBase(DeviceBase* device,
@@ -166,11 +170,34 @@ namespace dawn_native {
         }
     }
 
+    BufferBase::BufferBase(DeviceBase* device, BufferState state)
+        : ApiObjectBase(device, kLabelNotImplemented), mState(state) {
+        TrackInDevice();
+    }
+
     BufferBase::~BufferBase() {
-        if (mState == BufferState::Mapped) {
+        ASSERT(mState == BufferState::Unmapped || mState == BufferState::Destroyed);
-            ASSERT(!IsError());
+    }
-            CallMapCallback(mLastMapID, WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
+
+    bool BufferBase::DestroyApiObject() {
+        bool marked = MarkDestroyed();
+        if (!marked) {
+            return false;
         }
+
+        if (mState == BufferState::Mapped) {
+            UnmapInternal(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
+        } else if (mState == BufferState::MappedAtCreation) {
+            if (mStagingBuffer != nullptr) {
+                mStagingBuffer.reset();
+            } else if (mSize != 0) {
+                UnmapInternal(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
+            }
+        }
+
+        DestroyApiObjectImpl();
+        mState = BufferState::Destroyed;
+        return true;
     }
 
     // static
@@ -366,29 +393,7 @@ namespace dawn_native {
     }
 
     void BufferBase::APIDestroy() {
-        if (IsError()) {
+        DestroyApiObject();
-            // It is an error to call Destroy() on an ErrorBuffer, but we still need to reclaim the
-            // fake mapped staging data.
-            static_cast<ErrorBuffer*>(this)->ClearMappedData();
-            mState = BufferState::Destroyed;
-        }
-        if (GetDevice()->ConsumedError(ValidateDestroy(), "calling %s.Destroy()", this)) {
-            return;
-        }
-        ASSERT(!IsError());
-
-        if (mState == BufferState::Mapped) {
-            UnmapInternal(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
-        } else if (mState == BufferState::MappedAtCreation) {
-            if (mStagingBuffer != nullptr) {
-                mStagingBuffer.reset();
-            } else if (mSize != 0) {
-                ASSERT(IsCPUWritableAtCreation());
-                UnmapInternal(WGPUBufferMapAsyncStatus_DestroyedBeforeCallback);
-            }
-        }
-
-        DestroyInternal();
     }
 
     MaybeError BufferBase::CopyFromStagingBuffer() {
@@ -409,6 +414,9 @@ namespace dawn_native {
     }
 
     void BufferBase::APIUnmap() {
+        if (GetDevice()->ConsumedError(ValidateUnmap(), "calling %s.Unmap()", this)) {
+            return;
+        }
         Unmap();
     }
 
@@ -417,17 +425,6 @@ namespace dawn_native {
     }
 
     void BufferBase::UnmapInternal(WGPUBufferMapAsyncStatus callbackStatus) {
-        if (IsError()) {
-            // It is an error to call Unmap() on an ErrorBuffer, but we still need to reclaim the
-            // fake mapped staging data.
-            static_cast<ErrorBuffer*>(this)->ClearMappedData();
-            mState = BufferState::Unmapped;
-        }
-        if (GetDevice()->ConsumedError(ValidateUnmap(), "calling %s.Unmap()", this)) {
-            return;
-        }
-        ASSERT(!IsError());
-
         if (mState == BufferState::Mapped) {
             // A map request can only be called once, so this will fire only if the request wasn't
             // completed before the Unmap.
@@ -438,12 +435,10 @@ namespace dawn_native {
 
             mMapCallback = nullptr;
             mMapUserdata = 0;
-
         } else if (mState == BufferState::MappedAtCreation) {
             if (mStagingBuffer != nullptr) {
                 GetDevice()->ConsumedError(CopyFromStagingBuffer());
             } else if (mSize != 0) {
-                ASSERT(IsCPUWritableAtCreation());
                 UnmapImpl();
             }
         }
@@ -543,7 +538,6 @@ namespace dawn_native {
 
     MaybeError BufferBase::ValidateUnmap() const {
         DAWN_TRY(GetDevice()->ValidateIsAlive());
-        DAWN_TRY(GetDevice()->ValidateObject(this));
 
         switch (mState) {
             case BufferState::Mapped:
@@ -559,18 +553,6 @@ namespace dawn_native {
         UNREACHABLE();
     }
 
-    MaybeError BufferBase::ValidateDestroy() const {
-        DAWN_TRY(GetDevice()->ValidateObject(this));
-        return {};
-    }
-
-    void BufferBase::DestroyInternal() {
-        if (mState != BufferState::Destroyed) {
-            DestroyImpl();
-        }
-        mState = BufferState::Destroyed;
-    }
-
     void BufferBase::OnMapRequestCompleted(MapRequestID mapID, WGPUBufferMapAsyncStatus status) {
         CallMapCallback(mapID, status);
     }

src/dawn_native/Buffer.h

@@ -41,18 +41,18 @@ namespace dawn_native {
         wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite;
 
     class BufferBase : public ApiObjectBase {
+      public:
         enum class BufferState {
             Unmapped,
             Mapped,
             MappedAtCreation,
             Destroyed,
         };
-
-      public:
         BufferBase(DeviceBase* device, const BufferDescriptor* descriptor);
 
         static BufferBase* MakeError(DeviceBase* device, const BufferDescriptor* descriptor);
 
+        bool DestroyApiObject() override;
         ObjectType GetType() const override;
 
         uint64_t GetSize() const;
@@ -86,9 +86,11 @@ namespace dawn_native {
         BufferBase(DeviceBase* device,
                    const BufferDescriptor* descriptor,
                    ObjectBase::ErrorTag tag);
-        ~BufferBase() override;
 
-        void DestroyInternal();
+        // Constructor used only for mocking and testing.
+        BufferBase(DeviceBase* device, BufferState state);
+
+        ~BufferBase() override;
 
         MaybeError MapAtCreationInternal();
 
@@ -98,7 +100,6 @@ namespace dawn_native {
         virtual MaybeError MapAtCreationImpl() = 0;
         virtual MaybeError MapAsyncImpl(wgpu::MapMode mode, size_t offset, size_t size) = 0;
         virtual void UnmapImpl() = 0;
-        virtual void DestroyImpl() = 0;
         virtual void* GetMappedPointerImpl() = 0;
 
         virtual bool IsCPUWritableAtCreation() const = 0;
@@ -110,7 +111,6 @@ namespace dawn_native {
                                     size_t size,
                                     WGPUBufferMapAsyncStatus* status) const;
         MaybeError ValidateUnmap() const;
-        MaybeError ValidateDestroy() const;
         bool CanGetMappedRange(bool writable, size_t offset, size_t size) const;
         void UnmapInternal(WGPUBufferMapAsyncStatus callbackStatus);
 

src/dawn_native/Device.cpp

@@ -268,7 +268,7 @@ namespace dawn_native {
         // TODO(dawn/628) Add types into the array as they are implemented.
 
         // clang-format off
-        static constexpr std::array<ObjectType, 8> kObjectTypeDependencyOrder = {
+        static constexpr std::array<ObjectType, 10> kObjectTypeDependencyOrder = {
             ObjectType::RenderPipeline,
             ObjectType::ComputePipeline,
             ObjectType::PipelineLayout,
@@ -276,7 +276,9 @@ namespace dawn_native {
             ObjectType::BindGroup,
             ObjectType::BindGroupLayout,
             ObjectType::ShaderModule,
+            ObjectType::QuerySet,
             ObjectType::Sampler,
+            ObjectType::Buffer,
         };
         // clang-format on
 

src/dawn_native/ObjectBase.cpp

@@ -80,15 +80,17 @@ namespace dawn_native {
         GetDevice()->TrackObject(this);
     }
 
+    bool ApiObjectBase::MarkDestroyed() {
+        const std::lock_guard<std::mutex> lock(*GetDevice()->GetObjectListMutex(GetType()));
+        return RemoveFromList();
+    }
+
     bool ApiObjectBase::DestroyApiObject() {
-        {
+        bool marked = MarkDestroyed();
-            const std::lock_guard<std::mutex> lock(*GetDevice()->GetObjectListMutex(GetType()));
+        if (marked) {
-            if (!RemoveFromList()) {
+            DestroyApiObjectImpl();
-                return false;
-            }
         }
-        DestroyApiObjectImpl();
+        return marked;
-        return true;
     }
 
     void ApiObjectBase::DestroyApiObjectImpl() {

src/dawn_native/ObjectBase.h

@@ -83,6 +83,12 @@ namespace dawn_native {
         // somewhere.
         void DeleteThis() override;
         void TrackInDevice();
+
+        // Thread-safe manner to mark an object as destroyed. Returns true iff the call was the
+        // "winning" attempt to destroy the object. This is useful when sub-classes may have extra
+        // pre-destruction steps that need to occur only once, i.e. Buffer needs to be unmapped
+        // before being destroyed.
+        bool MarkDestroyed();
         virtual void DestroyApiObjectImpl();
 
       private:

src/dawn_native/QuerySet.cpp

@@ -31,7 +31,7 @@ namespace dawn_native {
             }
 
           private:
-            void DestroyImpl() override {
+            void DestroyApiObjectImpl() override {
                 UNREACHABLE();
             }
         };
@@ -110,6 +110,11 @@ namespace dawn_native {
         }
 
         mQueryAvailability.resize(descriptor->count);
+        TrackInDevice();
+    }
+
+    QuerySetBase::QuerySetBase(DeviceBase* device) : ApiObjectBase(device, kLabelNotImplemented) {
+        TrackInDevice();
     }
 
     QuerySetBase::QuerySetBase(DeviceBase* device, ObjectBase::ErrorTag tag)
@@ -121,6 +126,11 @@ namespace dawn_native {
         ASSERT(mState == QuerySetState::Unavailable || mState == QuerySetState::Destroyed);
     }
 
+    bool QuerySetBase::DestroyApiObject() {
+        mState = QuerySetState::Destroyed;
+        return ApiObjectBase::DestroyApiObject();
+    }
+
     // static
     QuerySetBase* QuerySetBase::MakeError(DeviceBase* device) {
         return new ErrorQuerySet(device);
@@ -160,7 +170,7 @@ namespace dawn_native {
         if (GetDevice()->ConsumedError(ValidateDestroy())) {
             return;
         }
-        DestroyInternal();
+        DestroyApiObject();
     }
 
     MaybeError QuerySetBase::ValidateDestroy() const {
@@ -168,11 +178,4 @@ namespace dawn_native {
         return {};
     }
 
-    void QuerySetBase::DestroyInternal() {
-        if (mState != QuerySetState::Destroyed) {
-            DestroyImpl();
-        }
-        mState = QuerySetState::Destroyed;
-    }
-
 }  // namespace dawn_native

src/dawn_native/QuerySet.h

@@ -31,6 +31,7 @@ namespace dawn_native {
 
         static QuerySetBase* MakeError(DeviceBase* device);
 
+        bool DestroyApiObject() override;
         ObjectType GetType() const override;
 
         wgpu::QueryType GetQueryType() const;
@@ -46,13 +47,13 @@ namespace dawn_native {
 
       protected:
         QuerySetBase(DeviceBase* device, ObjectBase::ErrorTag tag);
+
+        // Constructor used only for mocking and testing.
+        QuerySetBase(DeviceBase* device);
+
         ~QuerySetBase() override;
 
-        void DestroyInternal();
-
       private:
-        virtual void DestroyImpl() = 0;
-
         MaybeError ValidateDestroy() const;
 
         wgpu::QueryType mQueryType;

src/dawn_native/d3d12/BufferD3D12.cpp

@@ -185,9 +185,7 @@ namespace dawn_native { namespace d3d12 {
         return {};
     }
 
-    Buffer::~Buffer() {
+    Buffer::~Buffer() = default;
-        DestroyInternal();
-    }
 
     ID3D12Resource* Buffer::GetD3D12Resource() const {
         return mResourceAllocation.GetD3D12Resource();
@@ -380,7 +378,7 @@ namespace dawn_native { namespace d3d12 {
         return mMappedData;
     }
 
-    void Buffer::DestroyImpl() {
+    void Buffer::DestroyApiObjectImpl() {
         if (mMappedData != nullptr) {
             // If the buffer is currently mapped, unmap without flushing the writes to the GPU
             // since the buffer cannot be used anymore. UnmapImpl checks mWrittenRange to know

src/dawn_native/d3d12/BufferD3D12.h

@@ -58,7 +58,7 @@ namespace dawn_native { namespace d3d12 {
         MaybeError Initialize(bool mappedAtCreation);
         MaybeError MapAsyncImpl(wgpu::MapMode mode, size_t offset, size_t size) override;
         void UnmapImpl() override;
-        void DestroyImpl() override;
+        void DestroyApiObjectImpl() override;
         bool IsCPUWritableAtCreation() const override;
         virtual MaybeError MapAtCreationImpl() override;
         void* GetMappedPointerImpl() override;

src/dawn_native/d3d12/QuerySetD3D12.cpp

@@ -55,12 +55,11 @@ namespace dawn_native { namespace d3d12 {
         return mQueryHeap.Get();
     }
 
-    QuerySet::~QuerySet() {
+    QuerySet::~QuerySet() = default;
-        DestroyInternal();
-    }
 
-    void QuerySet::DestroyImpl() {
+    void QuerySet::DestroyApiObjectImpl() {
         ToBackend(GetDevice())->ReferenceUntilUnused(mQueryHeap);
+        mQueryHeap = nullptr;
     }
 
 }}  // namespace dawn_native::d3d12

src/dawn_native/d3d12/QuerySetD3D12.h

@@ -35,7 +35,7 @@ namespace dawn_native { namespace d3d12 {
         MaybeError Initialize();
 
         // Dawn API
-        void DestroyImpl() override;
+        void DestroyApiObjectImpl() override;
 
         ComPtr<ID3D12QueryHeap> mQueryHeap;
     };

src/dawn_native/metal/BufferMTL.h

@@ -26,7 +26,7 @@ namespace dawn_native { namespace metal {
     class CommandRecordingContext;
     class Device;
 
-    class Buffer : public BufferBase {
+    class Buffer final : public BufferBase {
       public:
         static ResultOrError<Ref<Buffer>> Create(Device* device,
                                                  const BufferDescriptor* descriptor);
@@ -48,7 +48,7 @@ namespace dawn_native { namespace metal {
         ~Buffer() override;
         MaybeError MapAsyncImpl(wgpu::MapMode mode, size_t offset, size_t size) override;
         void UnmapImpl() override;
-        void DestroyImpl() override;
+        void DestroyApiObjectImpl() override;
         void* GetMappedPointerImpl() override;
         bool IsCPUWritableAtCreation() const override;
         MaybeError MapAtCreationImpl() override;

src/dawn_native/metal/BufferMTL.mm

@@ -140,9 +140,7 @@ namespace dawn_native { namespace metal {
         return {};
     }
 
-    Buffer::~Buffer() {
+    Buffer::~Buffer() = default;
-        DestroyInternal();
-    }
 
     id<MTLBuffer> Buffer::GetMTLBuffer() const {
         return mMtlBuffer.Get();
@@ -173,7 +171,7 @@ namespace dawn_native { namespace metal {
         // Nothing to do, Metal StorageModeShared buffers are always mapped.
     }
 
-    void Buffer::DestroyImpl() {
+    void Buffer::DestroyApiObjectImpl() {
         mMtlBuffer = nullptr;
     }
 

src/dawn_native/metal/QuerySetMTL.h

@@ -40,7 +40,7 @@ namespace dawn_native { namespace metal {
         MaybeError Initialize();
 
         // Dawn API
-        void DestroyImpl() override;
+        void DestroyApiObjectImpl() override;
 
         NSPRef<id<MTLBuffer>> mVisibilityBuffer;
         // Note that mCounterSampleBuffer cannot be an NSRef because the API_AVAILABLE macros don't

src/dawn_native/metal/QuerySetMTL.mm

@@ -121,11 +121,9 @@ namespace dawn_native { namespace metal {
         return mCounterSampleBuffer;
     }
 
-    QuerySet::~QuerySet() {
+    QuerySet::~QuerySet() = default;
-        DestroyInternal();
-    }
 
-    void QuerySet::DestroyImpl() {
+    void QuerySet::DestroyApiObjectImpl() {
         mVisibilityBuffer = nullptr;
 
         // mCounterSampleBuffer isn't an NSRef because API_AVAILABLE doesn't work will with

src/dawn_native/null/DeviceNull.cpp

@@ -294,11 +294,6 @@ namespace dawn_native { namespace null {
         mAllocatedSize = GetSize();
     }
 
-    Buffer::~Buffer() {
-        DestroyInternal();
-        ToBackend(GetDevice())->DecrementMemoryUsage(GetSize());
-    }
-
     bool Buffer::IsCPUWritableAtCreation() const {
         // Only return true for mappable buffers so we can test cases that need / don't need a
         // staging buffer.
@@ -334,7 +329,8 @@ namespace dawn_native { namespace null {
     void Buffer::UnmapImpl() {
     }
 
-    void Buffer::DestroyImpl() {
+    void Buffer::DestroyApiObjectImpl() {
+        ToBackend(GetDevice())->DecrementMemoryUsage(GetSize());
     }
 
     // CommandBuffer
@@ -349,11 +345,7 @@ namespace dawn_native { namespace null {
         : QuerySetBase(device, descriptor) {
     }
 
-    QuerySet::~QuerySet() {
+    void QuerySet::DestroyApiObjectImpl() {
-        DestroyInternal();
-    }
-
-    void QuerySet::DestroyImpl() {
     }
 
     // Queue

src/dawn_native/null/DeviceNull.h

@@ -226,10 +226,9 @@ namespace dawn_native { namespace null {
         void DoWriteBuffer(uint64_t bufferOffset, const void* data, size_t size);
 
       private:
-        ~Buffer() override;
         MaybeError MapAsyncImpl(wgpu::MapMode mode, size_t offset, size_t size) override;
         void UnmapImpl() override;
-        void DestroyImpl() override;
+        void DestroyApiObjectImpl() override;
         bool IsCPUWritableAtCreation() const override;
         MaybeError MapAtCreationImpl() override;
         void* GetMappedPointerImpl() override;
@@ -247,9 +246,7 @@ namespace dawn_native { namespace null {
         QuerySet(Device* device, const QuerySetDescriptor* descriptor);
 
       private:
-        ~QuerySet() override;
+        void DestroyApiObjectImpl() override;
-
-        void DestroyImpl() override;
     };
 
     class Queue final : public QueueBase {

src/dawn_native/opengl/BufferGL.cpp

@@ -61,9 +61,7 @@ namespace dawn_native { namespace opengl {
         }
     }
 
-    Buffer::~Buffer() {
+    Buffer::~Buffer() = default;
-        DestroyInternal();
-    }
 
     GLuint Buffer::GetHandle() const {
         return mBuffer;
@@ -176,7 +174,7 @@ namespace dawn_native { namespace opengl {
         mMappedData = nullptr;
     }
 
-    void Buffer::DestroyImpl() {
+    void Buffer::DestroyApiObjectImpl() {
         ToBackend(GetDevice())->gl.DeleteBuffers(1, &mBuffer);
         mBuffer = 0;
     }

src/dawn_native/opengl/BufferGL.h

@@ -42,7 +42,7 @@ namespace dawn_native { namespace opengl {
         ~Buffer() override;
         MaybeError MapAsyncImpl(wgpu::MapMode mode, size_t offset, size_t size) override;
         void UnmapImpl() override;
-        void DestroyImpl() override;
+        void DestroyApiObjectImpl() override;
         bool IsCPUWritableAtCreation() const override;
         MaybeError MapAtCreationImpl() override;
         void* GetMappedPointerImpl() override;

src/dawn_native/opengl/QuerySetGL.cpp

@@ -22,11 +22,9 @@ namespace dawn_native { namespace opengl {
         : QuerySetBase(device, descriptor) {
     }
 
-    QuerySet::~QuerySet() {
+    QuerySet::~QuerySet() = default;
-        DestroyInternal();
-    }
 
-    void QuerySet::DestroyImpl() {
+    void QuerySet::DestroyApiObjectImpl() {
     }
 
 }}  // namespace dawn_native::opengl

src/dawn_native/opengl/QuerySetGL.h

@@ -28,7 +28,7 @@ namespace dawn_native { namespace opengl {
       private:
         ~QuerySet() override;
 
-        void DestroyImpl() override;
+        void DestroyApiObjectImpl() override;
     };
 
 }}  // namespace dawn_native::opengl

src/dawn_native/vulkan/BufferVk.cpp

@@ -236,9 +236,7 @@ namespace dawn_native { namespace vulkan {
         return {};
     }
 
-    Buffer::~Buffer() {
+    Buffer::~Buffer() = default;
-        DestroyInternal();
-    }
 
     VkBuffer Buffer::GetHandle() const {
         return mHandle;
@@ -331,7 +329,7 @@ namespace dawn_native { namespace vulkan {
         return memory;
     }
 
-    void Buffer::DestroyImpl() {
+    void Buffer::DestroyApiObjectImpl() {
         ToBackend(GetDevice())->GetResourceMemoryAllocator()->Deallocate(&mMemoryAllocation);
 
         if (mHandle != VK_NULL_HANDLE) {

src/dawn_native/vulkan/BufferVk.h

@@ -65,7 +65,7 @@ namespace dawn_native { namespace vulkan {
 
         MaybeError MapAsyncImpl(wgpu::MapMode mode, size_t offset, size_t size) override;
         void UnmapImpl() override;
-        void DestroyImpl() override;
+        void DestroyApiObjectImpl() override;
         bool IsCPUWritableAtCreation() const override;
         MaybeError MapAtCreationImpl() override;
         void* GetMappedPointerImpl() override;

src/dawn_native/vulkan/QuerySetVk.cpp

@@ -94,11 +94,9 @@ namespace dawn_native { namespace vulkan {
         return mHandle;
     }
 
-    QuerySet::~QuerySet() {
+    QuerySet::~QuerySet() = default;
-        DestroyInternal();
-    }
 
-    void QuerySet::DestroyImpl() {
+    void QuerySet::DestroyApiObjectImpl() {
         if (mHandle != VK_NULL_HANDLE) {
             ToBackend(GetDevice())->GetFencedDeleter()->DeleteWhenUnused(mHandle);
             mHandle = VK_NULL_HANDLE;

src/dawn_native/vulkan/QuerySetVk.h

@@ -35,7 +35,7 @@ namespace dawn_native { namespace vulkan {
         using QuerySetBase::QuerySetBase;
         MaybeError Initialize();
 
-        void DestroyImpl() override;
+        void DestroyApiObjectImpl() override;
 
         VkQueryPool mHandle = VK_NULL_HANDLE;
     };

src/tests/BUILD.gn

@@ -147,6 +147,7 @@ source_set("dawn_native_mocks_sources") {
     "unittests/native/mocks/ComputePipelineMock.h",
     "unittests/native/mocks/DeviceMock.h",
     "unittests/native/mocks/PipelineLayoutMock.h",
+    "unittests/native/mocks/QuerySetMock.h",
     "unittests/native/mocks/RenderPipelineMock.h",
     "unittests/native/mocks/SamplerMock.h",
     "unittests/native/mocks/ShaderModuleMock.cpp",

src/tests/unittests/native/DestroyObjectTests.cpp

@@ -17,9 +17,11 @@
 #include "dawn_native/Toggles.h"
 #include "mocks/BindGroupLayoutMock.h"
 #include "mocks/BindGroupMock.h"
+#include "mocks/BufferMock.h"
 #include "mocks/ComputePipelineMock.h"
 #include "mocks/DeviceMock.h"
 #include "mocks/PipelineLayoutMock.h"
+#include "mocks/QuerySetMock.h"
 #include "mocks/RenderPipelineMock.h"
 #include "mocks/SamplerMock.h"
 #include "mocks/ShaderModuleMock.h"
@@ -139,6 +141,64 @@ namespace dawn_native { namespace {
         }
     }
 
+    TEST_F(DestroyObjectTests, BufferExplicit) {
+        {
+            BufferMock bufferMock(&mDevice, BufferBase::BufferState::Unmapped);
+            EXPECT_CALL(bufferMock, DestroyApiObjectImpl).Times(1);
+
+            EXPECT_TRUE(bufferMock.IsAlive());
+            bufferMock.DestroyApiObject();
+            EXPECT_FALSE(bufferMock.IsAlive());
+        }
+        {
+            BufferMock bufferMock(&mDevice, BufferBase::BufferState::Mapped);
+            {
+                InSequence seq;
+                EXPECT_CALL(bufferMock, UnmapImpl).Times(1);
+                EXPECT_CALL(bufferMock, DestroyApiObjectImpl).Times(1);
+            }
+
+            EXPECT_TRUE(bufferMock.IsAlive());
+            bufferMock.DestroyApiObject();
+            EXPECT_FALSE(bufferMock.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_F(DestroyObjectTests, BufferImplicit) {
+        {
+            BufferMock* bufferMock = new BufferMock(&mDevice, BufferBase::BufferState::Unmapped);
+            EXPECT_CALL(*bufferMock, DestroyApiObjectImpl).Times(1);
+            {
+                BufferDescriptor desc = {};
+                Ref<BufferBase> buffer;
+                EXPECT_CALL(mDevice, CreateBufferImpl)
+                    .WillOnce(Return(ByMove(AcquireRef(bufferMock))));
+                DAWN_ASSERT_AND_ASSIGN(buffer, mDevice.CreateBuffer(&desc));
+
+                EXPECT_TRUE(buffer->IsAlive());
+            }
+        }
+        {
+            BufferMock* bufferMock = new BufferMock(&mDevice, BufferBase::BufferState::Mapped);
+            {
+                InSequence seq;
+                EXPECT_CALL(*bufferMock, UnmapImpl).Times(1);
+                EXPECT_CALL(*bufferMock, DestroyApiObjectImpl).Times(1);
+            }
+            {
+                BufferDescriptor desc = {};
+                Ref<BufferBase> buffer;
+                EXPECT_CALL(mDevice, CreateBufferImpl)
+                    .WillOnce(Return(ByMove(AcquireRef(bufferMock))));
+                DAWN_ASSERT_AND_ASSIGN(buffer, mDevice.CreateBuffer(&desc));
+
+                EXPECT_TRUE(buffer->IsAlive());
+            }
+        }
+    }
+
     TEST_F(DestroyObjectTests, ComputePipelineExplicit) {
         ComputePipelineMock computePipelineMock(&mDevice);
         EXPECT_CALL(computePipelineMock, DestroyApiObjectImpl).Times(1);
@@ -203,6 +263,31 @@ namespace dawn_native { namespace {
         }
     }
 
+    TEST_F(DestroyObjectTests, QuerySetExplicit) {
+        QuerySetMock querySetMock(&mDevice);
+        EXPECT_CALL(querySetMock, DestroyApiObjectImpl).Times(1);
+
+        EXPECT_TRUE(querySetMock.IsAlive());
+        querySetMock.DestroyApiObject();
+        EXPECT_FALSE(querySetMock.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_F(DestroyObjectTests, QuerySetImplicit) {
+        QuerySetMock* querySetMock = new QuerySetMock(&mDevice);
+        EXPECT_CALL(*querySetMock, DestroyApiObjectImpl).Times(1);
+        {
+            QuerySetDescriptor desc = {};
+            Ref<QuerySetBase> querySet;
+            EXPECT_CALL(mDevice, CreateQuerySetImpl)
+                .WillOnce(Return(ByMove(AcquireRef(querySetMock))));
+            DAWN_ASSERT_AND_ASSIGN(querySet, mDevice.CreateQuerySet(&desc));
+
+            EXPECT_TRUE(querySet->IsAlive());
+        }
+    }
+
     TEST_F(DestroyObjectTests, RenderPipelineExplicit) {
         RenderPipelineMock renderPipelineMock(&mDevice);
         EXPECT_CALL(renderPipelineMock, DestroyApiObjectImpl).Times(1);
@@ -329,8 +414,10 @@ namespace dawn_native { namespace {
     TEST_F(DestroyObjectTests, DestroyObjects) {
         BindGroupMock* bindGroupMock = new BindGroupMock(&mDevice);
         BindGroupLayoutMock* bindGroupLayoutMock = new BindGroupLayoutMock(&mDevice);
+        BufferMock* bufferMock = new BufferMock(&mDevice, BufferBase::BufferState::Unmapped);
         ComputePipelineMock* computePipelineMock = new ComputePipelineMock(&mDevice);
         PipelineLayoutMock* pipelineLayoutMock = new PipelineLayoutMock(&mDevice);
+        QuerySetMock* querySetMock = new QuerySetMock(&mDevice);
         RenderPipelineMock* renderPipelineMock = new RenderPipelineMock(&mDevice);
         SamplerMock* samplerMock = new SamplerMock(&mDevice);
         ShaderModuleMock* shaderModuleMock = new ShaderModuleMock(&mDevice);
@@ -344,7 +431,9 @@ namespace dawn_native { namespace {
             EXPECT_CALL(*bindGroupMock, DestroyApiObjectImpl).Times(1);
             EXPECT_CALL(*bindGroupLayoutMock, DestroyApiObjectImpl).Times(1);
             EXPECT_CALL(*shaderModuleMock, DestroyApiObjectImpl).Times(1);
+            EXPECT_CALL(*querySetMock, DestroyApiObjectImpl).Times(1);
             EXPECT_CALL(*samplerMock, DestroyApiObjectImpl).Times(1);
+            EXPECT_CALL(*bufferMock, DestroyApiObjectImpl).Times(1);
         }
 
         Ref<BindGroupBase> bindGroup;
@@ -366,6 +455,14 @@ namespace dawn_native { namespace {
             EXPECT_TRUE(bindGroupLayout->IsCachedReference());
         }
 
+        Ref<BufferBase> buffer;
+        {
+            BufferDescriptor desc = {};
+            EXPECT_CALL(mDevice, CreateBufferImpl).WillOnce(Return(ByMove(AcquireRef(bufferMock))));
+            DAWN_ASSERT_AND_ASSIGN(buffer, mDevice.CreateBuffer(&desc));
+            EXPECT_TRUE(buffer->IsAlive());
+        }
+
         Ref<ComputePipelineBase> computePipeline;
         {
             // Compute pipelines usually set their hash values at construction, but the mock does
@@ -397,6 +494,15 @@ namespace dawn_native { namespace {
             EXPECT_TRUE(pipelineLayout->IsCachedReference());
         }
 
+        Ref<QuerySetBase> querySet;
+        {
+            QuerySetDescriptor desc = {};
+            EXPECT_CALL(mDevice, CreateQuerySetImpl)
+                .WillOnce(Return(ByMove(AcquireRef(querySetMock))));
+            DAWN_ASSERT_AND_ASSIGN(querySet, mDevice.CreateQuerySet(&desc));
+            EXPECT_TRUE(querySet->IsAlive());
+        }
+
         Ref<RenderPipelineBase> renderPipeline;
         {
             // Render pipelines usually set their hash values at construction, but the mock does
@@ -457,8 +563,10 @@ namespace dawn_native { namespace {
         mDevice.DestroyObjects();
         EXPECT_FALSE(bindGroup->IsAlive());
         EXPECT_FALSE(bindGroupLayout->IsAlive());
+        EXPECT_FALSE(buffer->IsAlive());
         EXPECT_FALSE(computePipeline->IsAlive());
         EXPECT_FALSE(pipelineLayout->IsAlive());
+        EXPECT_FALSE(querySet->IsAlive());
         EXPECT_FALSE(renderPipeline->IsAlive());
         EXPECT_FALSE(sampler->IsAlive());
         EXPECT_FALSE(shaderModule->IsAlive());

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

@@ -0,0 +1,46 @@
+// Copyright 2021 The Dawn Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef TESTS_UNITTESTS_NATIVE_MOCKS_BUFFER_MOCK_H_
+#define TESTS_UNITTESTS_NATIVE_MOCKS_BUFFER_MOCK_H_
+
+#include "dawn_native/Buffer.h"
+#include "dawn_native/Device.h"
+
+#include <gmock/gmock.h>
+
+namespace dawn_native {
+
+    class BufferMock : public BufferBase {
+      public:
+        BufferMock(DeviceBase* device, BufferBase::BufferState state) : BufferBase(device, state) {
+        }
+        ~BufferMock() override = default;
+
+        MOCK_METHOD(void, DestroyApiObjectImpl, (), (override));
+
+        MOCK_METHOD(MaybeError, MapAtCreationImpl, (), (override));
+        MOCK_METHOD(MaybeError,
+                    MapAsyncImpl,
+                    (wgpu::MapMode mode, size_t offset, size_t size),
+                    (override));
+        MOCK_METHOD(void, UnmapImpl, (), (override));
+        MOCK_METHOD(void*, GetMappedPointerImpl, (), (override));
+
+        MOCK_METHOD(bool, IsCPUWritableAtCreation, (), (const, override));
+    };
+
+}  // namespace dawn_native
+
+#endif  // TESTS_UNITTESTS_NATIVE_MOCKS_BINDGROUP_MOCK_H_

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

@@ -0,0 +1,36 @@
+// Copyright 2021 The Dawn Authors
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//     http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+#ifndef TESTS_UNITTESTS_NATIVE_MOCKS_QUERYSET_MOCK_H_
+#define TESTS_UNITTESTS_NATIVE_MOCKS_QUERYSET_MOCK_H_
+
+#include "dawn_native/Device.h"
+#include "dawn_native/QuerySet.h"
+
+#include <gmock/gmock.h>
+
+namespace dawn_native {
+
+    class QuerySetMock : public QuerySetBase {
+      public:
+        QuerySetMock(DeviceBase* device) : QuerySetBase(device) {
+        }
+        ~QuerySetMock() override = default;
+
+        MOCK_METHOD(void, DestroyApiObjectImpl, (), (override));
+    };
+
+}  // namespace dawn_native
+
+#endif  // TESTS_UNITTESTS_NATIVE_MOCKS_QUERYSET_MOCK_H_

src/tests/unittests/validation/BufferValidationTests.cpp

@@ -467,7 +467,18 @@ TEST_F(BufferValidationTest, MappedAtCreationSizeAlignment) {
     ASSERT_DEVICE_ERROR(BufferMappedAtCreation(2, wgpu::BufferUsage::MapWrite));
 }
 
-// Test that it is valid to destroy an unmapped buffer
+// Test that it is valid to destroy an error buffer
+TEST_F(BufferValidationTest, DestroyErrorBuffer) {
+    wgpu::BufferDescriptor desc;
+    desc.size = 4;
+    desc.usage = wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite;
+    wgpu::Buffer buf;
+    ASSERT_DEVICE_ERROR(buf = device.CreateBuffer(&desc));
+
+    buf.Destroy();
+}
+
+// Test that it is valid to Destroy an unmapped buffer
 TEST_F(BufferValidationTest, DestroyUnmappedBuffer) {
     {
         wgpu::Buffer buf = CreateMapReadBuffer(4);
@@ -486,6 +497,17 @@ TEST_F(BufferValidationTest, DestroyDestroyedBuffer) {
     buf.Destroy();
 }
 
+// Test that it is invalid to Unmap an error buffer
+TEST_F(BufferValidationTest, UnmapErrorBuffer) {
+    wgpu::BufferDescriptor desc;
+    desc.size = 4;
+    desc.usage = wgpu::BufferUsage::MapRead | wgpu::BufferUsage::MapWrite;
+    wgpu::Buffer buf;
+    ASSERT_DEVICE_ERROR(buf = device.CreateBuffer(&desc));
+
+    ASSERT_DEVICE_ERROR(buf.Unmap());
+}
+
 // Test that it is invalid to Unmap a destroyed buffer
 TEST_F(BufferValidationTest, UnmapDestroyedBuffer) {
     {