First attempt to fix deadlock in GetOrCreate

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)

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<ICanvasDeviceInternal>(device)->GetD2DDevice();
-                D2DResourceLock lock(d2dDevice.Get());
 
                 ComPtr<ID2D1DeviceContext1> deviceContext;
-                auto adapter = CanvasSwapChainDrawingSessionAdapter::Create(
-                    device.Get(),
-                    dxgiSwapChain.Get(),
-                    ToD2DColor(clearColor),
-                    m_dpi,
-                    &deviceContext);
+                std::shared_ptr<CanvasSwapChainDrawingSessionAdapter> 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);
 

winrt/lib/utils/ResourceManager.cpp

@@ -56,6 +56,9 @@ std::unordered_map<IUnknown*, WeakRef> ResourceManager::m_resources;
 std::unordered_map<IID, ComPtr<ICanvasEffectFactoryNative>> ResourceManager::m_effectFactories;
 std::recursive_mutex ResourceManager::m_mutex;
 
+std::unordered_multiset<IUnknown*> ResourceManager::m_wrappingResources;
+std::unordered_set<IUnknown*> 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::TryCreateFunction> ResourceManager::tryCreateFunctions =
 {
@@ -101,38 +104,49 @@ std::vector<ResourceManager::TryCreateFunction> 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<IUnknown> resourceIdentity = AsUnknown(resource);
 
     std::lock_guard<std::recursive_mutex> 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<IUnknown> resourceIdentity = AsUnknown(resource);
 
     std::lock_guard<std::recursive_mutex> 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<IInspectable> ResourceManager::GetOrCreate(ICanvasDevice* device, IUnknow
     ComPtr<IUnknown> resourceIdentity = AsUnknown(resource);
     ComPtr<IInspectable> wrapper;
 
-    std::lock_guard<std::recursive_mutex> lock(m_mutex);
+    std::unique_lock<std::recursive_mutex> 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<IInspectable>(it->second);
@@ -196,6 +209,19 @@ ComPtr<IInspectable> 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<std::recursive_mutex> 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<IInspectable> 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<IInspectable> existingWrapper;
+        it = m_resources.find(resourceIdentity.Get());
+        if (it != m_resources.end())
+        {
+            existingWrapper = LockWeakRef<IInspectable>(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<ICanvasEffectFactoryNative> 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<std::recursive_mutex> lock(m_mutex);
+
     auto effectFactory = m_effectFactories.find(effectId);
 
     // Check if we did find a registered effect factory

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 <unordered_set>
 
 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<IID, ComPtr<ICanvasEffectFactoryNative>> 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<IUnknown*> m_wrappingResources;
+        static std::unordered_set<IUnknown*> m_creatingWrappers;
+
         // Table of try-create functions, one per type.
         static std::vector<TryCreateFunction> tryCreateFunctions;
     };

winrt/lib/utils/ResourceWrapper.h

@@ -29,6 +29,8 @@ namespace ABI { namespace Microsoft { namespace Graphics { namespace Canvas
                             private LifespanTracker<TWrapper>
     {
         ClosablePtr<TResource> 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);
             }
         }