From 204d048cc843fada829a3cea89475c3dc3ab9823 Mon Sep 17 00:00:00 2001 From: benstevens48 Date: Wed, 22 Jan 2020 21:02:39 +0000 Subject: [PATCH 1/4] first attempt to fix deadlock in GetOrCreate --- winrt/lib/utils/ResourceManager.cpp | 58 +++++++++++++++++++++++++++-- winrt/lib/utils/ResourceManager.h | 9 ++++- winrt/lib/utils/ResourceWrapper.h | 9 +++-- 3 files changed, 67 insertions(+), 9 deletions(-) diff --git a/winrt/lib/utils/ResourceManager.cpp b/winrt/lib/utils/ResourceManager.cpp index d42154118..50fef95fb 100644 --- a/winrt/lib/utils/ResourceManager.cpp +++ b/winrt/lib/utils/ResourceManager.cpp @@ -30,6 +30,9 @@ std::unordered_map ResourceManager::m_resources; std::recursive_mutex ResourceManager::m_mutex; +std::unordered_multiset ResourceManager::m_wrappingResources; +std::unordered_set ResourceManager::m_creatingWrappers; + // When adding new types here, please also update the "Types that support interop" table in winrt\docsrc\Interop.aml. std::vector ResourceManager::tryCreateFunctions = { @@ -81,12 +84,18 @@ std::vector ResourceManager::tryCreateFuncti // Called by the ResourceWrapper constructor, to add itself to the interop mapping table. -void ResourceManager::Add(IUnknown* resource, IInspectable* wrapper) +void ResourceManager::Add(IUnknown* resource, IInspectable* wrapper, IUnknown* wrapperIdentity) { ComPtr resourceIdentity = AsUnknown(resource); std::lock_guard lock(m_mutex); + //If this resource is being wrapped by GetOrCreate, then add wrapperIdentity to m_creatingWrappers instead of adding the resource to m_resources. + if (m_wrappingResources.find(resourceIdentity.Get()) != m_wrappingResources.end()) { + m_creatingWrappers.insert(wrapperIdentity); + return; + } + auto result = m_resources.insert(std::make_pair(resourceIdentity.Get(), AsWeak(wrapper))); if (!result.second) @@ -95,12 +104,17 @@ void ResourceManager::Add(IUnknown* resource, IInspectable* wrapper) // Called by ResourceWrapper::Close, to remove itself from the interop mapping table. -void ResourceManager::Remove(IUnknown* resource) +void ResourceManager::Remove(IUnknown* resource, IUnknown* wrapperIdentity) { ComPtr resourceIdentity = AsUnknown(resource); std::lock_guard lock(m_mutex); + //If this wrapper is being created by GetOrCreate, remove from m_creatingWrappers intead of removing the resource from m_resources. + if (m_creatingWrappers.erase(wrapperIdentity) > 0) { + return; + } + auto result = m_resources.erase(resourceIdentity.Get()); if (result != 1) @@ -113,11 +127,10 @@ ComPtr ResourceManager::GetOrCreate(ICanvasDevice* device, IUnknow ComPtr resourceIdentity = AsUnknown(resource); ComPtr wrapper; - std::lock_guard lock(m_mutex); + std::unique_lock lock(m_mutex); // Do we already have a wrapper around this resource? auto it = m_resources.find(resourceIdentity.Get()); - if (it != m_resources.end()) { wrapper = LockWeakRef(it->second); @@ -126,6 +139,19 @@ ComPtr ResourceManager::GetOrCreate(ICanvasDevice* device, IUnknow // Create a new wrapper instance? if (!wrapper) { + //Add the resource to the list of resources being wrapped, then unlock to avoid deadlock scenarios + m_wrappingResources.insert(resourceIdentity.Get()); + lock.unlock(); + + //Ensure the resource is removed from m_wrappingResources on leaving scope. + auto endWrapWarden = MakeScopeWarden([&] { + std::lock_guard endWrapLock(m_mutex); + auto endWrapIt = m_wrappingResources.find(resourceIdentity.Get()); + if (endWrapIt != m_wrappingResources.end()) { + m_wrappingResources.erase(endWrapIt); + } + }); + for (auto& tryCreateFunction : tryCreateFunctions) { if (tryCreateFunction(device, resource, dpi, &wrapper)) @@ -139,6 +165,30 @@ ComPtr ResourceManager::GetOrCreate(ICanvasDevice* device, IUnknow { ThrowHR(E_NOINTERFACE, Strings::ResourceManagerUnknownType); } + + lock.lock(); + //Check to see if another wrapper was created simultaneously for this resource while we were creating a wrapper. + ComPtr existingWrapper; + it = m_resources.find(resourceIdentity.Get()); + if (it != m_resources.end()) + { + existingWrapper = LockWeakRef(it->second); + } + if (existingWrapper) { + //If so, unlock and use the other wrapper. + lock.unlock(); + wrapper = existingWrapper; + } else { + //Else, remove the wrapper from the m_creatingWrappers set and add the resource to m_resources. + m_creatingWrappers.erase(AsUnknown(wrapper.Get()).Get()); + auto result = m_resources.insert(std::make_pair(resourceIdentity.Get(), AsWeak(wrapper.Get()))); + if (!result.second) { + ThrowHR(E_UNEXPECTED); + } + lock.unlock(); + } + } else { + lock.unlock(); } // Validate that the object we got back reports the expected device and DPI. diff --git a/winrt/lib/utils/ResourceManager.h b/winrt/lib/utils/ResourceManager.h index 0a324bb4d..d7da9b9ef 100644 --- a/winrt/lib/utils/ResourceManager.h +++ b/winrt/lib/utils/ResourceManager.h @@ -3,6 +3,7 @@ // Licensed under the MIT License. See LICENSE.txt in the project root for license information. #pragma once +#include namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { @@ -26,8 +27,8 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { public: // Used by ResourceWrapper to maintain its state in the interop mapping table. - static void Add(IUnknown* resource, IInspectable* wrapper); - static void Remove(IUnknown* resource); + static void Add(IUnknown* resource, IInspectable* wrapper, IUnknown * wrapperIdentity); + static void Remove(IUnknown* resource, IUnknown* wrapperIdentity); // Used internally, and exposed to apps via CanvasDeviceFactory::GetOrCreate and Microsoft.Graphics.Canvas.native.h. @@ -158,6 +159,10 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas static std::unordered_map m_resources; static std::recursive_mutex m_mutex; + //Used temporarily by GetOrCreate in conjunction with Add/Remove to prevent duplicate wrapped resources without having to lock. + static std::unordered_multiset m_wrappingResources; + static std::unordered_set m_creatingWrappers; + // Table of try-create functions, one per type. static std::vector tryCreateFunctions; }; diff --git a/winrt/lib/utils/ResourceWrapper.h b/winrt/lib/utils/ResourceWrapper.h index 4c956f86c..11e3b56da 100644 --- a/winrt/lib/utils/ResourceWrapper.h +++ b/winrt/lib/utils/ResourceWrapper.h @@ -29,6 +29,8 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas private LifespanTracker { ClosablePtr m_resource; + //Store the identity of the wrapper so we can safely use it in the destructor (ReleaseResource). + IUnknown* m_wrapperIdentity; protected: ResourceWrapper(TResource* resource) @@ -40,7 +42,8 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { if (resource) { - ResourceManager::Add(resource, outerInspectable); + m_wrapperIdentity = AsUnknown(outerInspectable).Get(); + ResourceManager::Add(resource, outerInspectable, m_wrapperIdentity); } } @@ -55,7 +58,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { auto resource = m_resource.Close(); - ResourceManager::Remove(resource.Get()); + ResourceManager::Remove(resource.Get(), m_wrapperIdentity); } } @@ -67,7 +70,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { m_resource = resource; - ResourceManager::Add(resource, GetOuterInspectable()); + ResourceManager::Add(resource, GetOuterInspectable(), m_wrapperIdentity); } } From 9050718cd27937babb91dc57c16986c4f99edacc Mon Sep 17 00:00:00 2001 From: benstevens48 Date: Mon, 6 Jul 2020 18:34:44 +0100 Subject: [PATCH 2/4] prevent unnecessary wrapping of CanvasDrawingSession constructor in D2D lock --- winrt/lib/drawing/CanvasSwapChain.cpp | 17 ++++++++++------- winrt/lib/utils/ResourceManager.cpp | 2 +- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/winrt/lib/drawing/CanvasSwapChain.cpp b/winrt/lib/drawing/CanvasSwapChain.cpp index 944cdf97f..6d278dcec 100644 --- a/winrt/lib/drawing/CanvasSwapChain.cpp +++ b/winrt/lib/drawing/CanvasSwapChain.cpp @@ -681,15 +681,18 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas ThrowHR(E_FAIL, Strings::CannotCreateDrawingSessionUntilPreviousOneClosed); auto d2dDevice = As(device)->GetD2DDevice(); - D2DResourceLock lock(d2dDevice.Get()); ComPtr deviceContext; - auto adapter = CanvasSwapChainDrawingSessionAdapter::Create( - device.Get(), - dxgiSwapChain.Get(), - ToD2DColor(clearColor), - m_dpi, - &deviceContext); + std::shared_ptr adapter; + { + D2DResourceLock lock(d2dDevice.Get()); + adapter = CanvasSwapChainDrawingSessionAdapter::Create( + device.Get(), + dxgiSwapChain.Get(), + ToD2DColor(clearColor), + m_dpi, + &deviceContext); + } auto newDrawingSession = CanvasDrawingSession::CreateNew(deviceContext.Get(), adapter, device.Get(), m_hasActiveDrawingSession); diff --git a/winrt/lib/utils/ResourceManager.cpp b/winrt/lib/utils/ResourceManager.cpp index 50fef95fb..6ab959d6a 100644 --- a/winrt/lib/utils/ResourceManager.cpp +++ b/winrt/lib/utils/ResourceManager.cpp @@ -150,7 +150,7 @@ ComPtr ResourceManager::GetOrCreate(ICanvasDevice* device, IUnknow if (endWrapIt != m_wrappingResources.end()) { m_wrappingResources.erase(endWrapIt); } - }); + }); for (auto& tryCreateFunction : tryCreateFunctions) { From c173089711674c0f35e9008695f122ca4b847e1a Mon Sep 17 00:00:00 2001 From: benstevens48 Date: Fri, 22 Nov 2024 19:30:56 +0000 Subject: [PATCH 3/4] fix resource wrapper --- winrt/lib/drawing/CanvasDevice.cpp | 2 +- winrt/lib/utils/ResourceManager.cpp | 18 +++++++++--------- winrt/lib/utils/ResourceManager.h | 8 ++++---- winrt/lib/utils/ResourceWrapper.h | 9 ++++++--- 4 files changed, 20 insertions(+), 17 deletions(-) diff --git a/winrt/lib/drawing/CanvasDevice.cpp b/winrt/lib/drawing/CanvasDevice.cpp index 953e67a36..34e520674 100644 --- a/winrt/lib/drawing/CanvasDevice.cpp +++ b/winrt/lib/drawing/CanvasDevice.cpp @@ -454,7 +454,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas ThrowHR(E_INVALIDARG); } - wasAdded = ResourceManager::TryRegisterWrapper(resource, wrapper); + wasAdded = ResourceManager::TryRegisterWrapper(resource, wrapper, nullptr); }); if (hresult == S_OK) diff --git a/winrt/lib/utils/ResourceManager.cpp b/winrt/lib/utils/ResourceManager.cpp index adbcc4f8c..33e66bd74 100644 --- a/winrt/lib/utils/ResourceManager.cpp +++ b/winrt/lib/utils/ResourceManager.cpp @@ -104,22 +104,22 @@ std::vector ResourceManager::tryCreateFuncti // Called by the ResourceWrapper constructor, to add itself to the interop mapping table. -void ResourceManager::RegisterWrapper(IUnknown* resource, IInspectable* wrapper) +void ResourceManager::RegisterWrapper(IUnknown* resource, IInspectable* wrapper, IUnknown * wrapperIdentity) { - if (!TryRegisterWrapper(resource, wrapper)) + if (!TryRegisterWrapper(resource, wrapper, wrapperIdentity)) ThrowHR(E_UNEXPECTED); } // Exposed through CanvasDeviceFactory::RegisterWrapper. -bool ResourceManager::TryRegisterWrapper(IUnknown* resource, IInspectable* wrapper) +bool ResourceManager::TryRegisterWrapper(IUnknown* resource, IInspectable* wrapper, IUnknown * wrapperIdentity) { ComPtr resourceIdentity = AsUnknown(resource); std::lock_guard lock(m_mutex); //If this resource is being wrapped by GetOrCreate, then add wrapperIdentity to m_creatingWrappers instead of adding the resource to m_resources. - if (m_wrappingResources.find(resourceIdentity.Get()) != m_wrappingResources.end()) { - m_creatingWrappers.insert(AsUnknown(wrapper).Get()); + if (wrapperIdentity != nullptr && m_wrappingResources.find(resourceIdentity.Get()) != m_wrappingResources.end()) { + m_creatingWrappers.insert(wrapperIdentity); return true; //We don't want any exceptions thrown in this case } @@ -129,21 +129,21 @@ bool ResourceManager::TryRegisterWrapper(IUnknown* resource, IInspectable* wrapp } // Called by ResourceWrapper::Close, to remove itself from the interop mapping table. -void ResourceManager::UnregisterWrapper(IUnknown* resource, IInspectable* wrapper) +void ResourceManager::UnregisterWrapper(IUnknown* resource, IUnknown * wrapperIdentity) { - if (!TryUnregisterWrapper(resource, wrapper)) + if (!TryUnregisterWrapper(resource, wrapperIdentity)) ThrowHR(E_UNEXPECTED); } // Exposed through CanvasDeviceFactory::UnregisterWrapper. -bool ResourceManager::TryUnregisterWrapper(IUnknown* resource, IInspectable* wrapper) +bool ResourceManager::TryUnregisterWrapper(IUnknown* resource, IUnknown * wrapperIdentity) { ComPtr resourceIdentity = AsUnknown(resource); std::lock_guard lock(m_mutex); //If this wrapper is being created by GetOrCreate, remove from m_creatingWrappers intead of removing the resource from m_resources. - if (wrapper != nullptr && m_creatingWrappers.erase(AsUnknown(wrapper).Get()) > 0) { + if (wrapperIdentity != nullptr && m_creatingWrappers.erase(wrapperIdentity) > 0) { return true; //We don't want any exceptions thrown in this case } diff --git a/winrt/lib/utils/ResourceManager.h b/winrt/lib/utils/ResourceManager.h index aaf76dc8f..e85f8b00a 100644 --- a/winrt/lib/utils/ResourceManager.h +++ b/winrt/lib/utils/ResourceManager.h @@ -27,10 +27,10 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { public: // Used by ResourceWrapper to maintain its state in the interop mapping table. - static void RegisterWrapper(IUnknown* resource, IInspectable* wrapper); - static bool TryRegisterWrapper(IUnknown* resource, IInspectable* wrapper); - static void UnregisterWrapper(IUnknown* resource, IInspectable* wrapper); //Note that wrapper can be null if definitely not being created through GetOrCreate - static bool TryUnregisterWrapper(IUnknown* resource, IInspectable* wrapper); //Note that wrapper can be null if definitely not being created through GetOrCreate + static void RegisterWrapper(IUnknown* resource, IInspectable* wrapper, IUnknown * wrapperIdentity); //Note that wrapperIdentity can be null if definitely not being created through GetOrCreate + static bool TryRegisterWrapper(IUnknown* resource, IInspectable* wrapper, IUnknown * wrapperIdentity); //Note that wrapperIdentity can be null if definitely not being created through GetOrCreate + static void UnregisterWrapper(IUnknown* resource, IUnknown * wrapperIdentity); //Note that wrapperIdentity can be null if definitely not being created through GetOrCreate + static bool TryUnregisterWrapper(IUnknown* resource, IUnknown * wrapperIdentity); //Note that wrwrapperIdentityapper can be null if definitely not being created through GetOrCreate static bool RegisterEffectFactory(REFIID effectId, ICanvasEffectFactoryNative* factory); static bool UnregisterEffectFactory(REFIID effectId); diff --git a/winrt/lib/utils/ResourceWrapper.h b/winrt/lib/utils/ResourceWrapper.h index 6c295fc81..730aa8a07 100644 --- a/winrt/lib/utils/ResourceWrapper.h +++ b/winrt/lib/utils/ResourceWrapper.h @@ -29,6 +29,8 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas private LifespanTracker { ClosablePtr m_resource; + //Store the identity of the wrapper so we can safely use it in the destructor (ReleaseResource). + IUnknown* m_wrapperIdentity; protected: ResourceWrapper(TResource* resource) @@ -40,7 +42,8 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { if (resource) { - ResourceManager::RegisterWrapper(resource, outerInspectable); + m_wrapperIdentity = AsUnknown(outerInspectable).Get(); + ResourceManager::RegisterWrapper(resource, outerInspectable, m_wrapperIdentity); } } @@ -55,7 +58,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { auto resource = m_resource.Close(); - ResourceManager::UnregisterWrapper(resource.Get(), GetOuterInspectable()); + ResourceManager::UnregisterWrapper(resource.Get(), m_wrapperIdentity); } } @@ -67,7 +70,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { m_resource = resource; - ResourceManager::RegisterWrapper(resource, GetOuterInspectable()); + ResourceManager::RegisterWrapper(resource, GetOuterInspectable(), m_wrapperIdentity); } } From 34fe032ce5c157fe883895cfafb2d647dfdb8ab0 Mon Sep 17 00:00:00 2001 From: benstevens48 Date: Sat, 23 Nov 2024 12:21:13 +0000 Subject: [PATCH 4/4] update a couple of comments --- winrt/lib/utils/ResourceManager.cpp | 1 + winrt/lib/utils/ResourceManager.h | 10 ++++++---- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/winrt/lib/utils/ResourceManager.cpp b/winrt/lib/utils/ResourceManager.cpp index 33e66bd74..b79250219 100644 --- a/winrt/lib/utils/ResourceManager.cpp +++ b/winrt/lib/utils/ResourceManager.cpp @@ -250,6 +250,7 @@ ComPtr ResourceManager::GetOrCreate(ICanvasDevice* device, IUnknow wrapper = existingWrapper; } else { //Else, remove the wrapper from the m_creatingWrappers set and add the resource to m_resources. + //Note if created by a registered external effect factory, it will not be present in m_creatingWrappers, but that's fine. m_creatingWrappers.erase(AsUnknown(wrapper.Get()).Get()); auto result = m_resources.insert(std::make_pair(resourceIdentity.Get(), AsWeak(wrapper.Get()))); if (!result.second) { diff --git a/winrt/lib/utils/ResourceManager.h b/winrt/lib/utils/ResourceManager.h index e85f8b00a..b8018eb79 100644 --- a/winrt/lib/utils/ResourceManager.h +++ b/winrt/lib/utils/ResourceManager.h @@ -27,10 +27,12 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas { public: // Used by ResourceWrapper to maintain its state in the interop mapping table. - static void RegisterWrapper(IUnknown* resource, IInspectable* wrapper, IUnknown * wrapperIdentity); //Note that wrapperIdentity can be null if definitely not being created through GetOrCreate - static bool TryRegisterWrapper(IUnknown* resource, IInspectable* wrapper, IUnknown * wrapperIdentity); //Note that wrapperIdentity can be null if definitely not being created through GetOrCreate - static void UnregisterWrapper(IUnknown* resource, IUnknown * wrapperIdentity); //Note that wrapperIdentity can be null if definitely not being created through GetOrCreate - static bool TryUnregisterWrapper(IUnknown* resource, IUnknown * wrapperIdentity); //Note that wrwrapperIdentityapper can be null if definitely not being created through GetOrCreate + // Note that wrapperIdentity should be null if directly registering/unregistering an external wrapper (not being created through GetOrCreate), + // otherwise it should be the IUnknown pointer which represents the object's identity in COM. + static void RegisterWrapper(IUnknown* resource, IInspectable* wrapper, IUnknown * wrapperIdentity); + static bool TryRegisterWrapper(IUnknown* resource, IInspectable* wrapper, IUnknown * wrapperIdentity); + static void UnregisterWrapper(IUnknown* resource, IUnknown * wrapperIdentity); + static bool TryUnregisterWrapper(IUnknown* resource, IUnknown * wrapperIdentity); static bool RegisterEffectFactory(REFIID effectId, ICanvasEffectFactoryNative* factory); static bool UnregisterEffectFactory(REFIID effectId);