diff --git a/winrt/lib/drawing/CanvasDevice.cpp b/winrt/lib/drawing/CanvasDevice.cpp index 0a28b8ede..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) @@ -480,7 +480,7 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas ThrowHR(E_INVALIDARG); } - wasRemoved = ResourceManager::TryUnregisterWrapper(resource); + wasRemoved = ResourceManager::TryUnregisterWrapper(resource, nullptr); }); if (hresult == S_OK) diff --git a/winrt/lib/drawing/CanvasSwapChain.cpp b/winrt/lib/drawing/CanvasSwapChain.cpp index c4f03b555..300973184 100644 --- a/winrt/lib/drawing/CanvasSwapChain.cpp +++ b/winrt/lib/drawing/CanvasSwapChain.cpp @@ -719,15 +719,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 ffa81db60..b79250219 100644 --- a/winrt/lib/utils/ResourceManager.cpp +++ b/winrt/lib/utils/ResourceManager.cpp @@ -56,6 +56,9 @@ std::unordered_map ResourceManager::m_resources; std::unordered_map> ResourceManager::m_effectFactories; 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 = { @@ -101,38 +104,49 @@ 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 (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 + } + auto result = m_resources.insert(std::make_pair(resourceIdentity.Get(), AsWeak(wrapper))); return result.second; } // Called by ResourceWrapper::Close, to remove itself from the interop mapping table. -void ResourceManager::UnregisterWrapper(IUnknown* resource) +void ResourceManager::UnregisterWrapper(IUnknown* resource, IUnknown * wrapperIdentity) { - if (!TryUnregisterWrapper(resource)) + if (!TryUnregisterWrapper(resource, wrapperIdentity)) ThrowHR(E_UNEXPECTED); } // Exposed through CanvasDeviceFactory::UnregisterWrapper. -bool ResourceManager::TryUnregisterWrapper(IUnknown* resource) +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 (wrapperIdentity != nullptr && m_creatingWrappers.erase(wrapperIdentity) > 0) { + return true; //We don't want any exceptions thrown in this case + } + auto result = m_resources.erase(resourceIdentity.Get()); return result == 1; @@ -183,11 +197,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); @@ -196,6 +209,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)) @@ -209,6 +235,31 @@ 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. + //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) { + ThrowHR(E_UNEXPECTED); + } + lock.unlock(); + } + } else { + lock.unlock(); } // Validate that the object we got back reports the expected device and DPI. @@ -325,9 +376,9 @@ void ResourceManager::ValidateDpi(ICanvasResourceWrapperWithDpi* wrapper, float ComPtr ResourceManager::TryGetEffectFactory(REFIID effectId) { - // This lookup doesn't require any locks, as this method is only ever called by CanvasEffect::TryCreateEffect, - // which is retrieved from the create factories declared above and invoked from GetOrCreate, which already - // acquires a lock to access the ResourceManager internal collections before doing so. + + std::lock_guard lock(m_mutex); + auto effectFactory = m_effectFactories.find(effectId); // Check if we did find a registered effect factory diff --git a/winrt/lib/utils/ResourceManager.h b/winrt/lib/utils/ResourceManager.h index 04684096a..b8018eb79 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,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); - static bool TryRegisterWrapper(IUnknown* resource, IInspectable* wrapper); - static void UnregisterWrapper(IUnknown* resource); - static bool TryUnregisterWrapper(IUnknown* resource); + // 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); @@ -167,6 +170,10 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas static std::unordered_map> m_effectFactories; 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 8d87ccc51..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()); + 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); } }