[release/7.0.2xx] [net7.0] [core] WeakEventManager.RemoveEventHandler…

…() should clear subscriptions (#14074)

* [core] WeakEventManager.RemoveEventHandler() should clear subscriptions

Context: #12039
Context: https://github.com/Vroomer/MAUI-master-detail-memory-leak
Context: https://github.com/symbiogenesis/Maui.DataGrid/tree/memory-leak

Reviewing memory snapshots in the above apps, sometimes I would see
new `WeakEventManager+Subscription` objects be created and never go
away.

I noticed the `WeakEventManager.RemoveEventHandler()` method did not
remove any `Subscription` entries that it encountered were no longer
alive.

I could reproduce this problem in a test, and improved an existing
test to check this collection is cleared appropriately:

    readonly Dictionary<string, List<Subscription>> _eventHandlers = new();

* Remove the `n - 1` business

This is closer to the standard `forr` snippet that has been around forever, just using `n` instead of `i`.

* - add null forgiveness to test

---------

Co-authored-by: Jonathan Peppers <jonathan.peppers@microsoft.com>
Co-authored-by: Shane Neuville <shneuvil@microsoft.com>

src/Core/src/WeakEventManager.cs

@@ -125,15 +125,23 @@ void RemoveEventHandler(string eventName, object? handlerTarget, MemberInfo meth
 			if (!_eventHandlers.TryGetValue(eventName, out List<Subscription>? subscriptions))
 				return;
 
-			for (int n = subscriptions.Count; n > 0; n--)
+			for (int n = subscriptions.Count - 1; n >= 0; n--)
 			{
-				Subscription current = subscriptions[n - 1];
+				Subscription current = subscriptions[n];
 
-				if (current.Subscriber?.Target != handlerTarget || current.Handler.Name != methodInfo.Name)
+				if (current.Subscriber != null && !current.Subscriber.IsAlive)
+				{
+					// If not alive, remove and continue
+					subscriptions.RemoveAt(n);
 					continue;
+				}
 
-				subscriptions.Remove(current);
-				break;
+				if (current.Subscriber?.Target == handlerTarget && current.Handler.Name == methodInfo.Name)
+				{
+					// Found the match, we can break
+					subscriptions.RemoveAt(n);
+					break;
+				}
 			}
 		}
 

src/Core/tests/UnitTests/WeakEventManagerTests.cs

@@ -1,6 +1,9 @@
 #nullable enable
 using System;
+using System.Collections;
 using System.ComponentModel;
+using System.Linq;
+using System.Reflection;
 using Xunit;
 
 namespace Microsoft.Maui.UnitTests
@@ -65,6 +68,19 @@ void OnTestEvent()
 			{
 				_weakEventManager.HandleEvent(this, EventArgs.Empty, nameof(TestEvent));
 			}
+
+			public int EventHandlerCount
+			{
+				get
+				{
+					// Access private members:
+					// Dictionary<string, List<Subscription>> eventHandlers = WeakEventManager._eventHandlers;
+					var flags = BindingFlags.NonPublic | BindingFlags.Instance;
+					var eventHandlers = _weakEventManager.GetType().GetField("_eventHandlers", flags)?.GetValue(_weakEventManager) as IDictionary;
+					Assert.NotNull(eventHandlers);
+					return eventHandlers!.Values.Cast<IList>().Sum(l => l.Count);
+				}
+			}
 		}
 
 		internal class TestSubscriber
@@ -74,6 +90,11 @@ public void Subscribe(TestEventSource source)
 				source.TestEvent += SourceOnTestEvent;
 			}
 
+			public void Unsubscribe(TestEventSource source)
+			{
+				source.TestEvent -= SourceOnTestEvent;
+			}
+
 			void SourceOnTestEvent(object? sender, EventArgs eventArgs)
 			{
 				throw new Exception("Fail");
@@ -230,7 +251,7 @@ public void StaticHandlerShouldRun()
 		}
 
 		[Fact]
-		public void VerifySubscriberCanBeCollected()
+		public void VerifySubscriberCanBeCollected_FireEvent()
 		{
 			WeakReference? wr = null;
 			var source = new TestEventSource();
@@ -250,6 +271,28 @@ public void VerifySubscriberCanBeCollected()
 			// The handler for this calls Assert.Fail, so if the subscriber has not been collected
 			// the handler will be called and the test will fail
 			source.FireTestEvent();
+			Assert.Equal(0, source.EventHandlerCount);
+		}
+
+		[Fact]
+		public void VerifySubscriberCanBeCollected_Unsubscribe()
+		{
+			WeakReference? wr = null;
+			var source = new TestEventSource();
+			new Action(() =>
+			{
+				var ts = new TestSubscriber();
+				wr = new WeakReference(ts);
+				ts.Subscribe(source);
+			})();
+
+			GC.Collect();
+			GC.WaitForPendingFinalizers();
+			new TestSubscriber().Unsubscribe(source);
+
+			Assert.NotNull(wr);
+			Assert.False(wr?.IsAlive);
+			Assert.Equal(0, source.EventHandlerCount);
 		}
 
 		[Fact]