From 57516a60a30c4e0dc0ae04e1ae8785180e26c54b Mon Sep 17 00:00:00 2001 From: Scott Jones Date: Thu, 17 Jun 2021 15:39:57 -0700 Subject: [PATCH 01/18] restore event source caching --- src/Tests/TestComponentCSharp/Singleton.cpp | 43 ++++ src/Tests/TestComponentCSharp/Singleton.h | 19 ++ .../TestComponentCSharp.idl | 13 +- .../TestComponentCSharp.vcxproj | 2 + .../TestComponentCSharp.vcxproj.filters | 2 + .../UnitTest/TestComponentCSharp_Tests.cs | 26 ++- src/WinRT.Runtime/ComWrappersSupport.cs | 2 +- src/cswinrt/code_writers.h | 71 +++--- src/cswinrt/cswinrt.vcxproj | 3 + src/cswinrt/strings/WinRT.cs | 215 ++++++++++++++---- 10 files changed, 313 insertions(+), 83 deletions(-) create mode 100644 src/Tests/TestComponentCSharp/Singleton.cpp create mode 100644 src/Tests/TestComponentCSharp/Singleton.h diff --git a/src/Tests/TestComponentCSharp/Singleton.cpp b/src/Tests/TestComponentCSharp/Singleton.cpp new file mode 100644 index 000000000..541403f6c --- /dev/null +++ b/src/Tests/TestComponentCSharp/Singleton.cpp @@ -0,0 +1,43 @@ +#include "pch.h" +#include "Singleton.h" +#include "Singleton.g.cpp" + +using namespace winrt; +using namespace Windows::Foundation; + +namespace winrt::TestComponentCSharp::implementation +{ + TestComponentCSharp::ISingleton Singleton::Instance() + { + struct singleton : winrt::implements + { + int _int{}; + winrt::event> _intChanged {}; + + int32_t IntProperty() + { + return _int; + } + void IntProperty(int32_t value) + { + _int = value; + _intChanged(nullptr, _int); + } + winrt::event_token IntPropertyChanged(EventHandler const& handler) + { + return _intChanged.add(handler); + } + void IntPropertyChanged(winrt::event_token const& token) noexcept + { + _intChanged.remove(token); + } + }; + static TestComponentCSharp::ISingleton _singleton = winrt::make(); + return _singleton; + } + + void Singleton::Instance(TestComponentCSharp::ISingleton const& value) + { + throw hresult_not_implemented(); + } +} diff --git a/src/Tests/TestComponentCSharp/Singleton.h b/src/Tests/TestComponentCSharp/Singleton.h new file mode 100644 index 000000000..aa02e6e64 --- /dev/null +++ b/src/Tests/TestComponentCSharp/Singleton.h @@ -0,0 +1,19 @@ +#pragma once +#include "Singleton.g.h" + +namespace winrt::TestComponentCSharp::implementation +{ + struct Singleton + { + Singleton() = default; + + static TestComponentCSharp::ISingleton Instance(); + static void Instance(TestComponentCSharp::ISingleton const& value); + }; +} +namespace winrt::TestComponentCSharp::factory_implementation +{ + struct Singleton : SingletonT + { + }; +} diff --git a/src/Tests/TestComponentCSharp/TestComponentCSharp.idl b/src/Tests/TestComponentCSharp/TestComponentCSharp.idl index 061986fc3..7e98fa0c9 100644 --- a/src/Tests/TestComponentCSharp/TestComponentCSharp.idl +++ b/src/Tests/TestComponentCSharp/TestComponentCSharp.idl @@ -130,6 +130,17 @@ namespace TestComponentCSharp static Int32 NumObjects{ get; }; } + interface ISingleton + { + Int32 IntProperty; + event Windows.Foundation.EventHandler IntPropertyChanged; + } + + static runtimeclass Singleton + { + static ISingleton Instance; + } + [default_interface, gc_pressure(Windows.Foundation.Metadata.GCPressureAmount.High)] runtimeclass Class : Windows.Foundation.IStringable @@ -458,7 +469,7 @@ namespace TestComponentCSharp static event Windows.Foundation.EventHandler WarningEvent; } } - + [contract(Windows.Foundation.UniversalApiContract, 8)] interface IWarning1 { diff --git a/src/Tests/TestComponentCSharp/TestComponentCSharp.vcxproj b/src/Tests/TestComponentCSharp/TestComponentCSharp.vcxproj index 01d56f880..56465fd5a 100644 --- a/src/Tests/TestComponentCSharp/TestComponentCSharp.vcxproj +++ b/src/Tests/TestComponentCSharp/TestComponentCSharp.vcxproj @@ -71,6 +71,7 @@ + @@ -86,6 +87,7 @@ + diff --git a/src/Tests/TestComponentCSharp/TestComponentCSharp.vcxproj.filters b/src/Tests/TestComponentCSharp/TestComponentCSharp.vcxproj.filters index 91f509b7c..472076c79 100644 --- a/src/Tests/TestComponentCSharp/TestComponentCSharp.vcxproj.filters +++ b/src/Tests/TestComponentCSharp/TestComponentCSharp.vcxproj.filters @@ -15,6 +15,7 @@ + @@ -24,6 +25,7 @@ + diff --git a/src/Tests/UnitTest/TestComponentCSharp_Tests.cs b/src/Tests/UnitTest/TestComponentCSharp_Tests.cs index 44f2515f1..ae5499e28 100644 --- a/src/Tests/UnitTest/TestComponentCSharp_Tests.cs +++ b/src/Tests/UnitTest/TestComponentCSharp_Tests.cs @@ -2447,15 +2447,14 @@ public void TestCovariance() Assert.True(TestObject.IterableOfObjectIterablesProperty.SequenceEqual(listOfListOfUris)); } + // Ensure that event subscription state is properly cached to enable later unsubscribes [Fact] - public void TestStaticEventWithGC() + public void TestEventSourceCaching() { bool eventCalled = false; - void Class_StaticIntPropertyChanged(object sender, int e) - { - eventCalled = (e == 3); - } + void Class_StaticIntPropertyChanged(object sender, int e) => eventCalled = (e == 3); + // Test static codegen-based EventSource caching Class.StaticIntPropertyChanged += Class_StaticIntPropertyChanged; GC.Collect(2, GCCollectionMode.Forced, true); GC.WaitForPendingFinalizers(); @@ -2467,6 +2466,23 @@ void Class_StaticIntPropertyChanged(object sender, int e) GC.WaitForPendingFinalizers(); Class.StaticIntProperty = 3; Assert.True(eventCalled); + + // Test dynamic WeakRef-based EventSource caching + eventCalled = false; + static void Subscribe(EventHandler handler) => Singleton.Instance.IntPropertyChanged += handler; + static void Unsubscribe(EventHandler handler) => Singleton.Instance.IntPropertyChanged -= handler; + static void Assign(int value) => Singleton.Instance.IntProperty = value; + Subscribe(Class_StaticIntPropertyChanged); + GC.Collect(2, GCCollectionMode.Forced, true); + GC.WaitForPendingFinalizers(); + Unsubscribe(Class_StaticIntPropertyChanged); + Assign(3); + Assert.False(eventCalled); + Subscribe(Class_StaticIntPropertyChanged); + GC.Collect(2, GCCollectionMode.Forced, true); + GC.WaitForPendingFinalizers(); + Assign(3); + Assert.True(eventCalled); } #if NET5_0 diff --git a/src/WinRT.Runtime/ComWrappersSupport.cs b/src/WinRT.Runtime/ComWrappersSupport.cs index c8e705abd..51ab7715a 100644 --- a/src/WinRT.Runtime/ComWrappersSupport.cs +++ b/src/WinRT.Runtime/ComWrappersSupport.cs @@ -60,7 +60,7 @@ public static IObjectReference GetObjectReferenceForInterface(IntPtr externalCom { if (externalComObject == IntPtr.Zero) { - return null; + return null; } using var unknownRef = ObjectReference.FromAbi(externalComObject); diff --git a/src/cswinrt/code_writers.h b/src/cswinrt/code_writers.h index cd940cdf9..74dab499b 100644 --- a/src/cswinrt/code_writers.h +++ b/src/cswinrt/code_writers.h @@ -2528,7 +2528,7 @@ db_path.stem().string()); void write_event_source_generic_args(writer& w, cswinrt::type_semantics eventTypeSemantics); - void write_event_source_ctor(writer& w, Event const& evt) + void write_event_source_ctor(writer& w, Event const& evt, int index) { if (for_typedef(w, get_type_semantics(evt.EventType()), [&](TypeDef const& eventType) { @@ -2537,10 +2537,12 @@ db_path.stem().string()); auto [add, remove] = get_event_methods(evt); w.write(R"( new EventSource__EventHandler%(_obj, %, +%, %))", bind(eventType), get_invoke_info(w, add).first, -get_invoke_info(w, remove).first); +get_invoke_info(w, remove).first, +index); return true; } return false; @@ -2551,13 +2553,15 @@ get_invoke_info(w, remove).first); auto [add, remove] = get_event_methods(evt); w.write(R"( - new %%(_obj, - %, - %))", +new %%(_obj, +%, +%, +%))", bind(get_type_semantics(evt.EventType())), bind(get_type_semantics(evt.EventType())), get_invoke_info(w, add).first, - get_invoke_info(w, remove).first); + get_invoke_info(w, remove).first, + index); } void write_event_sources(writer& w, TypeDef const& type) @@ -3465,26 +3469,37 @@ global::System.Collections.Concurrent.ConcurrentDictionary -{ -% -return %; -}))", - evt.Name(), - bind(init_call_variables), - bind(evt)); + auto event_source = w.write_temp(settings.netstandard_compat ? "_%" : "Get_%()", evt.Name()); w.write(R"( -%event % %% +%%event % %% { add => %.Subscribe(value); remove => %.Unsubscribe(value); } )", + bind([&](writer& w) + { + if(settings.netstandard_compat) + return; + w.write(R"(private EventSource<%> Get_%() +{ +return _%.GetValue((IWinRTObject)this, (key) => +{ +% +return %; +}); +} +)", + bind(get_type_semantics(evt.EventType()), typedef_name_type::Projected, false), + evt.Name(), + evt.Name(), + bind(init_call_variables), + bind(evt, index)); + }), settings.netstandard_compat ? "public " : "", bind(get_type_semantics(evt.EventType()), typedef_name_type::Projected, false), bind([&](writer& w) @@ -3497,6 +3512,7 @@ remove => %.Unsubscribe(value); evt.Name(), event_source, event_source); + index++; } } @@ -4962,11 +4978,14 @@ public static Guid PIID = Vftbl.PIID; type_name, type.TypeName(), type.TypeName(), - bind_each([&](writer& w, Event const& evt) + [&](writer& w) { - w.write("_% = %;\n", evt.Name(), bind(evt)); + int index = 0; + for (auto&& evt : type.EventList()) + { + w.write("_% = %;\n", evt.Name(), bind(evt, index++)); + } }, - type.EventList()), [&](writer& w) { for (auto required_interface : required_interfaces) { @@ -6428,7 +6447,7 @@ bind(type, typedef_name_type::CCW, true) { return; } - for_typedef(w, eventTypeSemantics, [&](TypeDef const& eventType) + for_typedef(w, eventTypeSemantics, [&](TypeDef const&) { std::vector genericArgs; auto arg_count = std::get(eventTypeSemantics).generic_args.size(); @@ -6467,11 +6486,11 @@ internal sealed unsafe class %% : EventSource<%> { private % handler; -internal %(IObjectReference obj, -delegate* unmanaged[Stdcall] addHandler, -delegate* unmanaged[Stdcall] removeHandler) : base(obj, addHandler, removeHandler) -{ -} + internal %(IObjectReference obj, + delegate* unmanaged[Stdcall] addHandler, + delegate* unmanaged[Stdcall] removeHandler, int index) : base(obj, addHandler, removeHandler, index) + { + } protected override IObjectReference CreateMarshaler(% del) => del is null ? null : %.CreateMarshaler(del); diff --git a/src/cswinrt/cswinrt.vcxproj b/src/cswinrt/cswinrt.vcxproj index 9856448e4..674c12582 100644 --- a/src/cswinrt/cswinrt.vcxproj +++ b/src/cswinrt/cswinrt.vcxproj @@ -46,6 +46,9 @@ Console kernel32.lib;user32.lib;%(AdditionalDependencies);windowsapp.lib;advapi32.lib;shlwapi.lib + + true + diff --git a/src/cswinrt/strings/WinRT.cs b/src/cswinrt/strings/WinRT.cs index 9ca4257ae..566b13f2e 100644 --- a/src/cswinrt/strings/WinRT.cs +++ b/src/cswinrt/strings/WinRT.cs @@ -18,7 +18,7 @@ namespace WinRT { - using System.Diagnostics; + using System.Diagnostics; using WinRT.Interop; internal static class DelegateExtensions @@ -142,8 +142,8 @@ public static DllModule Load(string fileName) #if !NETSTANDARD2_0 && !NETCOREAPP2_0 if (_moduleHandle == IntPtr.Zero) { - try - { + try + { // Allow runtime to find module in RID-specific relative subfolder _moduleHandle = NativeLibrary.Load(fileName, Assembly.GetExecutingAssembly(), null); } @@ -244,8 +244,8 @@ public static unsafe (ObjectReference obj, int hr) GetA { m.Dispose(); } - } - + } + ~WinrtModule() { Marshal.ThrowExceptionForHR(Platform.CoDecrementMTAUsage(_mtaCookie)); @@ -338,11 +338,18 @@ internal unsafe abstract class EventSource where TDelegate : class, MulticastDelegate { readonly IObjectReference _obj; + readonly int _index; readonly delegate* unmanaged[Stdcall] _addHandler; readonly delegate* unmanaged[Stdcall] _removeHandler; - private EventRegistrationToken _token; - protected TDelegate _event; + // Registration state, cached separately to survive EventSource garbage collection + protected class State + { + public EventRegistrationToken token; + public TDelegate del; + public System.Delegate eventInvoke; + } + protected State _state; protected abstract IObjectReference CreateMarshaler(TDelegate del); @@ -354,16 +361,16 @@ public void Subscribe(TDelegate del) { lock (this) { - bool registerHandler = _event is null; + bool registerHandler = _state.del is null; - _event = (TDelegate)global::System.Delegate.Combine(_event, del); + _state.del = (TDelegate)global::System.Delegate.Combine(_state.del, del); if (registerHandler) { var marshaler = CreateMarshaler((TDelegate)EventInvoke); try { var nativeDelegate = GetAbi(marshaler); - ExceptionHelpers.ThrowExceptionForHR(_addHandler(_obj.ThisPtr, nativeDelegate, out _token)); + ExceptionHelpers.ThrowExceptionForHR(_addHandler(_obj.ThisPtr, nativeDelegate, out _state.token)); } finally { @@ -379,9 +386,9 @@ public void Unsubscribe(TDelegate del) { lock (this) { - var oldEvent = _event; - _event = (TDelegate)global::System.Delegate.Remove(_event, del); - if (oldEvent is object && _event is null) + var oldEvent = _state.del; + _state.del = (TDelegate)global::System.Delegate.Remove(_state.del, del); + if (oldEvent is object && _state.del is null) { _UnsubscribeFromNative(); } @@ -390,33 +397,141 @@ public void Unsubscribe(TDelegate del) protected abstract System.Delegate EventInvoke { get; } + private class Cache + { + Cache(IWeakReference target, EventSource source, int index) + { + this.target = target; + SetState(source, index); + } + + private IWeakReference target; + private readonly ConcurrentDictionary.State> states = new ConcurrentDictionary.State>(); + + private static readonly ReaderWriterLockSlim cachesLock = new ReaderWriterLockSlim(); + private static readonly ConcurrentDictionary caches = new ConcurrentDictionary(); + + private Cache Update(IWeakReference target, EventSource source, int index) + { + // If target no longer exists, destroy cache + lock (this) + { + using var resolved = this.target.Resolve(typeof(IUnknownVftbl).GUID); + if (resolved == null) + { + this.target = target; + states.Clear(); + } + } + SetState(source, index); + return this; + } + + private void SetState(EventSource source, int index) + { + // If cache exists, use it, else create new + if (states.ContainsKey(index)) + { + source._state = states[index]; + } + else + { + source._state = new EventSource.State(); + states[index] = source._state; + } + } + + public static void Create(IObjectReference obj, EventSource source, int index) + { + // If event source implements weak reference support, track event registrations so that + // unsubscribes will work across garbage collections. Note that most static/factory classes + // do not implement IWeakReferenceSource, so static codegen caching approach is also used. + IWeakReference target = null; + try + { +#if NETSTANDARD2_0 + var weakRefSource = (IWeakReferenceSource)typeof(IWeakReferenceSource).GetHelperType().GetConstructor(new[] { typeof(IObjectReference) }).Invoke(new object[] { obj }); +#else + var weakRefSource = (IWeakReferenceSource)(object)new WinRT.IInspectable(obj); +#endif + target = weakRefSource.GetWeakReference(); + } + catch (Exception) + { + source._state = new EventSource.State(); + return; + } + + cachesLock.EnterReadLock(); + try + { + caches.AddOrUpdate(obj.ThisPtr, + (IntPtr ThisPtr) => new Cache(target, source, index), + (IntPtr ThisPtr, Cache cache) => cache.Update(target, source, index)); + } + finally + { + cachesLock.ExitReadLock(); + } + } + + public static void Remove(IntPtr thisPtr, int index) + { + if (caches.TryGetValue(thisPtr, out var cache)) + { + cache.states.TryRemove(index, out var _); + // using double-checked lock idiom + if (cache.states.IsEmpty) + { + cachesLock.EnterWriteLock(); + try + { + if (cache.states.IsEmpty) + { + caches.TryRemove(thisPtr, out var _); + } + } + finally + { + cachesLock.ExitWriteLock(); + } + } + } + } + } + protected EventSource(IObjectReference obj, delegate* unmanaged[Stdcall] addHandler, - delegate* unmanaged[Stdcall] removeHandler) + delegate* unmanaged[Stdcall] removeHandler, + int index = 0) { + Cache.Create(obj, this, index); _obj = obj; + _index = index; _addHandler = addHandler; _removeHandler = removeHandler; } void _UnsubscribeFromNative() { - ExceptionHelpers.ThrowExceptionForHR(_removeHandler(_obj.ThisPtr, _token)); - _token.Value = 0; + Cache.Remove(_obj.ThisPtr, _index); + ExceptionHelpers.ThrowExceptionForHR(_removeHandler(_obj.ThisPtr, _state.token)); + _state.token.Value = 0; } } - internal sealed unsafe class EventSource__EventHandler : EventSource> - { - private System.EventHandler handler; - - internal EventSource__EventHandler(IObjectReference obj, - delegate* unmanaged[Stdcall] addHandler, - delegate* unmanaged[Stdcall] removeHandler) : base(obj, addHandler, removeHandler) - { - } - - protected override IObjectReference CreateMarshaler(System.EventHandler del) => + internal unsafe class EventSource__EventHandler : EventSource> + { + private System.EventHandler handler; + + internal EventSource__EventHandler(IObjectReference obj, + delegate* unmanaged[Stdcall] addHandler, + delegate* unmanaged[Stdcall] removeHandler, + int index) : base(obj, addHandler, removeHandler, index) + { + } + + protected override IObjectReference CreateMarshaler(System.EventHandler del) => del is null ? null : ABI.System.EventHandler.CreateMarshaler(del); protected override void DisposeMarshaler(IObjectReference marshaler) => @@ -424,24 +539,24 @@ protected override void DisposeMarshaler(IObjectReference marshaler) => protected override IntPtr GetAbi(IObjectReference marshaler) => marshaler is null ? IntPtr.Zero : ABI.System.EventHandler.GetAbi(marshaler); - - protected override System.Delegate EventInvoke - { - // This is synchronized from the base class - get - { - if (handler == null) - { - handler = (System.Object obj, T e) => + + protected override System.Delegate EventInvoke + { + // This is synchronized from the base class + get + { + if (handler == null) + { + handler = (System.Object obj, T e) => { - var localDel = _event; - if (localDel != null) - localDel.Invoke(obj, e); - }; - } - return handler; - } - } + var localDel = _state.del; + if (localDel != null) + localDel.Invoke(obj, e); + }; + } + return handler; + } + } } #pragma warning restore CA2002 @@ -580,13 +695,13 @@ namespace WinRT { using System.Runtime.CompilerServices; internal static class ProjectionInitializer - { + { #pragma warning disable 0436 - [ModuleInitializer] + [ModuleInitializer] #pragma warning restore 0436 - internal static void InitalizeProjection() - { - ComWrappersSupport.RegisterProjectionAssembly(typeof(ProjectionInitializer).Assembly); - } + internal static void InitalizeProjection() + { + ComWrappersSupport.RegisterProjectionAssembly(typeof(ProjectionInitializer).Assembly); + } } } From bbcd64853e60fa20bb00ca720cc8122ff837c1af Mon Sep 17 00:00:00 2001 From: Scott Jones Date: Thu, 17 Jun 2021 15:50:48 -0700 Subject: [PATCH 02/18] fixed bad merge --- src/cswinrt/code_writers.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/cswinrt/code_writers.h b/src/cswinrt/code_writers.h index 74dab499b..89bd0b8f2 100644 --- a/src/cswinrt/code_writers.h +++ b/src/cswinrt/code_writers.h @@ -6509,7 +6509,7 @@ if (handler == null) { handler = (%) => { -var localDel = _event; +var localDel = _state.del; if (localDel == null) {% return %; From ad58eaa453aee797c7b7c4fa4837b11fa23595b3 Mon Sep 17 00:00:00 2001 From: UJJWAL CHADHA Date: Wed, 30 Jun 2021 14:25:36 -0700 Subject: [PATCH 03/18] Made weak references to handlers --- src/cswinrt/code_writers.h | 10 ++++++---- src/cswinrt/strings/WinRT.cs | 7 ++++--- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/src/cswinrt/code_writers.h b/src/cswinrt/code_writers.h index 89bd0b8f2..121403cdc 100644 --- a/src/cswinrt/code_writers.h +++ b/src/cswinrt/code_writers.h @@ -6482,9 +6482,9 @@ bind(type, typedef_name_type::CCW, true) auto eventTypeCode = w.write_temp("%", bind(eventType, typedef_name_type::Projected, false)); auto invokeMethodSig = get_event_invoke_method(eventType); w.write(R"( -internal sealed unsafe class %% : EventSource<%> -{ -private % handler; + internal sealed unsafe class %% : EventSource<%> + { + private System.WeakReference<%> handlerRef = new System.WeakReference<%>(null); internal %(IObjectReference obj, delegate* unmanaged[Stdcall] addHandler, @@ -6505,7 +6505,7 @@ protected override System.Delegate EventInvoke { get { -if (handler == null) +if (!handlerRef.TryGetTarget(out var handler) || handler == null) { handler = (%) => { @@ -6516,6 +6516,7 @@ return %; } %localDel.Invoke(%); }; +handlerRef.SetTarget(handler); } return handler; } @@ -6526,6 +6527,7 @@ return handler; bind(eventTypeSemantics), eventTypeCode, eventTypeCode, + eventTypeCode, bind(eventTypeSemantics), eventTypeCode, abiTypeName, diff --git a/src/cswinrt/strings/WinRT.cs b/src/cswinrt/strings/WinRT.cs index 566b13f2e..9f7278ec3 100644 --- a/src/cswinrt/strings/WinRT.cs +++ b/src/cswinrt/strings/WinRT.cs @@ -347,7 +347,7 @@ protected class State { public EventRegistrationToken token; public TDelegate del; - public System.Delegate eventInvoke; + public System.WeakReference eventInvoke = new System.WeakReference(null); } protected State _state; @@ -522,7 +522,7 @@ void _UnsubscribeFromNative() internal unsafe class EventSource__EventHandler : EventSource> { - private System.EventHandler handler; + private System.WeakReference> handlerRef = new System.WeakReference>(null); internal EventSource__EventHandler(IObjectReference obj, delegate* unmanaged[Stdcall] addHandler, @@ -545,7 +545,7 @@ protected override System.Delegate EventInvoke // This is synchronized from the base class get { - if (handler == null) + if (!handlerRef.TryGetTarget(out var handler) || handler == null) { handler = (System.Object obj, T e) => { @@ -553,6 +553,7 @@ protected override System.Delegate EventInvoke if (localDel != null) localDel.Invoke(obj, e); }; + handlerRef.SetTarget(handler); } return handler; } From 3b3644a19b189e7512a619944d36f031914d5275 Mon Sep 17 00:00:00 2001 From: UJJWAL CHADHA Date: Thu, 1 Jul 2021 16:10:59 -0700 Subject: [PATCH 04/18] Added cache cleaner --- src/cswinrt/code_writers.h | 47 ++++++++++++++--------------- src/cswinrt/strings/WinRT.cs | 58 ++++++++++++++++++++++++++++-------- 2 files changed, 67 insertions(+), 38 deletions(-) diff --git a/src/cswinrt/code_writers.h b/src/cswinrt/code_writers.h index 121403cdc..84cec38e1 100644 --- a/src/cswinrt/code_writers.h +++ b/src/cswinrt/code_writers.h @@ -6484,8 +6484,6 @@ bind(type, typedef_name_type::CCW, true) w.write(R"( internal sealed unsafe class %% : EventSource<%> { - private System.WeakReference<%> handlerRef = new System.WeakReference<%>(null); - internal %(IObjectReference obj, delegate* unmanaged[Stdcall] addHandler, delegate* unmanaged[Stdcall] removeHandler, int index) : base(obj, addHandler, removeHandler, index) @@ -6501,33 +6499,32 @@ protected override void DisposeMarshaler(IObjectReference marshaler) => protected override System.IntPtr GetAbi(IObjectReference marshaler) => marshaler is null ? System.IntPtr.Zero : %.GetAbi(marshaler); -protected override System.Delegate EventInvoke -{ -get -{ -if (!handlerRef.TryGetTarget(out var handler) || handler == null) -{ -handler = (%) => -{ -var localDel = _state.del; -if (localDel == null) -{% -return %; -} -%localDel.Invoke(%); -}; -handlerRef.SetTarget(handler); -} -return handler; -} -} -} + protected override System.Delegate EventInvoke + { + get + { + if (_state.eventInvoke.TryGetTarget(out var cachedInvoke) && cachedInvoke != null) + { + return cachedInvoke; + } + % invoke = (%) => + { + var localDel = _state.del; + if (localDel == null) + {% + return %; + } + %localDel.Invoke(%); + }; + _state.eventInvoke.SetTarget(invoke); + return invoke; + } + } + } )", bind(eventTypeSemantics), bind(eventTypeSemantics), eventTypeCode, - eventTypeCode, - eventTypeCode, bind(eventTypeSemantics), eventTypeCode, abiTypeName, diff --git a/src/cswinrt/strings/WinRT.cs b/src/cswinrt/strings/WinRT.cs index 9f7278ec3..cbc8503e3 100644 --- a/src/cswinrt/strings/WinRT.cs +++ b/src/cswinrt/strings/WinRT.cs @@ -366,11 +366,14 @@ public void Subscribe(TDelegate del) _state.del = (TDelegate)global::System.Delegate.Combine(_state.del, del); if (registerHandler) { - var marshaler = CreateMarshaler((TDelegate)EventInvoke); + var eventInvoke = (TDelegate)EventInvoke; + var marshaler = CreateMarshaler(eventInvoke); try { var nativeDelegate = GetAbi(marshaler); ExceptionHelpers.ThrowExceptionForHR(_addHandler(_obj.ThisPtr, nativeDelegate, out _state.token)); + + Cache.AddStateCleaner(_obj.ThisPtr, eventInvoke, _index); } finally { @@ -399,6 +402,23 @@ public void Unsubscribe(TDelegate del) private class Cache { + private class CacheCleaner + { + private IntPtr objPtr; + private int indexToClean; + + public CacheCleaner(IntPtr objPtr, int indexToClean) + { + this.indexToClean = indexToClean; + this.objPtr = objPtr; + } + + ~CacheCleaner() + { + Cache.Remove(objPtr, indexToClean); + } + } + Cache(IWeakReference target, EventSource source, int index) { this.target = target; @@ -407,9 +427,11 @@ private class Cache private IWeakReference target; private readonly ConcurrentDictionary.State> states = new ConcurrentDictionary.State>(); + private readonly System.Runtime.CompilerServices.ConditionalWeakTable stateCleaner = new System.Runtime.CompilerServices.ConditionalWeakTable(); private static readonly ReaderWriterLockSlim cachesLock = new ReaderWriterLockSlim(); private static readonly ConcurrentDictionary caches = new ConcurrentDictionary(); + private Cache Update(IWeakReference target, EventSource source, int index) { @@ -438,9 +460,18 @@ private void SetState(EventSource source, int index) { source._state = new EventSource.State(); states[index] = source._state; + //eventInvokeToCleanerMap.Add() } } + public static void AddStateCleaner(IntPtr objPtr, Delegate eventInvoke, int index) + { + if (caches.TryGetValue(objPtr, out var cache) && !cache.stateCleaner.TryGetValue(eventInvoke, out var _)) + { + cache.stateCleaner.Add(eventInvoke, new CacheCleaner(objPtr, index)); + } + } + public static void Create(IObjectReference obj, EventSource source, int index) { // If event source implements weak reference support, track event registrations so that @@ -481,7 +512,8 @@ public static void Remove(IntPtr thisPtr, int index) { cache.states.TryRemove(index, out var _); // using double-checked lock idiom - if (cache.states.IsEmpty) + using var resolvedTarget = cache.target.Resolve(typeof(IUnknownVftbl).GUID); + if (cache.states.IsEmpty && resolvedTarget == null) { cachesLock.EnterWriteLock(); try @@ -522,7 +554,6 @@ void _UnsubscribeFromNative() internal unsafe class EventSource__EventHandler : EventSource> { - private System.WeakReference> handlerRef = new System.WeakReference>(null); internal EventSource__EventHandler(IObjectReference obj, delegate* unmanaged[Stdcall] addHandler, @@ -545,17 +576,18 @@ protected override System.Delegate EventInvoke // This is synchronized from the base class get { - if (!handlerRef.TryGetTarget(out var handler) || handler == null) + if (_state.eventInvoke.TryGetTarget(out var handler) && handler != null) { - handler = (System.Object obj, T e) => - { - var localDel = _state.del; - if (localDel != null) - localDel.Invoke(obj, e); - }; - handlerRef.SetTarget(handler); - } - return handler; + return handler; + } + System.EventHandler newHandler = (System.Object obj, T e) => + { + var localDel = _state.del; + if (localDel != null) + localDel.Invoke(obj, e); + }; + _state.eventInvoke.SetTarget(newHandler); + return newHandler; } } } From 3fb27d13ce26928b0e4e0bcc2ac86f351af60b8d Mon Sep 17 00:00:00 2001 From: UJJWAL CHADHA Date: Thu, 1 Jul 2021 16:49:14 -0700 Subject: [PATCH 05/18] Remove state empty condition --- src/cswinrt/strings/WinRT.cs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/src/cswinrt/strings/WinRT.cs b/src/cswinrt/strings/WinRT.cs index cbc8503e3..dda7f54c5 100644 --- a/src/cswinrt/strings/WinRT.cs +++ b/src/cswinrt/strings/WinRT.cs @@ -460,7 +460,6 @@ private void SetState(EventSource source, int index) { source._state = new EventSource.State(); states[index] = source._state; - //eventInvokeToCleanerMap.Add() } } @@ -513,7 +512,7 @@ public static void Remove(IntPtr thisPtr, int index) cache.states.TryRemove(index, out var _); // using double-checked lock idiom using var resolvedTarget = cache.target.Resolve(typeof(IUnknownVftbl).GUID); - if (cache.states.IsEmpty && resolvedTarget == null) + if (resolvedTarget == null) { cachesLock.EnterWriteLock(); try From 51b08d1f5d5a588d5f878a97678b2947e0e7ff71 Mon Sep 17 00:00:00 2001 From: Manodasan Date: Thu, 8 Jul 2021 08:17:50 -0700 Subject: [PATCH 06/18] Fix event source caching. --- src/cswinrt/code_writers.h | 2 +- src/cswinrt/strings/WinRT.cs | 86 +++++++++++++++++++++--------------- 2 files changed, 51 insertions(+), 37 deletions(-) diff --git a/src/cswinrt/code_writers.h b/src/cswinrt/code_writers.h index 84cec38e1..905118184 100644 --- a/src/cswinrt/code_writers.h +++ b/src/cswinrt/code_writers.h @@ -6503,7 +6503,7 @@ marshaler is null ? System.IntPtr.Zero : %.GetAbi(marshaler); { get { - if (_state.eventInvoke.TryGetTarget(out var cachedInvoke) && cachedInvoke != null) + if (_state.eventInvoke.TryGetTarget(out var cachedInvoke)) { return cachedInvoke; } diff --git a/src/cswinrt/strings/WinRT.cs b/src/cswinrt/strings/WinRT.cs index dda7f54c5..aac9b70ff 100644 --- a/src/cswinrt/strings/WinRT.cs +++ b/src/cswinrt/strings/WinRT.cs @@ -1,16 +1,13 @@ using System; -using System.Collections; using System.Collections.Concurrent; using System.Collections.Generic; -using System.Linq; using System.Reflection; -using System.Runtime.ExceptionServices; using System.Runtime.InteropServices; -using System.Numerics; -using System.Security.Cryptography; -using System.Text; using System.Threading; -using System.Linq.Expressions; +using System.Linq.Expressions; +using System.Diagnostics; +using WinRT.Interop; +using System.Runtime.CompilerServices; #pragma warning disable 0169 // The field 'xxx' is never used #pragma warning disable 0649 // Field 'xxx' is never assigned to, and will always have its default value @@ -18,9 +15,6 @@ namespace WinRT { - using System.Diagnostics; - using WinRT.Interop; - internal static class DelegateExtensions { public static void DynamicInvokeAbi(this System.Delegate del, object[] invoke_params) @@ -361,19 +355,23 @@ public void Subscribe(TDelegate del) { lock (this) { - bool registerHandler = _state.del is null; - + bool registerHandler = _state is null || !_state.eventInvoke.TryGetTarget(out var _); + if (registerHandler) + { + Cache.Create(_obj, this, _index); + } + _state.del = (TDelegate)global::System.Delegate.Combine(_state.del, del); if (registerHandler) { - var eventInvoke = (TDelegate)EventInvoke; + var eventInvoke = (TDelegate)EventInvoke; + Cache.AddStateCleaner(_obj.ThisPtr, eventInvoke, _index, _state, this); + var marshaler = CreateMarshaler(eventInvoke); try { var nativeDelegate = GetAbi(marshaler); ExceptionHelpers.ThrowExceptionForHR(_addHandler(_obj.ThisPtr, nativeDelegate, out _state.token)); - - Cache.AddStateCleaner(_obj.ThisPtr, eventInvoke, _index); } finally { @@ -388,7 +386,12 @@ public void Subscribe(TDelegate del) public void Unsubscribe(TDelegate del) { lock (this) - { + { + if (_state is null) + { + Cache.Create(_obj, this, _index); + } + var oldEvent = _state.del; _state.del = (TDelegate)global::System.Delegate.Remove(_state.del, del); if (oldEvent is object && _state.del is null) @@ -404,18 +407,26 @@ private class Cache { private class CacheCleaner { - private IntPtr objPtr; - private int indexToClean; + private readonly IntPtr objPtr; + private readonly int indexToClean; + private readonly State stateToClean; + private readonly System.WeakReference> eventSourceToClean; - public CacheCleaner(IntPtr objPtr, int indexToClean) + public CacheCleaner(IntPtr objPtr, int indexToClean, State stateToClean, EventSource eventSourceToClean) { - this.indexToClean = indexToClean; this.objPtr = objPtr; + this.indexToClean = indexToClean; + this.stateToClean = stateToClean; + this.eventSourceToClean = new System.WeakReference>(eventSourceToClean); } ~CacheCleaner() { - Cache.Remove(objPtr, indexToClean); + Cache.Remove(objPtr, indexToClean, stateToClean); + if (eventSourceToClean.TryGetTarget(out var strongEventSourceToClean)) + { + strongEventSourceToClean._state = null; + } } } @@ -452,7 +463,7 @@ private Cache Update(IWeakReference target, EventSource source, int i private void SetState(EventSource source, int index) { // If cache exists, use it, else create new - if (states.ContainsKey(index)) + if (states.TryGetValue(index, out var state) && state.eventInvoke.TryGetTarget(out _)) { source._state = states[index]; } @@ -463,11 +474,11 @@ private void SetState(EventSource source, int index) } } - public static void AddStateCleaner(IntPtr objPtr, Delegate eventInvoke, int index) + public static void AddStateCleaner(IntPtr objPtr, Delegate eventInvoke, int index, State state, EventSource eventSourceToClean) { if (caches.TryGetValue(objPtr, out var cache) && !cache.stateCleaner.TryGetValue(eventInvoke, out var _)) { - cache.stateCleaner.Add(eventInvoke, new CacheCleaner(objPtr, index)); + cache.stateCleaner.Add(eventInvoke, new CacheCleaner(objPtr, index, state, eventSourceToClean)); } } @@ -504,15 +515,19 @@ public static void Create(IObjectReference obj, EventSource source, i cachesLock.ExitReadLock(); } } - - public static void Remove(IntPtr thisPtr, int index) + + public static void Remove(IntPtr thisPtr, int index, State state) { if (caches.TryGetValue(thisPtr, out var cache)) - { - cache.states.TryRemove(index, out var _); - // using double-checked lock idiom - using var resolvedTarget = cache.target.Resolve(typeof(IUnknownVftbl).GUID); - if (resolvedTarget == null) + { +#if NETSTANDARD2_0 + // https://devblogs.microsoft.com/pfxteam/little-known-gems-atomic-conditional-removals-from-concurrentdictionary/ + ((ICollection>) cache.states).Remove(new KeyValuePair(index, state)); +#else + cache.states.TryRemove(new KeyValuePair(index, state)); +#endif + // using double-checked lock idiom + if (cache.states.IsEmpty) { cachesLock.EnterWriteLock(); try @@ -529,6 +544,7 @@ public static void Remove(IntPtr thisPtr, int index) } } } + } protected EventSource(IObjectReference obj, @@ -536,7 +552,6 @@ protected EventSource(IObjectReference obj, delegate* unmanaged[Stdcall] removeHandler, int index = 0) { - Cache.Create(obj, this, index); _obj = obj; _index = index; _addHandler = addHandler; @@ -545,9 +560,9 @@ protected EventSource(IObjectReference obj, void _UnsubscribeFromNative() { - Cache.Remove(_obj.ThisPtr, _index); + Cache.Remove(_obj.ThisPtr, _index, _state); ExceptionHelpers.ThrowExceptionForHR(_removeHandler(_obj.ThisPtr, _state.token)); - _state.token.Value = 0; + _state = null; } } @@ -575,7 +590,7 @@ protected override System.Delegate EventInvoke // This is synchronized from the base class get { - if (_state.eventInvoke.TryGetTarget(out var handler) && handler != null) + if (_state.eventInvoke.TryGetTarget(out var handler)) { return handler; } @@ -725,7 +740,6 @@ internal class ModuleInitializerAttribute : Attribute { } namespace WinRT { - using System.Runtime.CompilerServices; internal static class ProjectionInitializer { #pragma warning disable 0436 From 9a0d121298aeddcd4f81fdd5cd59002f6d925eb9 Mon Sep 17 00:00:00 2001 From: Manodasan Date: Thu, 8 Jul 2021 14:19:32 -0700 Subject: [PATCH 07/18] Fix merge. --- src/WinRT.Runtime/Projections/EventHandler.cs | 2 +- src/WinRT.Runtime/Projections/INotifyDataErrorInfo.net5.cs | 3 ++- .../Projections/INotifyDataErrorInfo.netstandard2.0.cs | 3 ++- .../Projections/NotifyCollectionChangedEventHandler.cs | 2 +- src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs | 2 +- src/cswinrt/code_writers.h | 1 + 6 files changed, 8 insertions(+), 5 deletions(-) diff --git a/src/WinRT.Runtime/Projections/EventHandler.cs b/src/WinRT.Runtime/Projections/EventHandler.cs index b08914aab..1821ba367 100644 --- a/src/WinRT.Runtime/Projections/EventHandler.cs +++ b/src/WinRT.Runtime/Projections/EventHandler.cs @@ -300,7 +300,7 @@ protected override IntPtr GetAbi(IObjectReference marshaler) => { handler = (global::System.Object obj, global::System.EventArgs e) => { - var localDel = _event; + var localDel = _state.del; if (localDel != null) localDel.Invoke(obj, e); }; diff --git a/src/WinRT.Runtime/Projections/INotifyDataErrorInfo.net5.cs b/src/WinRT.Runtime/Projections/INotifyDataErrorInfo.net5.cs index 9e173a74b..a729b8fa2 100644 --- a/src/WinRT.Runtime/Projections/INotifyDataErrorInfo.net5.cs +++ b/src/WinRT.Runtime/Projections/INotifyDataErrorInfo.net5.cs @@ -129,7 +129,8 @@ private static unsafe int Do_Abi_remove_ErrorsChanged_2(IntPtr thisPtr, global:: return (EventSource__EventHandler)_this.GetOrCreateTypeHelperData(typeof(global::System.Collections.Specialized.INotifyCollectionChanged).TypeHandle, () => new EventSource__EventHandler(_obj, (delegate* unmanaged[Stdcall])(delegate* unmanaged[Stdcall])_obj.Vftbl.add_ErrorsChanged_1, - _obj.Vftbl.remove_ErrorsChanged_2)); + _obj.Vftbl.remove_ErrorsChanged_2, + 0)); } unsafe global::System.Collections.IEnumerable global::System.ComponentModel.INotifyDataErrorInfo.GetErrors(string propertyName) diff --git a/src/WinRT.Runtime/Projections/INotifyDataErrorInfo.netstandard2.0.cs b/src/WinRT.Runtime/Projections/INotifyDataErrorInfo.netstandard2.0.cs index e4b3694fd..1c2adcb6b 100644 --- a/src/WinRT.Runtime/Projections/INotifyDataErrorInfo.netstandard2.0.cs +++ b/src/WinRT.Runtime/Projections/INotifyDataErrorInfo.netstandard2.0.cs @@ -162,7 +162,8 @@ internal INotifyDataErrorInfo(ObjectReference obj) _ErrorsChanged = new EventSource__EventHandler(_obj, (delegate* unmanaged[Stdcall])(delegate* unmanaged[Stdcall])_obj.Vftbl.add_ErrorsChanged_1, - _obj.Vftbl.remove_ErrorsChanged_2); + _obj.Vftbl.remove_ErrorsChanged_2, + 0); } public unsafe global::System.Collections.IEnumerable GetErrors(string propertyName) diff --git a/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs b/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs index ada2a784f..93341e213 100644 --- a/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs +++ b/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs @@ -164,7 +164,7 @@ protected override IntPtr GetAbi(IObjectReference marshaler) => { handler = (global::System.Object obj, global::System.Collections.Specialized.NotifyCollectionChangedEventArgs e) => { - var localDel = _event; + var localDel = _state.del; if (localDel != null) localDel.Invoke(obj, e); }; diff --git a/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs b/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs index 714ace9ae..094a8984d 100644 --- a/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs +++ b/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs @@ -165,7 +165,7 @@ protected override IntPtr GetAbi(IObjectReference marshaler) => { handler = (global::System.Object obj, global::System.ComponentModel.PropertyChangedEventArgs e) => { - var localDel = _event; + var localDel = _state.del; if (localDel != null) localDel.Invoke(obj, e); }; diff --git a/src/cswinrt/code_writers.h b/src/cswinrt/code_writers.h index 905118184..2c40ebec3 100644 --- a/src/cswinrt/code_writers.h +++ b/src/cswinrt/code_writers.h @@ -6530,6 +6530,7 @@ marshaler is null ? System.IntPtr.Zero : %.GetAbi(marshaler); abiTypeName, abiTypeName, abiTypeName, + eventTypeCode, bind(invokeMethodSig), bind(invokeMethodSig), bind(invokeMethodSig), From 4c76b6823711be7d413cf79a815ecdcad1621e43 Mon Sep 17 00:00:00 2001 From: Manodasan Date: Thu, 8 Jul 2021 17:32:43 -0700 Subject: [PATCH 08/18] Fix merge. --- src/WinRT.Runtime/Projections/EventHandler.cs | 23 ++--- .../NotifyCollectionChangedEventHandler.cs | 22 ++--- .../PropertyChangedEventHandler.cs | 22 ++--- src/cswinrt/code_writers.h | 88 +++++++++---------- 4 files changed, 80 insertions(+), 75 deletions(-) diff --git a/src/WinRT.Runtime/Projections/EventHandler.cs b/src/WinRT.Runtime/Projections/EventHandler.cs index 1821ba367..63fe6ff76 100644 --- a/src/WinRT.Runtime/Projections/EventHandler.cs +++ b/src/WinRT.Runtime/Projections/EventHandler.cs @@ -273,8 +273,6 @@ private static unsafe int Do_Abi_Invoke(IntPtr thisPtr, IntPtr sender, IntPtr ar internal sealed unsafe class EventHandlerEventSource : EventSource { - private global::System.EventHandler handler; - internal EventHandlerEventSource(IObjectReference obj, delegate* unmanaged[Stdcall] addHandler, delegate* unmanaged[Stdcall] removeHandler) @@ -296,15 +294,18 @@ protected override IntPtr GetAbi(IObjectReference marshaler) => // This is synchronized from the base class get { - if (handler == null) - { - handler = (global::System.Object obj, global::System.EventArgs e) => - { - var localDel = _state.del; - if (localDel != null) - localDel.Invoke(obj, e); - }; - } + if (_state.eventInvoke.TryGetTarget(out var cachedInvoke)) + { + return cachedInvoke; + } + + global::System.EventHandler handler = (global::System.Object obj, global::System.EventArgs e) => + { + var localDel = _state.del; + if (localDel != null) + localDel.Invoke(obj, e); + }; + _state.eventInvoke.SetTarget(handler); return handler; } } diff --git a/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs b/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs index 93341e213..8a230d264 100644 --- a/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs +++ b/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs @@ -137,8 +137,6 @@ private static unsafe int Do_Abi_Invoke(IntPtr thisPtr, IntPtr sender, IntPtr e) internal sealed unsafe class NotifyCollectionChangedEventSource : EventSource { - private global::System.Collections.Specialized.NotifyCollectionChangedEventHandler handler; - internal NotifyCollectionChangedEventSource(IObjectReference obj, delegate* unmanaged[Stdcall] addHandler, delegate* unmanaged[Stdcall] removeHandler) @@ -159,16 +157,20 @@ protected override IntPtr GetAbi(IObjectReference marshaler) => { // This is synchronized from the base class get - { - if (handler == null) + { + if (_state.eventInvoke.TryGetTarget(out var cachedInvoke)) { - handler = (global::System.Object obj, global::System.Collections.Specialized.NotifyCollectionChangedEventArgs e) => - { - var localDel = _state.del; - if (localDel != null) - localDel.Invoke(obj, e); - }; + return cachedInvoke; } + + global::System.Collections.Specialized.NotifyCollectionChangedEventHandler handler = + (global::System.Object obj, global::System.Collections.Specialized.NotifyCollectionChangedEventArgs e) => + { + var localDel = _state.del; + if (localDel != null) + localDel.Invoke(obj, e); + }; + _state.eventInvoke.SetTarget(handler); return handler; } } diff --git a/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs b/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs index 094a8984d..c7f094671 100644 --- a/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs +++ b/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs @@ -138,8 +138,6 @@ private static unsafe int Do_Abi_Invoke(IntPtr thisPtr, IntPtr sender, IntPtr e) internal sealed unsafe class PropertyChangedEventSource : EventSource { - private global::System.ComponentModel.PropertyChangedEventHandler handler; - internal PropertyChangedEventSource(IObjectReference obj, delegate* unmanaged[Stdcall] addHandler, delegate* unmanaged[Stdcall] removeHandler) @@ -160,16 +158,20 @@ protected override IntPtr GetAbi(IObjectReference marshaler) => { // This is synchronized from the base class get - { - if (handler == null) + { + if (_state.eventInvoke.TryGetTarget(out var cachedInvoke)) { - handler = (global::System.Object obj, global::System.ComponentModel.PropertyChangedEventArgs e) => - { - var localDel = _state.del; - if (localDel != null) - localDel.Invoke(obj, e); - }; + return cachedInvoke; } + + global::System.ComponentModel.PropertyChangedEventHandler handler = + (global::System.Object obj, global::System.ComponentModel.PropertyChangedEventArgs e) => + { + var localDel = _state.del; + if (localDel != null) + localDel.Invoke(obj, e); + }; + _state.eventInvoke.SetTarget(handler); return handler; } } diff --git a/src/cswinrt/code_writers.h b/src/cswinrt/code_writers.h index 2c40ebec3..520874c11 100644 --- a/src/cswinrt/code_writers.h +++ b/src/cswinrt/code_writers.h @@ -6482,13 +6482,13 @@ bind(type, typedef_name_type::CCW, true) auto eventTypeCode = w.write_temp("%", bind(eventType, typedef_name_type::Projected, false)); auto invokeMethodSig = get_event_invoke_method(eventType); w.write(R"( - internal sealed unsafe class %% : EventSource<%> - { - internal %(IObjectReference obj, - delegate* unmanaged[Stdcall] addHandler, - delegate* unmanaged[Stdcall] removeHandler, int index) : base(obj, addHandler, removeHandler, index) - { - } +internal sealed unsafe class %% : EventSource<%> +{ +internal %(IObjectReference obj, +delegate* unmanaged[Stdcall] addHandler, +delegate* unmanaged[Stdcall] removeHandler, int index) : base(obj, addHandler, removeHandler, index) +{ +} protected override IObjectReference CreateMarshaler(% del) => del is null ? null : %.CreateMarshaler(del); @@ -6499,44 +6499,44 @@ protected override void DisposeMarshaler(IObjectReference marshaler) => protected override System.IntPtr GetAbi(IObjectReference marshaler) => marshaler is null ? System.IntPtr.Zero : %.GetAbi(marshaler); - protected override System.Delegate EventInvoke - { - get - { - if (_state.eventInvoke.TryGetTarget(out var cachedInvoke)) - { - return cachedInvoke; - } - % invoke = (%) => - { - var localDel = _state.del; - if (localDel == null) - {% - return %; - } - %localDel.Invoke(%); - }; - _state.eventInvoke.SetTarget(invoke); - return invoke; - } - } - } +protected override System.Delegate EventInvoke +{ +get +{ +if (_state.eventInvoke.TryGetTarget(out var cachedInvoke)) +{ +return cachedInvoke; +} +% invoke = (%) => +{ +var localDel = _state.del; +if (localDel == null) +{% + return %; +} +%localDel.Invoke(%); +}; +_state.eventInvoke.SetTarget(invoke); +return invoke; +} +} +} )", - bind(eventTypeSemantics), - bind(eventTypeSemantics), - eventTypeCode, - bind(eventTypeSemantics), - eventTypeCode, - abiTypeName, - abiTypeName, - abiTypeName, - eventTypeCode, - bind(invokeMethodSig), - bind(invokeMethodSig), - bind(invokeMethodSig), - bind(invokeMethodSig), - bind(invokeMethodSig)); - }); +bind(eventTypeSemantics), +bind(eventTypeSemantics), +eventTypeCode, +bind(eventTypeSemantics), +eventTypeCode, +abiTypeName, +abiTypeName, +abiTypeName, +eventTypeCode, +bind(invokeMethodSig), +bind(invokeMethodSig), +bind(invokeMethodSig), +bind(invokeMethodSig), +bind(invokeMethodSig)); +}); } void write_temp_class_event_source_subclass(writer& w, TypeDef const& classType, concurrency::concurrent_unordered_map& typeNameToDefinitionMap) From cbdcdae981ab752c873a93ba0bab8dbaa7fd0c53 Mon Sep 17 00:00:00 2001 From: Manodasan Date: Thu, 8 Jul 2021 17:41:12 -0700 Subject: [PATCH 09/18] Spacing. --- src/cswinrt/code_writers.h | 2 +- src/cswinrt/strings/WinRT.cs | 1 - 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/cswinrt/code_writers.h b/src/cswinrt/code_writers.h index 520874c11..468eaf446 100644 --- a/src/cswinrt/code_writers.h +++ b/src/cswinrt/code_writers.h @@ -6512,7 +6512,7 @@ return cachedInvoke; var localDel = _state.del; if (localDel == null) {% - return %; +return %; } %localDel.Invoke(%); }; diff --git a/src/cswinrt/strings/WinRT.cs b/src/cswinrt/strings/WinRT.cs index aac9b70ff..dc630b706 100644 --- a/src/cswinrt/strings/WinRT.cs +++ b/src/cswinrt/strings/WinRT.cs @@ -568,7 +568,6 @@ void _UnsubscribeFromNative() internal unsafe class EventSource__EventHandler : EventSource> { - internal EventSource__EventHandler(IObjectReference obj, delegate* unmanaged[Stdcall] addHandler, delegate* unmanaged[Stdcall] removeHandler, From e89710bef5a853371e19fa3deb9ca9189c26a71d Mon Sep 17 00:00:00 2001 From: Manodasan Date: Thu, 8 Jul 2021 18:33:49 -0700 Subject: [PATCH 10/18] reorder --- src/cswinrt/strings/WinRT.cs | 46 ++++++++++++++++++------------------ 1 file changed, 23 insertions(+), 23 deletions(-) diff --git a/src/cswinrt/strings/WinRT.cs b/src/cswinrt/strings/WinRT.cs index dc630b706..68794cd28 100644 --- a/src/cswinrt/strings/WinRT.cs +++ b/src/cswinrt/strings/WinRT.cs @@ -343,13 +343,26 @@ protected class State public TDelegate del; public System.WeakReference eventInvoke = new System.WeakReference(null); } - protected State _state; + protected State _state; + + protected EventSource(IObjectReference obj, + delegate* unmanaged[Stdcall] addHandler, + delegate* unmanaged[Stdcall] removeHandler, + int index = 0) + { + _obj = obj; + _addHandler = addHandler; + _removeHandler = removeHandler; + _index = index; + } protected abstract IObjectReference CreateMarshaler(TDelegate del); protected abstract IntPtr GetAbi(IObjectReference marshaler); - protected abstract void DisposeMarshaler(IObjectReference marshaler); + protected abstract void DisposeMarshaler(IObjectReference marshaler); + + protected abstract System.Delegate EventInvoke { get; } public void Subscribe(TDelegate del) { @@ -396,12 +409,17 @@ public void Unsubscribe(TDelegate del) _state.del = (TDelegate)global::System.Delegate.Remove(_state.del, del); if (oldEvent is object && _state.del is null) { - _UnsubscribeFromNative(); + UnsubscribeFromNative(); } } } - - protected abstract System.Delegate EventInvoke { get; } + + void UnsubscribeFromNative() + { + Cache.Remove(_obj.ThisPtr, _index, _state); + ExceptionHelpers.ThrowExceptionForHR(_removeHandler(_obj.ThisPtr, _state.token)); + _state = null; + } private class Cache { @@ -546,24 +564,6 @@ public static void Remove(IntPtr thisPtr, int index, State state) } } - - protected EventSource(IObjectReference obj, - delegate* unmanaged[Stdcall] addHandler, - delegate* unmanaged[Stdcall] removeHandler, - int index = 0) - { - _obj = obj; - _index = index; - _addHandler = addHandler; - _removeHandler = removeHandler; - } - - void _UnsubscribeFromNative() - { - Cache.Remove(_obj.ThisPtr, _index, _state); - ExceptionHelpers.ThrowExceptionForHR(_removeHandler(_obj.ThisPtr, _state.token)); - _state = null; - } } internal unsafe class EventSource__EventHandler : EventSource> From bc25c95969c420ee8cdb038b84f29b56586acadb Mon Sep 17 00:00:00 2001 From: Manodasan Date: Mon, 12 Jul 2021 00:15:41 -0700 Subject: [PATCH 11/18] Fix issue where registered delegates caused the WinRT object to not get cleaned up. --- .../UnitTest/TestComponentCSharp_Tests.cs | 56 +++- src/WinRT.Runtime/Projections/EventHandler.cs | 19 +- .../NotifyCollectionChangedEventHandler.cs | 41 +-- .../PropertyChangedEventHandler.cs | 41 +-- src/cswinrt/code_writers.h | 17 +- src/cswinrt/strings/WinRT.cs | 284 ++++++++++-------- 6 files changed, 278 insertions(+), 180 deletions(-) diff --git a/src/Tests/UnitTest/TestComponentCSharp_Tests.cs b/src/Tests/UnitTest/TestComponentCSharp_Tests.cs index ae5499e28..0dd71facd 100644 --- a/src/Tests/UnitTest/TestComponentCSharp_Tests.cs +++ b/src/Tests/UnitTest/TestComponentCSharp_Tests.cs @@ -2447,6 +2447,36 @@ public void TestCovariance() Assert.True(TestObject.IterableOfObjectIterablesProperty.SequenceEqual(listOfListOfUris)); } + (System.WeakReference, System.WeakReference) TestEventDelegateCleanup() + { + // Both WinRT object and handler class alive. + var eventCalled = false; + var eventHandlerClass = new EventHandlerClass(() => eventCalled = true); + var classInstance = new Class(); + classInstance.IntPropertyChanged += eventHandlerClass.IntPropertyChanged; + GC.Collect(2, GCCollectionMode.Forced, true); + GC.WaitForPendingFinalizers(); + classInstance.IntProperty = 3; + Assert.True(eventCalled); + + // No strong reference to handler class, but delegate is still registered on + // the WinRT object keeping it alive. + eventCalled = false; + var weakEventHandlerClass = new System.WeakReference(eventHandlerClass); + eventHandlerClass = null; + GC.Collect(2, GCCollectionMode.Forced, true); + GC.WaitForPendingFinalizers(); + classInstance.IntProperty = 3; + Assert.True(eventCalled); + Assert.True(weakEventHandlerClass.TryGetTarget(out var _)); + + // No strong reference to WinRT object. It should no longer be alive + // and should also cause for the event handler class to be no longer alive. + var weakClassInstance = new System.WeakReference(classInstance); + classInstance = null; + return (weakClassInstance, weakEventHandlerClass); + } + // Ensure that event subscription state is properly cached to enable later unsubscribes [Fact] public void TestEventSourceCaching() @@ -2482,7 +2512,31 @@ public void TestEventSourceCaching() GC.Collect(2, GCCollectionMode.Forced, true); GC.WaitForPendingFinalizers(); Assign(3); - Assert.True(eventCalled); + Assert.True(eventCalled); + + // Test that event delegates don't leak when not unsubscribed. + // Test runs into a different function as the finalizer wasn't + // getting triggered otherwise with a weak reference. + (System.WeakReference weakClassInstance, System.WeakReference weakEventHandlerClass) = + TestEventDelegateCleanup(); + GC.Collect(2, GCCollectionMode.Forced, true); + GC.WaitForPendingFinalizers(); + GC.Collect(2, GCCollectionMode.Forced, true); + GC.WaitForPendingFinalizers(); + Assert.False(weakClassInstance.TryGetTarget(out _)); + Assert.False(weakEventHandlerClass.TryGetTarget(out _)); + } + + class EventHandlerClass + { + private readonly Action eventCalled; + + public EventHandlerClass(Action eventCalled) + { + this.eventCalled = eventCalled; + } + + public void IntPropertyChanged(object sender, int e) => eventCalled(); } #if NET5_0 diff --git a/src/WinRT.Runtime/Projections/EventHandler.cs b/src/WinRT.Runtime/Projections/EventHandler.cs index 63fe6ff76..4af7077f2 100644 --- a/src/WinRT.Runtime/Projections/EventHandler.cs +++ b/src/WinRT.Runtime/Projections/EventHandler.cs @@ -289,23 +289,24 @@ protected override void DisposeMarshaler(IObjectReference marshaler) => protected override IntPtr GetAbi(IObjectReference marshaler) => marshaler is null ? IntPtr.Zero : EventHandler.GetAbi(marshaler); - protected override global::System.Delegate EventInvoke + protected override State CreateEventState() => + new EventState(_obj.ThisPtr, _index); + + private sealed class EventState : State { - // This is synchronized from the base class - get + public EventState(IntPtr obj, int index) + : base(obj, index) { - if (_state.eventInvoke.TryGetTarget(out var cachedInvoke)) - { - return cachedInvoke; - } + } + protected override Delegate GetEventInvoke() + { global::System.EventHandler handler = (global::System.Object obj, global::System.EventArgs e) => { - var localDel = _state.del; + var localDel = del; if (localDel != null) localDel.Invoke(obj, e); }; - _state.eventInvoke.SetTarget(handler); return handler; } } diff --git a/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs b/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs index 8a230d264..1b606d0b9 100644 --- a/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs +++ b/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs @@ -151,28 +151,29 @@ protected override void DisposeMarshaler(IObjectReference marshaler) => NotifyCollectionChangedEventHandler.DisposeMarshaler(marshaler); protected override IntPtr GetAbi(IObjectReference marshaler) => - marshaler is null ? IntPtr.Zero : NotifyCollectionChangedEventHandler.GetAbi(marshaler); + marshaler is null ? IntPtr.Zero : NotifyCollectionChangedEventHandler.GetAbi(marshaler); + + protected override State CreateEventState() => + new EventState(_obj.ThisPtr, _index); - protected override global::System.Delegate EventInvoke - { - // This is synchronized from the base class - get + private sealed class EventState : State + { + public EventState(IntPtr obj, int index) + : base(obj, index) { - if (_state.eventInvoke.TryGetTarget(out var cachedInvoke)) - { - return cachedInvoke; - } - - global::System.Collections.Specialized.NotifyCollectionChangedEventHandler handler = - (global::System.Object obj, global::System.Collections.Specialized.NotifyCollectionChangedEventArgs e) => - { - var localDel = _state.del; - if (localDel != null) - localDel.Invoke(obj, e); - }; - _state.eventInvoke.SetTarget(handler); - return handler; - } + } + + protected override Delegate GetEventInvoke() + { + global::System.Collections.Specialized.NotifyCollectionChangedEventHandler handler = + (global::System.Object obj, global::System.Collections.Specialized.NotifyCollectionChangedEventArgs e) => + { + var localDel = del; + if (localDel != null) + localDel.Invoke(obj, e); + }; + return handler; + } } } } diff --git a/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs b/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs index c7f094671..b2c0b098b 100644 --- a/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs +++ b/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs @@ -152,28 +152,29 @@ protected override void DisposeMarshaler(IObjectReference marshaler) => PropertyChangedEventHandler.DisposeMarshaler(marshaler); protected override IntPtr GetAbi(IObjectReference marshaler) => - marshaler is null ? IntPtr.Zero : PropertyChangedEventHandler.GetAbi(marshaler); + marshaler is null ? IntPtr.Zero : PropertyChangedEventHandler.GetAbi(marshaler); + + protected override State CreateEventState() => + new EventState(_obj.ThisPtr, _index); - protected override global::System.Delegate EventInvoke - { - // This is synchronized from the base class - get + private sealed class EventState : State + { + public EventState(IntPtr obj, int index) + : base(obj, index) { - if (_state.eventInvoke.TryGetTarget(out var cachedInvoke)) - { - return cachedInvoke; - } - - global::System.ComponentModel.PropertyChangedEventHandler handler = - (global::System.Object obj, global::System.ComponentModel.PropertyChangedEventArgs e) => - { - var localDel = _state.del; - if (localDel != null) - localDel.Invoke(obj, e); - }; - _state.eventInvoke.SetTarget(handler); - return handler; - } + } + + protected override Delegate GetEventInvoke() + { + global::System.ComponentModel.PropertyChangedEventHandler handler = + (global::System.Object obj, global::System.ComponentModel.PropertyChangedEventArgs e) => + { + var localDel = del; + if (localDel != null) + localDel.Invoke(obj, e); + }; + return handler; + } } } } diff --git a/src/cswinrt/code_writers.h b/src/cswinrt/code_writers.h index 468eaf446..293827a98 100644 --- a/src/cswinrt/code_writers.h +++ b/src/cswinrt/code_writers.h @@ -6499,24 +6499,27 @@ protected override void DisposeMarshaler(IObjectReference marshaler) => protected override System.IntPtr GetAbi(IObjectReference marshaler) => marshaler is null ? System.IntPtr.Zero : %.GetAbi(marshaler); -protected override System.Delegate EventInvoke -{ -get +protected override State CreateEventState() => +new EventState(_obj.ThisPtr, _index); + +private sealed class EventState : State { -if (_state.eventInvoke.TryGetTarget(out var cachedInvoke)) +public EventState(System.IntPtr obj, int index) +: base(obj, index) { -return cachedInvoke; } + +protected override System.Delegate GetEventInvoke() +{ % invoke = (%) => { -var localDel = _state.del; +var localDel = del; if (localDel == null) {% return %; } %localDel.Invoke(%); }; -_state.eventInvoke.SetTarget(invoke); return invoke; } } diff --git a/src/cswinrt/strings/WinRT.cs b/src/cswinrt/strings/WinRT.cs index 68794cd28..9e5c52d29 100644 --- a/src/cswinrt/strings/WinRT.cs +++ b/src/cswinrt/strings/WinRT.cs @@ -331,29 +331,79 @@ public IntPtr ActivateInstance() internal unsafe abstract class EventSource where TDelegate : class, MulticastDelegate { - readonly IObjectReference _obj; - readonly int _index; + protected readonly IObjectReference _obj; + protected readonly int _index; readonly delegate* unmanaged[Stdcall] _addHandler; readonly delegate* unmanaged[Stdcall] _removeHandler; - // Registration state, cached separately to survive EventSource garbage collection - protected class State + // Registration state and delegate cached separately to survive EventSource garbage collection + // and to prevent the generated event delegate from impacting the lifetime of the + // event source. + protected abstract class State : IDisposable { public EventRegistrationToken token; public TDelegate del; - public System.WeakReference eventInvoke = new System.WeakReference(null); + public System.Delegate eventInvoke; + private bool disposedValue; + private readonly IntPtr obj; + private readonly int index; + private readonly System.WeakReference cacheEntry; + + protected State(IntPtr obj, int index) + { + this.obj = obj; + this.index = index; + eventInvoke = GetEventInvoke(); + cacheEntry = new System.WeakReference(this); + } + + // The lifetime of this object is managed by the delegate / eventInvoke + // through its target reference to it. Once the delegate no longer has + // any references, this object will also no longer have any references. + ~State() + { + Dispose(false); + } + + // Allows to retrieve a singleton like weak reference to use + // with the cache to allow for proper removal with comparision. + public System.WeakReference GetWeakReferenceForCache() + { + return cacheEntry; + } + + protected abstract System.Delegate GetEventInvoke(); + + protected virtual void Dispose(bool disposing) + { + // Uses the dispose pattern to ensure we only remove + // from the cache once: either via unsubscribe or via + // the finalizer. + if (!disposedValue) + { + Cache.Remove(obj, index, cacheEntry); + disposedValue = true; + } + } + + public void Dispose() + { + Dispose(true); + GC.SuppressFinalize(this); + } } - protected State _state; + protected System.WeakReference _state; protected EventSource(IObjectReference obj, delegate* unmanaged[Stdcall] addHandler, delegate* unmanaged[Stdcall] removeHandler, int index = 0) { - _obj = obj; + _obj = obj; _addHandler = addHandler; _removeHandler = removeHandler; - _index = index; + _index = index; + _state = Cache.GetState(obj, index); } protected abstract IObjectReference CreateMarshaler(TDelegate del); @@ -362,29 +412,30 @@ protected EventSource(IObjectReference obj, protected abstract void DisposeMarshaler(IObjectReference marshaler); - protected abstract System.Delegate EventInvoke { get; } + protected abstract State CreateEventState(); public void Subscribe(TDelegate del) { lock (this) { - bool registerHandler = _state is null || !_state.eventInvoke.TryGetTarget(out var _); - if (registerHandler) - { - Cache.Create(_obj, this, _index); - } - - _state.del = (TDelegate)global::System.Delegate.Combine(_state.del, del); + State state = null; + bool registerHandler = _state is null || !_state.TryGetTarget(out state); if (registerHandler) { - var eventInvoke = (TDelegate)EventInvoke; - Cache.AddStateCleaner(_obj.ThisPtr, eventInvoke, _index, _state, this); + state = CreateEventState(); + _state = state.GetWeakReferenceForCache(); + Cache.Create(_obj, _index, _state); + } + state.del = (TDelegate)global::System.Delegate.Combine(state.del, del); + if (registerHandler) + { + var eventInvoke = (TDelegate)state.eventInvoke; var marshaler = CreateMarshaler(eventInvoke); try { var nativeDelegate = GetAbi(marshaler); - ExceptionHelpers.ThrowExceptionForHR(_addHandler(_obj.ThisPtr, nativeDelegate, out _state.token)); + ExceptionHelpers.ThrowExceptionForHR(_addHandler(_obj.ThisPtr, nativeDelegate, out state.token)); } finally { @@ -398,71 +449,45 @@ public void Subscribe(TDelegate del) public void Unsubscribe(TDelegate del) { - lock (this) - { - if (_state is null) - { - Cache.Create(_obj, this, _index); - } + if (_state is null || !_state.TryGetTarget(out var state)) + { + return; + } - var oldEvent = _state.del; - _state.del = (TDelegate)global::System.Delegate.Remove(_state.del, del); - if (oldEvent is object && _state.del is null) + lock (this) + { + var oldEvent = state.del; + state.del = (TDelegate)global::System.Delegate.Remove(state.del, del); + if (oldEvent is object && state.del is null) { - UnsubscribeFromNative(); + UnsubscribeFromNative(state); } } - } + } - void UnsubscribeFromNative() + void UnsubscribeFromNative(State state) { - Cache.Remove(_obj.ThisPtr, _index, _state); - ExceptionHelpers.ThrowExceptionForHR(_removeHandler(_obj.ThisPtr, _state.token)); + ExceptionHelpers.ThrowExceptionForHR(_removeHandler(_obj.ThisPtr, state.token)); + state.Dispose(); _state = null; - } + } private class Cache { - private class CacheCleaner - { - private readonly IntPtr objPtr; - private readonly int indexToClean; - private readonly State stateToClean; - private readonly System.WeakReference> eventSourceToClean; - - public CacheCleaner(IntPtr objPtr, int indexToClean, State stateToClean, EventSource eventSourceToClean) - { - this.objPtr = objPtr; - this.indexToClean = indexToClean; - this.stateToClean = stateToClean; - this.eventSourceToClean = new System.WeakReference>(eventSourceToClean); - } - - ~CacheCleaner() - { - Cache.Remove(objPtr, indexToClean, stateToClean); - if (eventSourceToClean.TryGetTarget(out var strongEventSourceToClean)) - { - strongEventSourceToClean._state = null; - } - } - } - - Cache(IWeakReference target, EventSource source, int index) + Cache(IWeakReference target, int index, System.WeakReference state) { this.target = target; - SetState(source, index); + SetState(index, state); } private IWeakReference target; - private readonly ConcurrentDictionary.State> states = new ConcurrentDictionary.State>(); - private readonly System.Runtime.CompilerServices.ConditionalWeakTable stateCleaner = new System.Runtime.CompilerServices.ConditionalWeakTable(); + private readonly ConcurrentDictionary> states = new ConcurrentDictionary>(); private static readonly ReaderWriterLockSlim cachesLock = new ReaderWriterLockSlim(); - private static readonly ConcurrentDictionary caches = new ConcurrentDictionary(); - - - private Cache Update(IWeakReference target, EventSource source, int index) + private static readonly ConcurrentDictionary caches = new ConcurrentDictionary(); + + + private Cache Update(IWeakReference target, int index, System.WeakReference state) { // If target no longer exists, destroy cache lock (this) @@ -474,50 +499,51 @@ private Cache Update(IWeakReference target, EventSource source, int i states.Clear(); } } - SetState(source, index); + SetState(index, state); return this; } - private void SetState(EventSource source, int index) + private System.WeakReference GetState(int index) { - // If cache exists, use it, else create new - if (states.TryGetValue(index, out var state) && state.eventInvoke.TryGetTarget(out _)) + // If target no longer exists, destroy cache + lock (this) { - source._state = states[index]; + using var resolved = this.target.Resolve(typeof(IUnknownVftbl).GUID); + if (resolved == null) + { + return null; + } } - else + + if (states.TryGetValue(index, out var weakState)) { - source._state = new EventSource.State(); - states[index] = source._state; + return weakState; } + return null; } - public static void AddStateCleaner(IntPtr objPtr, Delegate eventInvoke, int index, State state, EventSource eventSourceToClean) - { - if (caches.TryGetValue(objPtr, out var cache) && !cache.stateCleaner.TryGetValue(eventInvoke, out var _)) - { - cache.stateCleaner.Add(eventInvoke, new CacheCleaner(objPtr, index, state, eventSourceToClean)); - } + private void SetState(int index, System.WeakReference state) + { + states[index] = state; } - public static void Create(IObjectReference obj, EventSource source, int index) + public static void Create(IObjectReference obj, int index, System.WeakReference state) { // If event source implements weak reference support, track event registrations so that // unsubscribes will work across garbage collections. Note that most static/factory classes // do not implement IWeakReferenceSource, so static codegen caching approach is also used. IWeakReference target = null; try - { + { #if NETSTANDARD2_0 - var weakRefSource = (IWeakReferenceSource)typeof(IWeakReferenceSource).GetHelperType().GetConstructor(new[] { typeof(IObjectReference) }).Invoke(new object[] { obj }); + var weakRefSource = (IWeakReferenceSource)typeof(IWeakReferenceSource).GetHelperType().GetConstructor(new[] { typeof(IObjectReference) }).Invoke(new object[] { obj }); #else var weakRefSource = (IWeakReferenceSource)(object)new WinRT.IInspectable(obj); #endif - target = weakRefSource.GetWeakReference(); + target = weakRefSource.GetWeakReference(); } catch (Exception) { - source._state = new EventSource.State(); return; } @@ -525,48 +551,58 @@ public static void Create(IObjectReference obj, EventSource source, i try { caches.AddOrUpdate(obj.ThisPtr, - (IntPtr ThisPtr) => new Cache(target, source, index), - (IntPtr ThisPtr, Cache cache) => cache.Update(target, source, index)); + (IntPtr ThisPtr) => new Cache(target, index, state), + (IntPtr ThisPtr, Cache cache) => cache.Update(target, index, state)); } finally { cachesLock.ExitReadLock(); } } - - public static void Remove(IntPtr thisPtr, int index, State state) + + public static System.WeakReference GetState(IObjectReference obj, int index) + { + if (caches.TryGetValue(obj.ThisPtr, out var cache)) + { + return cache.GetState(index); + } + + return null; + } + + public static void Remove(IntPtr thisPtr, int index, System.WeakReference state) { if (caches.TryGetValue(thisPtr, out var cache)) { #if NETSTANDARD2_0 - // https://devblogs.microsoft.com/pfxteam/little-known-gems-atomic-conditional-removals-from-concurrentdictionary/ - ((ICollection>) cache.states).Remove(new KeyValuePair(index, state)); + // https://devblogs.microsoft.com/pfxteam/little-known-gems-atomic-conditional-removals-from-concurrentdictionary/ + ((ICollection>>)cache.states).Remove( + new KeyValuePair>(index, state)); #else - cache.states.TryRemove(new KeyValuePair(index, state)); + cache.states.TryRemove(new KeyValuePair>(index, state)); #endif // using double-checked lock idiom if (cache.states.IsEmpty) - { - cachesLock.EnterWriteLock(); - try - { - if (cache.states.IsEmpty) - { - caches.TryRemove(thisPtr, out var _); - } - } - finally - { - cachesLock.ExitWriteLock(); - } - } + { + cachesLock.EnterWriteLock(); + try + { + if (cache.states.IsEmpty) + { + caches.TryRemove(thisPtr, out var _); + } + } + finally + { + cachesLock.ExitWriteLock(); + } + } } } - } } - internal unsafe class EventSource__EventHandler : EventSource> + internal unsafe sealed class EventSource__EventHandler : EventSource> { internal EventSource__EventHandler(IObjectReference obj, delegate* unmanaged[Stdcall] addHandler, @@ -584,24 +620,26 @@ protected override void DisposeMarshaler(IObjectReference marshaler) => protected override IntPtr GetAbi(IObjectReference marshaler) => marshaler is null ? IntPtr.Zero : ABI.System.EventHandler.GetAbi(marshaler); - protected override System.Delegate EventInvoke - { - // This is synchronized from the base class - get - { - if (_state.eventInvoke.TryGetTarget(out var handler)) - { - return handler; - } - System.EventHandler newHandler = (System.Object obj, T e) => + protected override State CreateEventState() => + new EventState(_obj.ThisPtr, _index); + + private sealed class EventState : State + { + public EventState(IntPtr obj, int index) + : base(obj, index) + { + } + + protected override Delegate GetEventInvoke() + { + System.EventHandler handler = (System.Object obj, T e) => { - var localDel = _state.del; + var localDel = del; if (localDel != null) localDel.Invoke(obj, e); }; - _state.eventInvoke.SetTarget(newHandler); - return newHandler; - } + return handler; + } } } From a0a0ba50eada3b9a35c2583f60cc00be2b7871fb Mon Sep 17 00:00:00 2001 From: Manodasan Wignarajah Date: Wed, 2 Jun 2021 22:06:44 -0700 Subject: [PATCH 12/18] Update delegates to take advantage of agile changes in IObjectReference. --- src/WinRT.Runtime/Projections/EventHandler.cs | 33 +++------------- .../NotifyCollectionChangedEventHandler.cs | 34 +++-------------- .../PropertyChangedEventHandler.cs | 32 ++-------------- src/cswinrt/code_writers.h | 38 +++---------------- 4 files changed, 20 insertions(+), 117 deletions(-) diff --git a/src/WinRT.Runtime/Projections/EventHandler.cs b/src/WinRT.Runtime/Projections/EventHandler.cs index 4af7077f2..b54d2b85e 100644 --- a/src/WinRT.Runtime/Projections/EventHandler.cs +++ b/src/WinRT.Runtime/Projections/EventHandler.cs @@ -43,8 +43,8 @@ public static IntPtr GetAbi(IObjectReference value) => value is null ? IntPtr.Zero : MarshalInterfaceHelper>.GetAbi(value); public static unsafe global::System.EventHandler FromAbi(IntPtr nativeDelegate) - { - var abiDelegate = ObjectReference.FromAbi(nativeDelegate); + { + var abiDelegate = ComWrappersSupport.GetObjectReferenceForInterface(nativeDelegate).As(GuidGenerator.GetIID(typeof(EventHandler))); return (global::System.EventHandler)ComWrappersSupport.TryRegisterObjectForInterface(new global::System.EventHandler(new NativeDelegateWrapper(abiDelegate).Invoke), nativeDelegate); } @@ -56,25 +56,10 @@ private class NativeDelegateWrapper : IWinRTObject #endif { private readonly ObjectReference _nativeDelegate; -#if NETSTANDARD2_0 - private readonly AgileReference _agileReference = default; -#endif + public NativeDelegateWrapper(ObjectReference nativeDelegate) { _nativeDelegate = nativeDelegate; - if (_nativeDelegate.TryAs(out var objRef) < 0) - { - var agileReference = new AgileReference(_nativeDelegate); -#if NETSTANDARD2_0 - _agileReference = agileReference; -#else - ((IWinRTObject)this).AdditionalTypeData.TryAdd(typeof(AgileReference).TypeHandle, agileReference); -#endif - } - else - { - objRef.Dispose(); - } } #if !NETSTANDARD2_0 @@ -86,16 +71,8 @@ public NativeDelegateWrapper(ObjectReference(GuidGenerator.GetIID(typeof(EventHandler))); - var delegateToInvoke = agileDelegate ?? _nativeDelegate; - IntPtr ThisPtr = delegateToInvoke.ThisPtr; - var abiInvoke = Marshal.GetDelegateForFunctionPointer(delegateToInvoke.Vftbl.Invoke, Abi_Invoke_Type); + IntPtr ThisPtr = _nativeDelegate.ThisPtr; + var abiInvoke = Marshal.GetDelegateForFunctionPointer(_nativeDelegate.Vftbl.Invoke, Abi_Invoke_Type); IObjectReference __sender = default; object __args = default; var __params = new object[] { ThisPtr, null, null }; diff --git a/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs b/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs index 1b606d0b9..7eb1095f5 100644 --- a/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs +++ b/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs @@ -39,8 +39,8 @@ public static unsafe IObjectReference CreateMarshaler(global::System.Collections public static IntPtr GetAbi(IObjectReference value) => MarshalInterfaceHelper.GetAbi(value); public static unsafe global::System.Collections.Specialized.NotifyCollectionChangedEventHandler FromAbi(IntPtr nativeDelegate) - { - var abiDelegate = ObjectReference.FromAbi(nativeDelegate); + { + var abiDelegate = ComWrappersSupport.GetObjectReferenceForInterface(nativeDelegate).As(GuidGenerator.GetIID(typeof(NotifyCollectionChangedEventHandler))); return abiDelegate is null ? null : (global::System.Collections.Specialized.NotifyCollectionChangedEventHandler)ComWrappersSupport.TryRegisterObjectForInterface(new global::System.Collections.Specialized.NotifyCollectionChangedEventHandler(new NativeDelegateWrapper(abiDelegate).Invoke), nativeDelegate); } @@ -52,26 +52,10 @@ private class NativeDelegateWrapper : IWinRTObject #endif { private readonly ObjectReference _nativeDelegate; -#if NETSTANDARD2_0 - private readonly AgileReference _agileReference = default; -#endif public NativeDelegateWrapper(ObjectReference nativeDelegate) { _nativeDelegate = nativeDelegate; - if (_nativeDelegate.TryAs(out var objRef) < 0) - { - var agileReference = new AgileReference(_nativeDelegate); -#if NETSTANDARD2_0 - _agileReference = agileReference; -#else - ((IWinRTObject)this).AdditionalTypeData.TryAdd(typeof(AgileReference).TypeHandle, agileReference); -#endif - } - else - { - objRef.Dispose(); - } } #if !NETSTANDARD2_0 @@ -82,17 +66,9 @@ public NativeDelegateWrapper(ObjectReference(GuidGenerator.GetIID(typeof(NotifyCollectionChangedEventHandler))); - var delegateToInvoke = agileDelegate ?? _nativeDelegate; - IntPtr ThisPtr = delegateToInvoke.ThisPtr; - var abiInvoke = Marshal.GetDelegateForFunctionPointer(delegateToInvoke.Vftbl.Invoke); + { + IntPtr ThisPtr = _nativeDelegate.ThisPtr; + var abiInvoke = Marshal.GetDelegateForFunctionPointer(_nativeDelegate.Vftbl.Invoke); IObjectReference __sender = default; IObjectReference __e = default; try diff --git a/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs b/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs index b2c0b098b..93ea48287 100644 --- a/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs +++ b/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs @@ -38,8 +38,8 @@ public static unsafe IObjectReference CreateMarshaler(global::System.ComponentMo public static IntPtr GetAbi(IObjectReference value) => MarshalInterfaceHelper.GetAbi(value); public static unsafe global::System.ComponentModel.PropertyChangedEventHandler FromAbi(IntPtr nativeDelegate) - { - var abiDelegate = ObjectReference.FromAbi(nativeDelegate); + { + var abiDelegate = ComWrappersSupport.GetObjectReferenceForInterface(nativeDelegate).As(GuidGenerator.GetIID(typeof(PropertyChangedEventHandler))); return abiDelegate is null ? null : (global::System.ComponentModel.PropertyChangedEventHandler)ComWrappersSupport.TryRegisterObjectForInterface(new global::System.ComponentModel.PropertyChangedEventHandler(new NativeDelegateWrapper(abiDelegate).Invoke), nativeDelegate); } @@ -51,26 +51,10 @@ private class NativeDelegateWrapper : IWinRTObject #endif { private readonly ObjectReference _nativeDelegate; -#if NETSTANDARD2_0 - private readonly AgileReference _agileReference = default; -#endif public NativeDelegateWrapper(ObjectReference nativeDelegate) { _nativeDelegate = nativeDelegate; - if (_nativeDelegate.TryAs(out var objRef) < 0) - { - var agileReference = new AgileReference(_nativeDelegate); -#if NETSTANDARD2_0 - _agileReference = agileReference; -#else - ((IWinRTObject)this).AdditionalTypeData.TryAdd(typeof(AgileReference).TypeHandle, agileReference); -#endif - } - else - { - objRef.Dispose(); - } } #if !NETSTANDARD2_0 @@ -82,16 +66,8 @@ public NativeDelegateWrapper(ObjectReference(GuidGenerator.GetIID(typeof(PropertyChangedEventHandler))); - var delegateToInvoke = agileDelegate ?? _nativeDelegate; - IntPtr ThisPtr = delegateToInvoke.ThisPtr; - var abiInvoke = Marshal.GetDelegateForFunctionPointer(delegateToInvoke.Vftbl.Invoke); + IntPtr ThisPtr = _nativeDelegate.ThisPtr; + var abiInvoke = Marshal.GetDelegateForFunctionPointer(_nativeDelegate.Vftbl.Invoke); IObjectReference __sender = default; IObjectReference __e = default; try diff --git a/src/cswinrt/code_writers.h b/src/cswinrt/code_writers.h index 293827a98..3dacb2b2d 100644 --- a/src/cswinrt/code_writers.h +++ b/src/cswinrt/code_writers.h @@ -5665,7 +5665,7 @@ public static IntPtr GetAbi(IObjectReference value) => MarshalInterfaceHelper<%> public static unsafe % FromAbi(IntPtr nativeDelegate) { -var abiDelegate = ObjectReference.FromAbi(nativeDelegate); +var abiDelegate = ComWrappersSupport.GetObjectReferenceForInterface(nativeDelegate).As(GuidGenerator.GetIID(typeof(@%))); return abiDelegate is null ? null : (%)ComWrappersSupport.TryRegisterObjectForInterface(new %(new NativeDelegateWrapper(abiDelegate).Invoke), nativeDelegate); } @@ -5677,29 +5677,10 @@ private class NativeDelegateWrapper : IWinRTObject #endif { private readonly ObjectReference _nativeDelegate; -#if NETSTANDARD2_0 -private readonly AgileReference _agileReference = default; -#endif public NativeDelegateWrapper(ObjectReference nativeDelegate) { _nativeDelegate = nativeDelegate; -#if NETSTANDARD2_0 -if (_nativeDelegate.TryAs(out var objRef) < 0) -{ -_agileReference = new AgileReference(_nativeDelegate); -} -#else -if (_nativeDelegate.TryAs(IAgileObject.IID, out var objRef) < 0) -{ -var agileReference = new AgileReference(_nativeDelegate); -((IWinRTObject)this).AdditionalTypeData.TryAdd(typeof(AgileReference).TypeHandle, agileReference); -} -#endif -else -{ -objRef.Dispose(); -} } #if !NETSTANDARD2_0 @@ -5711,14 +5692,7 @@ global::System.Collections.Concurrent.ConcurrentDictionary(GuidGenerator.GetIID(typeof(@%))); -var delegateToInvoke = agileDelegate ?? _nativeDelegate; -IntPtr ThisPtr = delegateToInvoke.ThisPtr; +IntPtr ThisPtr = _nativeDelegate.ThisPtr; %% } } @@ -5813,24 +5787,24 @@ public static Guid PIID = GuidGenerator.CreateIID(typeof(%));)", type_name, // FromAbi type_name, + type.TypeName(), + type_params, type_name, type_name, // NativeDelegateWrapper.Invoke bind(signature), bind_list(", ", signature.params()), - type.TypeName(), - type_params, bind([&](writer& w) { if (is_generic || settings.netstandard_compat) { - w.write("var abiInvoke = Marshal.GetDelegateForFunctionPointer%(delegateToInvoke.Vftbl.Invoke%);", + w.write("var abiInvoke = Marshal.GetDelegateForFunctionPointer%(_nativeDelegate.Vftbl.Invoke%);", is_generic ? "" : "", is_generic ? ", Abi_Invoke_Type" : ""); } else { - w.write("var abiInvoke = (delegate* unmanaged[Stdcall]<%, int>)(delegateToInvoke.Vftbl.Invoke);", + w.write("var abiInvoke = (delegate* unmanaged[Stdcall]<%, int>)(_nativeDelegate.Vftbl.Invoke);", bind(signature)); } }), From c48ba7c27c6985338a9fb7dd7b39973f26ff9763 Mon Sep 17 00:00:00 2001 From: Manodasan Date: Tue, 13 Jul 2021 06:59:39 -0700 Subject: [PATCH 13/18] Fix merge --- src/WinRT.Runtime/Projections/EventHandler.cs | 33 +++---------------- 1 file changed, 5 insertions(+), 28 deletions(-) diff --git a/src/WinRT.Runtime/Projections/EventHandler.cs b/src/WinRT.Runtime/Projections/EventHandler.cs index b54d2b85e..7a548f21b 100644 --- a/src/WinRT.Runtime/Projections/EventHandler.cs +++ b/src/WinRT.Runtime/Projections/EventHandler.cs @@ -148,8 +148,8 @@ public static IntPtr GetAbi(IObjectReference value) => value is null ? IntPtr.Zero : MarshalInterfaceHelper>.GetAbi(value); public static unsafe global::System.EventHandler FromAbi(IntPtr nativeDelegate) - { - var abiDelegate = ObjectReference.FromAbi(nativeDelegate); + { + var abiDelegate = ComWrappersSupport.GetObjectReferenceForInterface(nativeDelegate).As(GuidGenerator.GetIID(typeof(EventHandler))); return (global::System.EventHandler)ComWrappersSupport.TryRegisterObjectForInterface(new global::System.EventHandler(new NativeDelegateWrapper(abiDelegate).Invoke), nativeDelegate); } @@ -161,25 +161,10 @@ private class NativeDelegateWrapper : IWinRTObject #endif { private readonly ObjectReference _nativeDelegate; -#if NETSTANDARD2_0 - private readonly AgileReference _agileReference = default; -#endif + public NativeDelegateWrapper(ObjectReference nativeDelegate) { _nativeDelegate = nativeDelegate; - if (_nativeDelegate.TryAs(out var objRef) < 0) - { - var agileReference = new AgileReference(_nativeDelegate); -#if NETSTANDARD2_0 - _agileReference = agileReference; -#else - ((IWinRTObject)this).AdditionalTypeData.TryAdd(typeof(AgileReference).TypeHandle, agileReference); -#endif - } - else - { - objRef.Dispose(); - } } #if !NETSTANDARD2_0 @@ -191,16 +176,8 @@ public NativeDelegateWrapper(ObjectReference(GuidGenerator.GetIID(typeof(EventHandler))); - var delegateToInvoke = agileDelegate ?? _nativeDelegate; - IntPtr ThisPtr = delegateToInvoke.ThisPtr; - var abiInvoke = Marshal.GetDelegateForFunctionPointer(delegateToInvoke.Vftbl.Invoke); + IntPtr ThisPtr = _nativeDelegate.ThisPtr; + var abiInvoke = Marshal.GetDelegateForFunctionPointer(_nativeDelegate.Vftbl.Invoke); IObjectReference __sender = default; IObjectReference __args = default; var __params = new object[] { ThisPtr, null, null }; From efb3f1c980e03a5a805b7683620166e74715f551 Mon Sep 17 00:00:00 2001 From: Manodasan Date: Tue, 13 Jul 2021 08:43:21 -0700 Subject: [PATCH 14/18] Make all event ConditionalWeakTables Lazy to reduce finalizer load. --- src/WinRT.Runtime/Projections/ICommand.net5.cs | 4 ++-- .../Projections/ICommand.netstandard2.0.cs | 4 ++-- .../Projections/INotifyCollectionChanged.net5.cs | 5 +++-- .../INotifyCollectionChanged.netstandard2.0.cs | 3 ++- .../Projections/INotifyDataErrorInfo.net5.cs | 3 ++- .../INotifyDataErrorInfo.netstandard2.0.cs | 3 ++- .../Projections/INotifyPropertyChanged.net5.cs | 3 ++- .../INotifyPropertyChanged.netstandard2.0.cs | 3 ++- src/cswinrt/code_writers.h | 16 +++++++++++++--- 9 files changed, 30 insertions(+), 14 deletions(-) diff --git a/src/WinRT.Runtime/Projections/ICommand.net5.cs b/src/WinRT.Runtime/Projections/ICommand.net5.cs index 5b7f4be17..c5623e158 100644 --- a/src/WinRT.Runtime/Projections/ICommand.net5.cs +++ b/src/WinRT.Runtime/Projections/ICommand.net5.cs @@ -90,8 +90,8 @@ private static unsafe int Do_Abi_Execute_3(IntPtr thisPtr, IntPtr parameter) return 0; } - private static ConditionalWeakTable> _CanExecuteChanged_TokenTables = new ConditionalWeakTable>(); - + private readonly static Lazy>> _CanExecuteChanged_TokenTablesLazy = new(); + private static ConditionalWeakTable> _CanExecuteChanged_TokenTables => _CanExecuteChanged_TokenTablesLazy.Value; [UnmanagedCallersOnly] diff --git a/src/WinRT.Runtime/Projections/ICommand.netstandard2.0.cs b/src/WinRT.Runtime/Projections/ICommand.netstandard2.0.cs index 89a47e6a2..f071420ef 100644 --- a/src/WinRT.Runtime/Projections/ICommand.netstandard2.0.cs +++ b/src/WinRT.Runtime/Projections/ICommand.netstandard2.0.cs @@ -85,8 +85,8 @@ private static unsafe int Do_Abi_Execute_3(IntPtr thisPtr, IntPtr parameter) return 0; } - private static ConditionalWeakTable> _CanExecuteChanged_TokenTables = new ConditionalWeakTable>(); - + private readonly static Lazy>> _CanExecuteChanged_TokenTablesLazy = new(); + private static ConditionalWeakTable> _CanExecuteChanged_TokenTables => _CanExecuteChanged_TokenTablesLazy.Value; private static unsafe int Do_Abi_add_CanExecuteChanged_0(IntPtr thisPtr, IntPtr handler, global::WinRT.EventRegistrationToken* token) { diff --git a/src/WinRT.Runtime/Projections/INotifyCollectionChanged.net5.cs b/src/WinRT.Runtime/Projections/INotifyCollectionChanged.net5.cs index 61c8b75e7..7e66df4ed 100644 --- a/src/WinRT.Runtime/Projections/INotifyCollectionChanged.net5.cs +++ b/src/WinRT.Runtime/Projections/INotifyCollectionChanged.net5.cs @@ -41,8 +41,9 @@ static unsafe Vftbl() AbiToProjectionVftablePtr = (IntPtr)nativeVftbl; } - private static global::System.Runtime.CompilerServices.ConditionalWeakTable> _CollectionChanged_TokenTables = new global::System.Runtime.CompilerServices.ConditionalWeakTable>(); - + private readonly static Lazy>> _CollectionChanged_TokenTablesLazy = new(); + private static global::System.Runtime.CompilerServices.ConditionalWeakTable> _CollectionChanged_TokenTables => _CollectionChanged_TokenTablesLazy.Value; + [UnmanagedCallersOnly] private static unsafe int Do_Abi_add_CollectionChanged_0(IntPtr thisPtr, IntPtr handler, global::WinRT.EventRegistrationToken* token) { diff --git a/src/WinRT.Runtime/Projections/INotifyCollectionChanged.netstandard2.0.cs b/src/WinRT.Runtime/Projections/INotifyCollectionChanged.netstandard2.0.cs index d948dc616..2c30e2f72 100644 --- a/src/WinRT.Runtime/Projections/INotifyCollectionChanged.netstandard2.0.cs +++ b/src/WinRT.Runtime/Projections/INotifyCollectionChanged.netstandard2.0.cs @@ -42,7 +42,8 @@ static unsafe Vftbl() AbiToProjectionVftablePtr = (IntPtr)nativeVftbl; } - private static global::System.Runtime.CompilerServices.ConditionalWeakTable> _CollectionChanged_TokenTables = new global::System.Runtime.CompilerServices.ConditionalWeakTable>(); + private readonly static Lazy>> _CollectionChanged_TokenTablesLazy = new(); + private static global::System.Runtime.CompilerServices.ConditionalWeakTable> _CollectionChanged_TokenTables => _CollectionChanged_TokenTablesLazy.Value; private static unsafe int Do_Abi_add_CollectionChanged_0(IntPtr thisPtr, IntPtr handler, out global::WinRT.EventRegistrationToken token) { diff --git a/src/WinRT.Runtime/Projections/INotifyDataErrorInfo.net5.cs b/src/WinRT.Runtime/Projections/INotifyDataErrorInfo.net5.cs index a729b8fa2..34aa4a170 100644 --- a/src/WinRT.Runtime/Projections/INotifyDataErrorInfo.net5.cs +++ b/src/WinRT.Runtime/Projections/INotifyDataErrorInfo.net5.cs @@ -82,7 +82,8 @@ private static unsafe int Do_Abi_get_HasErrors_0(IntPtr thisPtr, byte* value) } return 0; } - private static global::System.Runtime.CompilerServices.ConditionalWeakTable>> _ErrorsChanged_TokenTables = new global::System.Runtime.CompilerServices.ConditionalWeakTable>>(); + private readonly static Lazy>>> _ErrorsChanged_TokenTablesLazy = new(); + private static global::System.Runtime.CompilerServices.ConditionalWeakTable>> _ErrorsChanged_TokenTables => _ErrorsChanged_TokenTablesLazy.Value; [UnmanagedCallersOnly(CallConvs = new [] {typeof(CallConvStdcall)})] private static unsafe int Do_Abi_add_ErrorsChanged_1(IntPtr thisPtr, IntPtr handler, global::WinRT.EventRegistrationToken* token) diff --git a/src/WinRT.Runtime/Projections/INotifyDataErrorInfo.netstandard2.0.cs b/src/WinRT.Runtime/Projections/INotifyDataErrorInfo.netstandard2.0.cs index 1c2adcb6b..296275a90 100644 --- a/src/WinRT.Runtime/Projections/INotifyDataErrorInfo.netstandard2.0.cs +++ b/src/WinRT.Runtime/Projections/INotifyDataErrorInfo.netstandard2.0.cs @@ -104,7 +104,8 @@ private static unsafe int Do_Abi_get_HasErrors_0(IntPtr thisPtr, byte* value) } return 0; } - private static global::System.Runtime.CompilerServices.ConditionalWeakTable>> _ErrorsChanged_TokenTables = new global::System.Runtime.CompilerServices.ConditionalWeakTable>>(); + private readonly static Lazy>>> _ErrorsChanged_TokenTablesLazy = new(); + private static global::System.Runtime.CompilerServices.ConditionalWeakTable>> _ErrorsChanged_TokenTables => _ErrorsChanged_TokenTablesLazy.Value; #if !NETSTANDARD2_0 [UnmanagedCallersOnly(CallConvs = new [] {typeof(CallConvStdcall)})] diff --git a/src/WinRT.Runtime/Projections/INotifyPropertyChanged.net5.cs b/src/WinRT.Runtime/Projections/INotifyPropertyChanged.net5.cs index 6ef0e6c46..959025a13 100644 --- a/src/WinRT.Runtime/Projections/INotifyPropertyChanged.net5.cs +++ b/src/WinRT.Runtime/Projections/INotifyPropertyChanged.net5.cs @@ -40,7 +40,8 @@ static unsafe Vftbl() AbiToProjectionVftablePtr = (IntPtr)nativeVftbl; } - private static global::System.Runtime.CompilerServices.ConditionalWeakTable> _PropertyChanged_TokenTables = new global::System.Runtime.CompilerServices.ConditionalWeakTable>(); + private readonly static Lazy>> _PropertyChanged_TokenTablesLazy = new(); + private static global::System.Runtime.CompilerServices.ConditionalWeakTable> _PropertyChanged_TokenTables => _PropertyChanged_TokenTablesLazy.Value; [UnmanagedCallersOnly] private static unsafe int Do_Abi_add_PropertyChanged_0(IntPtr thisPtr, IntPtr handler, global::WinRT.EventRegistrationToken* token) diff --git a/src/WinRT.Runtime/Projections/INotifyPropertyChanged.netstandard2.0.cs b/src/WinRT.Runtime/Projections/INotifyPropertyChanged.netstandard2.0.cs index 84774774d..9f2274aef 100644 --- a/src/WinRT.Runtime/Projections/INotifyPropertyChanged.netstandard2.0.cs +++ b/src/WinRT.Runtime/Projections/INotifyPropertyChanged.netstandard2.0.cs @@ -42,7 +42,8 @@ static unsafe Vftbl() AbiToProjectionVftablePtr = (IntPtr)nativeVftbl; } - private static global::System.Runtime.CompilerServices.ConditionalWeakTable> _PropertyChanged_TokenTables = new global::System.Runtime.CompilerServices.ConditionalWeakTable>(); + private readonly static Lazy>> _PropertyChanged_TokenTablesLazy = new(); + private static global::System.Runtime.CompilerServices.ConditionalWeakTable> _PropertyChanged_TokenTables => _PropertyChanged_TokenTablesLazy.Value; private static unsafe int Do_Abi_add_PropertyChanged_0(IntPtr thisPtr, IntPtr handler, out global::WinRT.EventRegistrationToken token) { diff --git a/src/cswinrt/code_writers.h b/src/cswinrt/code_writers.h index 3dacb2b2d..fc0563603 100644 --- a/src/cswinrt/code_writers.h +++ b/src/cswinrt/code_writers.h @@ -2580,8 +2580,13 @@ evt.Name()); for (auto&& evt : type.EventList()) { w.write(R"( -private static global::System.Runtime.CompilerServices.ConditionalWeakTable> _% = new();)", +private readonly static global::System.Lazy>> _%Lazy = new(); +private static global::System.Runtime.CompilerServices.ConditionalWeakTable> _% => _%Lazy.Value; +)", + bind(get_type_semantics(evt.EventType()), typedef_name_type::Projected, false), + evt.Name(), bind(get_type_semantics(evt.EventType()), typedef_name_type::Projected, false), + evt.Name(), evt.Name()); } } @@ -4490,12 +4495,17 @@ private static unsafe int Do_Abi_%% auto add_handler_event_token_name = add_signature.return_param_name(); auto remove_handler_event_token_name = method_signature{ remove_method }.params().back().first.Name(); - w.write("\nprivate static global::System.Runtime.CompilerServices.ConditionalWeakTable<%, global::WinRT.EventRegistrationTokenTable<%>> _%_TokenTables = new global::System.Runtime.CompilerServices.ConditionalWeakTable<%, global::WinRT.EventRegistrationTokenTable<%>>();", + w.write(R"( +private readonly static global::System.Lazy>> _%_TokenTablesLazy = new (); +private static global::System.Runtime.CompilerServices.ConditionalWeakTable<%, global::WinRT.EventRegistrationTokenTable<%>> _%_TokenTables => _%_TokenTablesLazy.Value; +)", type_name, bind(semantics, typedef_name_type::Projected, false), evt.Name(), type_name, - bind(semantics, typedef_name_type::Projected, false)); + bind(semantics, typedef_name_type::Projected, false), + evt.Name(), + evt.Name()); w.write( R"( From 792461bbdcc94fc5c9c198758458c18c623a298e Mon Sep 17 00:00:00 2001 From: Manodasan Date: Wed, 14 Jul 2021 13:30:39 -0700 Subject: [PATCH 15/18] Fix memory leak. --- src/WinRT.Runtime/Interop/IWeakReferenceSource.net5.cs | 9 ++++++++- .../Interop/IWeakReferenceSource.netstandard2.0.cs | 9 ++++++++- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/src/WinRT.Runtime/Interop/IWeakReferenceSource.net5.cs b/src/WinRT.Runtime/Interop/IWeakReferenceSource.net5.cs index 97dcf30ee..42219959e 100644 --- a/src/WinRT.Runtime/Interop/IWeakReferenceSource.net5.cs +++ b/src/WinRT.Runtime/Interop/IWeakReferenceSource.net5.cs @@ -172,7 +172,14 @@ private static int Do_Abi_Resolve(IntPtr thisPtr, Guid* riid, IntPtr* objectRefe var ThisPtr = _obj.ThisPtr; ExceptionHelpers.ThrowExceptionForHR(_obj.Vftbl.Resolve(ThisPtr, ref riid, out IntPtr objRef)); - return ComWrappersSupport.GetObjectReferenceForInterface(objRef); + try + { + return ComWrappersSupport.GetObjectReferenceForInterface(objRef); + } + finally + { + MarshalInspectable.DisposeAbi(objRef); + } } } } diff --git a/src/WinRT.Runtime/Interop/IWeakReferenceSource.netstandard2.0.cs b/src/WinRT.Runtime/Interop/IWeakReferenceSource.netstandard2.0.cs index bd2c92bf9..120f1e2fa 100644 --- a/src/WinRT.Runtime/Interop/IWeakReferenceSource.netstandard2.0.cs +++ b/src/WinRT.Runtime/Interop/IWeakReferenceSource.netstandard2.0.cs @@ -182,7 +182,14 @@ public IWeakReference(ObjectReference obj) public IObjectReference Resolve(Guid riid) { ExceptionHelpers.ThrowExceptionForHR(_obj.Vftbl.Resolve(ThisPtr, ref riid, out IntPtr objRef)); - return ComWrappersSupport.GetObjectReferenceForInterface(objRef); + try + { + return ComWrappersSupport.GetObjectReferenceForInterface(objRef); + } + finally + { + MarshalInspectable.DisposeAbi(objRef); + } } } } From d129bff64b4f7dea440b0218bbaa1da31619ebee Mon Sep 17 00:00:00 2001 From: Manodasan Date: Wed, 14 Jul 2021 16:26:56 -0700 Subject: [PATCH 16/18] Re-enable tests as they are now passing. --- .../ObjectLifetimeTests.cs | 43 +++++++++---------- 1 file changed, 21 insertions(+), 22 deletions(-) diff --git a/src/Tests/ObjectLifetimeTests/ObjectLifetimeTests.cs b/src/Tests/ObjectLifetimeTests/ObjectLifetimeTests.cs index ced231a29..433734144 100644 --- a/src/Tests/ObjectLifetimeTests/ObjectLifetimeTests.cs +++ b/src/Tests/ObjectLifetimeTests/ObjectLifetimeTests.cs @@ -355,10 +355,10 @@ public void BasicTest6b() AutoResetEvent loadedSignal = new AutoResetEvent(false); - AutoResetEvent unloadedSignal = new AutoResetEvent(false); - - - // Bug: _parentRef is still alive https://github.com/microsoft/CsWinRT/issues/897 + AutoResetEvent unloadedSignal = new AutoResetEvent(false); + + + [TestMethod] public void CycleTest1() { //Debugger.Launch(); @@ -414,15 +414,14 @@ public void CycleTest1() }); _asyncQueue.Run(); - } - - // - // Cycle from StackPanel to custom element to delegate back to StackPanel. - // - - - //[MethodImplAttribute(MethodImplOptions.NoOptimization | MethodImplOptions.NoInlining)] - //Bug:_parentRef is still alive https://github.com/microsoft/CsWinRT/issues/897 + } + + // + // Cycle from StackPanel to custom element to delegate back to StackPanel. + // + + [TestMethod] + [MethodImplAttribute(MethodImplOptions.NoOptimization | MethodImplOptions.NoInlining)] public void CycleTest1b() { _asyncQueue @@ -476,15 +475,15 @@ public void CycleTest1b() }); _asyncQueue.Run(); - } - - // - // Cycle from StackPanel to built-in element to delegate on framework event back to StackPanel. - // - - - //[MethodImplAttribute(MethodImplOptions.NoOptimization | MethodImplOptions.NoInlining)] - //Bug:_element1Ref is still alive https://github.com/microsoft/CsWinRT/issues/897 + } + + // + // Cycle from StackPanel to built-in element to delegate on framework event back to StackPanel. + // + + + [TestMethod] + [MethodImplAttribute(MethodImplOptions.NoOptimization | MethodImplOptions.NoInlining)] public void CycleTest1c() { _asyncQueue From d5b0fb3bb6648f3651bbeccb99e2d8f6c5edf9a9 Mon Sep 17 00:00:00 2001 From: Manodasan Date: Wed, 14 Jul 2021 21:38:42 -0700 Subject: [PATCH 17/18] Fix scenario where delegate can be null. --- src/WinRT.Runtime/Projections/EventHandler.cs | 8 ++++---- .../Projections/NotifyCollectionChangedEventHandler.cs | 2 +- .../Projections/PropertyChangedEventHandler.cs | 2 +- src/cswinrt/code_writers.h | 2 +- 4 files changed, 7 insertions(+), 7 deletions(-) diff --git a/src/WinRT.Runtime/Projections/EventHandler.cs b/src/WinRT.Runtime/Projections/EventHandler.cs index 7a548f21b..731df9485 100644 --- a/src/WinRT.Runtime/Projections/EventHandler.cs +++ b/src/WinRT.Runtime/Projections/EventHandler.cs @@ -44,8 +44,8 @@ public static IntPtr GetAbi(IObjectReference value) => public static unsafe global::System.EventHandler FromAbi(IntPtr nativeDelegate) { - var abiDelegate = ComWrappersSupport.GetObjectReferenceForInterface(nativeDelegate).As(GuidGenerator.GetIID(typeof(EventHandler))); - return (global::System.EventHandler)ComWrappersSupport.TryRegisterObjectForInterface(new global::System.EventHandler(new NativeDelegateWrapper(abiDelegate).Invoke), nativeDelegate); + var abiDelegate = ComWrappersSupport.GetObjectReferenceForInterface(nativeDelegate)?.As(GuidGenerator.GetIID(typeof(EventHandler))); + return abiDelegate is null ? null : (global::System.EventHandler)ComWrappersSupport.TryRegisterObjectForInterface(new global::System.EventHandler(new NativeDelegateWrapper(abiDelegate).Invoke), nativeDelegate); } [global::WinRT.ObjectReferenceWrapper(nameof(_nativeDelegate))] @@ -149,8 +149,8 @@ public static IntPtr GetAbi(IObjectReference value) => public static unsafe global::System.EventHandler FromAbi(IntPtr nativeDelegate) { - var abiDelegate = ComWrappersSupport.GetObjectReferenceForInterface(nativeDelegate).As(GuidGenerator.GetIID(typeof(EventHandler))); - return (global::System.EventHandler)ComWrappersSupport.TryRegisterObjectForInterface(new global::System.EventHandler(new NativeDelegateWrapper(abiDelegate).Invoke), nativeDelegate); + var abiDelegate = ComWrappersSupport.GetObjectReferenceForInterface(nativeDelegate)?.As(GuidGenerator.GetIID(typeof(EventHandler))); + return abiDelegate is null ? null : (global::System.EventHandler)ComWrappersSupport.TryRegisterObjectForInterface(new global::System.EventHandler(new NativeDelegateWrapper(abiDelegate).Invoke), nativeDelegate); } [global::WinRT.ObjectReferenceWrapper(nameof(_nativeDelegate))] diff --git a/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs b/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs index 7eb1095f5..0918334d1 100644 --- a/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs +++ b/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs @@ -40,7 +40,7 @@ public static unsafe IObjectReference CreateMarshaler(global::System.Collections public static unsafe global::System.Collections.Specialized.NotifyCollectionChangedEventHandler FromAbi(IntPtr nativeDelegate) { - var abiDelegate = ComWrappersSupport.GetObjectReferenceForInterface(nativeDelegate).As(GuidGenerator.GetIID(typeof(NotifyCollectionChangedEventHandler))); + var abiDelegate = ComWrappersSupport.GetObjectReferenceForInterface(nativeDelegate)?.As(GuidGenerator.GetIID(typeof(NotifyCollectionChangedEventHandler))); return abiDelegate is null ? null : (global::System.Collections.Specialized.NotifyCollectionChangedEventHandler)ComWrappersSupport.TryRegisterObjectForInterface(new global::System.Collections.Specialized.NotifyCollectionChangedEventHandler(new NativeDelegateWrapper(abiDelegate).Invoke), nativeDelegate); } diff --git a/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs b/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs index 93ea48287..cb251952c 100644 --- a/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs +++ b/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs @@ -39,7 +39,7 @@ public static unsafe IObjectReference CreateMarshaler(global::System.ComponentMo public static unsafe global::System.ComponentModel.PropertyChangedEventHandler FromAbi(IntPtr nativeDelegate) { - var abiDelegate = ComWrappersSupport.GetObjectReferenceForInterface(nativeDelegate).As(GuidGenerator.GetIID(typeof(PropertyChangedEventHandler))); + var abiDelegate = ComWrappersSupport.GetObjectReferenceForInterface(nativeDelegate)?.As(GuidGenerator.GetIID(typeof(PropertyChangedEventHandler))); return abiDelegate is null ? null : (global::System.ComponentModel.PropertyChangedEventHandler)ComWrappersSupport.TryRegisterObjectForInterface(new global::System.ComponentModel.PropertyChangedEventHandler(new NativeDelegateWrapper(abiDelegate).Invoke), nativeDelegate); } diff --git a/src/cswinrt/code_writers.h b/src/cswinrt/code_writers.h index fc0563603..bd45d772d 100644 --- a/src/cswinrt/code_writers.h +++ b/src/cswinrt/code_writers.h @@ -5675,7 +5675,7 @@ public static IntPtr GetAbi(IObjectReference value) => MarshalInterfaceHelper<%> public static unsafe % FromAbi(IntPtr nativeDelegate) { -var abiDelegate = ComWrappersSupport.GetObjectReferenceForInterface(nativeDelegate).As(GuidGenerator.GetIID(typeof(@%))); +var abiDelegate = ComWrappersSupport.GetObjectReferenceForInterface(nativeDelegate)?.As(GuidGenerator.GetIID(typeof(@%))); return abiDelegate is null ? null : (%)ComWrappersSupport.TryRegisterObjectForInterface(new %(new NativeDelegateWrapper(abiDelegate).Invoke), nativeDelegate); } From ac7e25bdfde8a109901e9b54138742c29e6dd5be Mon Sep 17 00:00:00 2001 From: Manodasan Date: Mon, 19 Jul 2021 11:18:43 -0700 Subject: [PATCH 18/18] Don't wrap native delegates. --- src/WinRT.Runtime/Projections/EventHandler.cs | 4 ++-- .../Projections/NotifyCollectionChangedEventHandler.cs | 4 ++-- src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/WinRT.Runtime/Projections/EventHandler.cs b/src/WinRT.Runtime/Projections/EventHandler.cs index 731df9485..493b01d87 100644 --- a/src/WinRT.Runtime/Projections/EventHandler.cs +++ b/src/WinRT.Runtime/Projections/EventHandler.cs @@ -37,7 +37,7 @@ static EventHandler() public static global::System.Delegate AbiInvokeDelegate { get; } public static unsafe IObjectReference CreateMarshaler(global::System.EventHandler managedDelegate) => - managedDelegate is null ? null : ComWrappersSupport.CreateCCWForObject(managedDelegate).As(GuidGenerator.GetIID(typeof(EventHandler))); + managedDelegate is null ? null : MarshalDelegate.CreateMarshaler(managedDelegate, GuidGenerator.GetIID(typeof(EventHandler))); public static IntPtr GetAbi(IObjectReference value) => value is null ? IntPtr.Zero : MarshalInterfaceHelper>.GetAbi(value); @@ -142,7 +142,7 @@ static EventHandler() public static global::System.Delegate AbiInvokeDelegate { get; } public static unsafe IObjectReference CreateMarshaler(global::System.EventHandler managedDelegate) => - managedDelegate is null ? null : ComWrappersSupport.CreateCCWForObject(managedDelegate).As(GuidGenerator.GetIID(typeof(EventHandler))); + managedDelegate is null ? null : MarshalDelegate.CreateMarshaler(managedDelegate, GuidGenerator.GetIID(typeof(EventHandler))); public static IntPtr GetAbi(IObjectReference value) => value is null ? IntPtr.Zero : MarshalInterfaceHelper>.GetAbi(value); diff --git a/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs b/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs index 0918334d1..2745bb30e 100644 --- a/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs +++ b/src/WinRT.Runtime/Projections/NotifyCollectionChangedEventHandler.cs @@ -34,7 +34,7 @@ static NotifyCollectionChangedEventHandler() public static global::System.Delegate AbiInvokeDelegate { get; } public static unsafe IObjectReference CreateMarshaler(global::System.Collections.Specialized.NotifyCollectionChangedEventHandler managedDelegate) => - managedDelegate is null ? null : ComWrappersSupport.CreateCCWForObject(managedDelegate).As(GuidGenerator.GetIID(typeof(NotifyCollectionChangedEventHandler))); + managedDelegate is null ? null : MarshalDelegate.CreateMarshaler(managedDelegate, GuidGenerator.GetIID(typeof(NotifyCollectionChangedEventHandler))); public static IntPtr GetAbi(IObjectReference value) => MarshalInterfaceHelper.GetAbi(value); @@ -87,7 +87,7 @@ public void Invoke(object sender, global::System.Collections.Specialized.NotifyC } public static IntPtr FromManaged(global::System.Collections.Specialized.NotifyCollectionChangedEventHandler managedDelegate) => - managedDelegate is null ? IntPtr.Zero : CreateMarshaler(managedDelegate).GetRef(); + CreateMarshaler(managedDelegate)?.GetRef() ?? IntPtr.Zero; public static void DisposeMarshaler(IObjectReference value) => MarshalInterfaceHelper.DisposeMarshaler(value); diff --git a/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs b/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs index cb251952c..f2793a7b9 100644 --- a/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs +++ b/src/WinRT.Runtime/Projections/PropertyChangedEventHandler.cs @@ -33,7 +33,7 @@ static PropertyChangedEventHandler() public static global::System.Delegate AbiInvokeDelegate { get; } public static unsafe IObjectReference CreateMarshaler(global::System.ComponentModel.PropertyChangedEventHandler managedDelegate) => - managedDelegate is null ? null : ComWrappersSupport.CreateCCWForObject(managedDelegate).As(GuidGenerator.GetIID(typeof(PropertyChangedEventHandler))); + managedDelegate is null ? null : MarshalDelegate.CreateMarshaler(managedDelegate, GuidGenerator.GetIID(typeof(PropertyChangedEventHandler))); public static IntPtr GetAbi(IObjectReference value) => MarshalInterfaceHelper.GetAbi(value); @@ -86,7 +86,7 @@ public void Invoke(object sender, global::System.ComponentModel.PropertyChangedE } public static IntPtr FromManaged(global::System.ComponentModel.PropertyChangedEventHandler managedDelegate) => - managedDelegate is null ? IntPtr.Zero : CreateMarshaler(managedDelegate).GetRef(); + CreateMarshaler(managedDelegate)?.GetRef() ?? IntPtr.Zero; public static void DisposeMarshaler(IObjectReference value) => MarshalInterfaceHelper.DisposeMarshaler(value);