Make Adapter and Instance lifetimes more robust Previously, we would get a use-after-free if you dropped the instance before an adapter created from it. This CL fixes up the lifetimes such that Device refs Adapter refs Instance. Instance uses a cycle-breaking refcount so that it releases internal refs to its adapters when the last external ref is dropped. Bug: none Change-Id: I5304ec86f425247d4c45ca342fda393cc19689e3 Reviewed-on: https://dawn-review.googlesource.com/c/dawn/+/99820 Kokoro: Kokoro <noreply+kokoro@google.com> Reviewed-by: Loko Kung <lokokung@google.com> Commit-Queue: Austin Eng <enga@chromium.org>
diff --git a/include/dawn/native/DawnNative.h b/include/dawn/native/DawnNative.h index 9dab397..0d0df14 100644 --- a/include/dawn/native/DawnNative.h +++ b/include/dawn/native/DawnNative.h
@@ -190,6 +190,9 @@ // Backdoor to get the number of deprecation warnings for testing DAWN_NATIVE_EXPORT size_t GetDeprecationWarningCountForTesting(WGPUDevice device); +// Backdoor to get the number of adapters an instance knows about for testing +DAWN_NATIVE_EXPORT size_t GetAdapterCountForTesting(WGPUInstance instance); + // Query if texture has been initialized DAWN_NATIVE_EXPORT bool IsTextureSubresourceInitialized( WGPUTexture texture,
diff --git a/src/dawn/native/Adapter.cpp b/src/dawn/native/Adapter.cpp index 68e0420..7415312 100644 --- a/src/dawn/native/Adapter.cpp +++ b/src/dawn/native/Adapter.cpp
@@ -31,6 +31,8 @@ mSupportedFeatures.EnableFeature(Feature::DawnInternalUsages); } +AdapterBase::~AdapterBase() = default; + MaybeError AdapterBase::Initialize() { DAWN_TRY_CONTEXT(InitializeImpl(), "initializing adapter (backend=%s)", mBackend); InitializeVendorArchitectureImpl(); @@ -157,7 +159,7 @@ } InstanceBase* AdapterBase::GetInstance() const { - return mInstance; + return mInstance.Get(); } FeaturesSet AdapterBase::GetSupportedFeatures() const {
diff --git a/src/dawn/native/Adapter.h b/src/dawn/native/Adapter.h index f979b35..6b6448f 100644 --- a/src/dawn/native/Adapter.h +++ b/src/dawn/native/Adapter.h
@@ -33,7 +33,7 @@ class AdapterBase : public RefCounted { public: AdapterBase(InstanceBase* instance, wgpu::BackendType backend); - ~AdapterBase() override = default; + ~AdapterBase() override; MaybeError Initialize(); @@ -90,7 +90,7 @@ ResultOrError<Ref<DeviceBase>> CreateDeviceInternal(const DeviceDescriptor* descriptor); virtual MaybeError ResetInternalDeviceForTestingImpl(); - InstanceBase* mInstance = nullptr; + Ref<InstanceBase> mInstance; wgpu::BackendType mBackend; CombinedLimits mLimits; bool mUseTieredLimits = false;
diff --git a/src/dawn/native/DawnNative.cpp b/src/dawn/native/DawnNative.cpp index 5cb93e2..89c48ac 100644 --- a/src/dawn/native/DawnNative.cpp +++ b/src/dawn/native/DawnNative.cpp
@@ -191,7 +191,7 @@ Instance::~Instance() { if (mImpl != nullptr) { - mImpl->Release(); + mImpl->APIRelease(); mImpl = nullptr; } } @@ -256,6 +256,10 @@ return FromAPI(device)->GetDeprecationWarningCountForTesting(); } +size_t GetAdapterCountForTesting(WGPUInstance instance) { + return FromAPI(instance)->GetAdapters().size(); +} + bool IsTextureSubresourceInitialized(WGPUTexture texture, uint32_t baseMipLevel, uint32_t levelCount,
diff --git a/src/dawn/native/Device.cpp b/src/dawn/native/Device.cpp index f737e47..a83d185 100644 --- a/src/dawn/native/Device.cpp +++ b/src/dawn/native/Device.cpp
@@ -171,8 +171,8 @@ // DeviceBase DeviceBase::DeviceBase(AdapterBase* adapter, const DeviceDescriptor* descriptor) - : mInstance(adapter->GetInstance()), mAdapter(adapter), mNextPipelineCompatibilityToken(1) { - mInstance->IncrementDeviceCountForTesting(); + : mAdapter(adapter), mNextPipelineCompatibilityToken(1) { + mAdapter->GetInstance()->IncrementDeviceCountForTesting(); ASSERT(descriptor != nullptr); AdapterProperties adapterProperties; @@ -221,9 +221,9 @@ // 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; - // mInstance is not set for mock test devices. - if (mInstance != nullptr) { - mInstance->DecrementDeviceCountForTesting(); + // mAdapter is not set for mock test devices. + if (mAdapter != nullptr) { + mAdapter->GetInstance()->DecrementDeviceCountForTesting(); } } @@ -628,7 +628,7 @@ // generate cache keys. We can lift the dependency once we also cache frontend parsing, // transformations, and reflection. if (IsToggleEnabled(Toggle::EnableBlobCache)) { - return mInstance->GetBlobCache(); + return mAdapter->GetInstance()->GetBlobCache(); } #endif return nullptr; @@ -696,7 +696,7 @@ } AdapterBase* DeviceBase::GetAdapter() const { - return mAdapter; + return mAdapter.Get(); } dawn::platform::Platform* DeviceBase::GetPlatform() const { @@ -1286,7 +1286,7 @@ AdapterBase* DeviceBase::APIGetAdapter() { mAdapter->Reference(); - return mAdapter; + return mAdapter.Get(); } QueueBase* DeviceBase::APIGetQueue() {
diff --git a/src/dawn/native/Device.h b/src/dawn/native/Device.h index b8b145b..9e4fe03 100644 --- a/src/dawn/native/Device.h +++ b/src/dawn/native/Device.h
@@ -527,12 +527,7 @@ std::unique_ptr<ErrorScopeStack> mErrorScopeStack; - // The Device keeps a ref to the Instance so that any live Device keeps the Instance alive. - // The Instance shouldn't need to ref child objects so this shouldn't introduce ref cycles. - // The Device keeps a simple pointer to the Adapter because the Adapter is owned by the - // Instance. - Ref<InstanceBase> mInstance; - AdapterBase* mAdapter = nullptr; + Ref<AdapterBase> mAdapter; // The object caches aren't exposed in the header as they would require a lot of // additional includes.
diff --git a/src/dawn/native/Instance.cpp b/src/dawn/native/Instance.cpp index afe9c1d..a582199 100644 --- a/src/dawn/native/Instance.cpp +++ b/src/dawn/native/Instance.cpp
@@ -125,6 +125,17 @@ InstanceBase::~InstanceBase() = default; +void InstanceBase::WillDropLastExternalRef() { + // InstanceBase uses RefCountedWithExternalCount to break refcycles. + // + // InstanceBase holds Refs to AdapterBases it has discovered, which hold Refs back to the + // InstanceBase. + // In order to break this cycle and prevent leaks, when the application drops the last external + // ref and WillDropLastExternalRef is called, the instance clears out any member refs to + // adapters that hold back-refs to the instance - thus breaking any reference cycles. + mAdapters.clear(); +} + // TODO(crbug.com/dawn/832): make the platform an initialization parameter of the instance. MaybeError InstanceBase::Initialize(const InstanceDescriptor* descriptor) { DAWN_TRY(ValidateSingleSType(descriptor->nextInChain, wgpu::SType::DawnInstanceDescriptor));
diff --git a/src/dawn/native/Instance.h b/src/dawn/native/Instance.h index df22f27..0589061 100644 --- a/src/dawn/native/Instance.h +++ b/src/dawn/native/Instance.h
@@ -27,6 +27,7 @@ #include "dawn/native/BackendConnection.h" #include "dawn/native/BlobCache.h" #include "dawn/native/Features.h" +#include "dawn/native/RefCountedWithExternalCount.h" #include "dawn/native/Toggles.h" #include "dawn/native/dawn_platform.h" @@ -45,7 +46,7 @@ // This is called InstanceBase for consistency across the frontend, even if the backends don't // specialize this class. -class InstanceBase final : public RefCounted { +class InstanceBase final : public RefCountedWithExternalCount { public: static Ref<InstanceBase> Create(const InstanceDescriptor* descriptor = nullptr); @@ -110,6 +111,8 @@ InstanceBase(); ~InstanceBase() override; + void WillDropLastExternalRef() override; + InstanceBase(const InstanceBase& other) = delete; InstanceBase& operator=(const InstanceBase& other) = delete;
diff --git a/src/dawn/tests/end2end/DeviceInitializationTests.cpp b/src/dawn/tests/end2end/DeviceInitializationTests.cpp index 0c7e621..023b7ac 100644 --- a/src/dawn/tests/end2end/DeviceInitializationTests.cpp +++ b/src/dawn/tests/end2end/DeviceInitializationTests.cpp
@@ -13,6 +13,7 @@ // limitations under the License. #include <memory> +#include <utility> #include <vector> #include "dawn/dawn_proc.h" @@ -21,9 +22,48 @@ #include "dawn/utils/WGPUHelpers.h" class DeviceInitializationTest : public testing::Test { + protected: void SetUp() override { dawnProcSetProcs(&dawn::native::GetProcs()); } void TearDown() override { dawnProcSetProcs(nullptr); } + + // Test that the device can still be used by testing a buffer copy. + void ExpectDeviceUsable(wgpu::Device device) { + wgpu::Buffer src = + utils::CreateBufferFromData<uint32_t>(device, wgpu::BufferUsage::CopySrc, {1, 2, 3, 4}); + + wgpu::Buffer dst = utils::CreateBufferFromData<uint32_t>( + device, wgpu::BufferUsage::CopyDst | wgpu::BufferUsage::MapRead, {0, 0, 0, 0}); + + wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); + encoder.CopyBufferToBuffer(src, 0, dst, 0, 4 * sizeof(uint32_t)); + + wgpu::CommandBuffer commands = encoder.Finish(); + device.GetQueue().Submit(1, &commands); + + bool done = false; + dst.MapAsync( + wgpu::MapMode::Read, 0, 4 * sizeof(uint32_t), + [](WGPUBufferMapAsyncStatus status, void* userdata) { + EXPECT_EQ(status, WGPUBufferMapAsyncStatus_Success); + *static_cast<bool*>(userdata) = true; + }, + &done); + + // Note: we can't actually test this if Tick moves over to + // wgpuInstanceProcessEvents. We can still test that object creation works + // without crashing. + while (!done) { + device.Tick(); + utils::USleep(100); + } + + const uint32_t* mapping = static_cast<const uint32_t*>(dst.GetConstMappedRange()); + EXPECT_EQ(mapping[0], 1u); + EXPECT_EQ(mapping[1], 2u); + EXPECT_EQ(mapping[2], 3u); + EXPECT_EQ(mapping[3], 4u); + } }; // Test that device operations are still valid if the reference to the instance @@ -66,40 +106,64 @@ } } - // Now, test that the device can still be used by testing a buffer copy. - wgpu::Buffer src = - utils::CreateBufferFromData<uint32_t>(device, wgpu::BufferUsage::CopySrc, {1, 2, 3, 4}); + if (device) { + ExpectDeviceUsable(std::move(device)); + } + } +} - wgpu::Buffer dst = utils::CreateBufferFromData<uint32_t>( - device, wgpu::BufferUsage::CopyDst | wgpu::BufferUsage::MapRead, {0, 0, 0, 0}); +// Test that it is still possible to create a device from an adapter after the reference to the +// instance is dropped. +TEST_F(DeviceInitializationTest, AdapterOutlivesInstance) { + // Get properties of all available adapters and then free the instance. + // We want to create a device on a fresh instance and adapter each time. + std::vector<wgpu::AdapterProperties> availableAdapterProperties; + { + auto instance = std::make_unique<dawn::native::Instance>(); + instance->DiscoverDefaultAdapters(); + for (const dawn::native::Adapter& adapter : instance->GetAdapters()) { + wgpu::AdapterProperties properties; + adapter.GetProperties(&properties); - wgpu::CommandEncoder encoder = device.CreateCommandEncoder(); - encoder.CopyBufferToBuffer(src, 0, dst, 0, 4 * sizeof(uint32_t)); + if (properties.backendType == wgpu::BackendType::Null) { + continue; + } + availableAdapterProperties.push_back(properties); + } + } - wgpu::CommandBuffer commands = encoder.Finish(); - device.GetQueue().Submit(1, &commands); + for (const wgpu::AdapterProperties& desiredProperties : availableAdapterProperties) { + wgpu::Adapter adapter; - bool done = false; - dst.MapAsync( - wgpu::MapMode::Read, 0, 4 * sizeof(uint32_t), - [](WGPUBufferMapAsyncStatus status, void* userdata) { - EXPECT_EQ(status, WGPUBufferMapAsyncStatus_Success); - *static_cast<bool*>(userdata) = true; - }, - &done); + auto instance = std::make_unique<dawn::native::Instance>(); + // Save a pointer to the instance. + // It will only be valid as long as the instance is alive. + WGPUInstance unsafeInstancePtr = instance->Get(); - // Note: we can't actually test this if Tick moves over to - // wgpuInstanceProcessEvents. We can still test that object creation works - // without crashing. - while (!done) { - device.Tick(); - utils::USleep(100); + instance->DiscoverDefaultAdapters(); + for (dawn::native::Adapter& nativeAdapter : instance->GetAdapters()) { + wgpu::AdapterProperties properties; + nativeAdapter.GetProperties(&properties); + + if (properties.deviceID == desiredProperties.deviceID && + properties.vendorID == desiredProperties.vendorID && + properties.adapterType == desiredProperties.adapterType && + properties.backendType == desiredProperties.backendType) { + // Save the adapter, and reset the instance. + // Check that the number of adapters before the reset is > 0, and after the reset + // is 0. Unsafe, but we assume the pointer is still valid since the adapter is + // holding onto the instance. The instance should have cleared all internal + // references to adapters when the last external ref is dropped. + adapter = wgpu::Adapter(nativeAdapter.Get()); + EXPECT_GT(dawn::native::GetAdapterCountForTesting(unsafeInstancePtr), 0u); + instance.reset(); + EXPECT_EQ(dawn::native::GetAdapterCountForTesting(unsafeInstancePtr), 0u); + break; + } } - const uint32_t* mapping = static_cast<const uint32_t*>(dst.GetConstMappedRange()); - EXPECT_EQ(mapping[0], 1u); - EXPECT_EQ(mapping[1], 2u); - EXPECT_EQ(mapping[2], 3u); - EXPECT_EQ(mapping[3], 4u); + if (adapter) { + ExpectDeviceUsable(adapter.CreateDevice()); + } } }
diff --git a/src/dawn/tests/unittests/ToBackendTests.cpp b/src/dawn/tests/unittests/ToBackendTests.cpp index 91c4b66..8720a27 100644 --- a/src/dawn/tests/unittests/ToBackendTests.cpp +++ b/src/dawn/tests/unittests/ToBackendTests.cpp
@@ -19,17 +19,23 @@ #include "dawn/common/RefCounted.h" #include "dawn/native/ToBackend.h" -// Make our own Base - Backend object pair, reusing the AdapterBase name +// Make our own Base - Backend object pair, reusing the MyObjectBase name namespace dawn::native { -class AdapterBase : public RefCounted {}; -class MyAdapter : public AdapterBase {}; +class MyObjectBase : public RefCounted {}; + +class MyObject : public MyObjectBase {}; struct MyBackendTraits { - using AdapterType = MyAdapter; + using MyObjectType = MyObject; }; -// Instanciate ToBackend for our "backend" +template <typename BackendTraits> +struct ToBackendTraits<MyObjectBase, BackendTraits> { + using BackendType = typename BackendTraits::MyObjectType; +}; + +// Instantiate ToBackend for our "backend" template <typename T> auto ToBackend(T&& common) -> decltype(ToBackendBase<MyBackendTraits>(common)) { return ToBackendBase<MyBackendTraits>(common); @@ -38,49 +44,48 @@ // Test that ToBackend correctly converts pointers to base classes. TEST(ToBackend, Pointers) { { - MyAdapter* adapter = new MyAdapter; - const AdapterBase* base = adapter; + MyObject* myObject = new MyObject; + const MyObjectBase* base = myObject; auto* backendAdapter = ToBackend(base); - static_assert(std::is_same<decltype(backendAdapter), const MyAdapter*>::value); - ASSERT_EQ(adapter, backendAdapter); + static_assert(std::is_same<decltype(backendAdapter), const MyObject*>::value); + ASSERT_EQ(myObject, backendAdapter); - adapter->Release(); + myObject->Release(); } { - MyAdapter* adapter = new MyAdapter; - AdapterBase* base = adapter; + MyObject* myObject = new MyObject; + MyObjectBase* base = myObject; auto* backendAdapter = ToBackend(base); - static_assert(std::is_same<decltype(backendAdapter), MyAdapter*>::value); - ASSERT_EQ(adapter, backendAdapter); + static_assert(std::is_same<decltype(backendAdapter), MyObject*>::value); + ASSERT_EQ(myObject, backendAdapter); - adapter->Release(); + myObject->Release(); } } // Test that ToBackend correctly converts Refs to base classes. TEST(ToBackend, Ref) { { - MyAdapter* adapter = new MyAdapter; - const Ref<AdapterBase> base(adapter); + MyObject* myObject = new MyObject; + const Ref<MyObjectBase> base(myObject); const auto& backendAdapter = ToBackend(base); - static_assert(std::is_same<decltype(ToBackend(base)), const Ref<MyAdapter>&>::value); - ASSERT_EQ(adapter, backendAdapter.Get()); + static_assert(std::is_same<decltype(ToBackend(base)), const Ref<MyObject>&>::value); + ASSERT_EQ(myObject, backendAdapter.Get()); - adapter->Release(); + myObject->Release(); } { - MyAdapter* adapter = new MyAdapter; - Ref<AdapterBase> base(adapter); + MyObject* myObject = new MyObject; + Ref<MyObjectBase> base(myObject); auto backendAdapter = ToBackend(base); - static_assert(std::is_same<decltype(ToBackend(base)), Ref<MyAdapter>&>::value); - ASSERT_EQ(adapter, backendAdapter.Get()); + static_assert(std::is_same<decltype(ToBackend(base)), Ref<MyObject>&>::value); + ASSERT_EQ(myObject, backendAdapter.Get()); - adapter->Release(); + myObject->Release(); } } - } // namespace dawn::native