Allow subscriber to be collected if MessagingCenter is the only reference to it (#617)

* Repro

* Make messaging center callbacks weak references

* Preserve attribute

* Fix test method name

* Watch for collection of actual delegate target instead of wrapper delegate

* Preserve the original platform instance when changing main page

* Better tests for lambda situations

* Update tests, make callback target a weakreference if it's the subscriber

* Ensure old Platform MessagingCenter subs are gone before creating new Platform

1 files changed, 123 insertions, 41 deletions

diff --git a/Xamarin.Forms.Core/MessagingCenter.cs b/Xamarin.Forms.Core/MessagingCenter.cs
index 0efb3a37..5e60046d 100644
--- a/Xamarin.Forms.Core/MessagingCenter.cs
+++ b/Xamarin.Forms.Core/MessagingCenter.cs
@@ -1,60 +1,140 @@
 using System;
 using System.Collections.Generic;
 using System.Linq;
+using System.Reflection;
+using System.Runtime.CompilerServices;
 
 namespace Xamarin.Forms
 {
 	public static class MessagingCenter
 	{
-		static readonly Dictionary<Tuple<string, Type, Type>, List<Tuple<WeakReference, Action<object, object>>>> s_callbacks =
-			new Dictionary<Tuple<string, Type, Type>, List<Tuple<WeakReference, Action<object, object>>>>();
+		class Sender : Tuple<string, Type, Type>
+		{
+			public Sender(string message, Type senderType, Type argType) : base(message, senderType, argType)
+			{
+			}
+		}
+
+		delegate bool Filter(object sender);
+
+		class MaybeWeakReference
+		{
+			WeakReference DelegateWeakReference { get; set; }
+			object DelegateStrongReference { get; set; }
+
+			readonly bool _isStrongReference;
+
+			public MaybeWeakReference(object subscriber, object delegateSource)
+			{
+				if (subscriber.Equals(delegateSource))
+				{
+					// The target is the subscriber; we can use a weakreference
+					DelegateWeakReference = new WeakReference(delegateSource);
+					_isStrongReference = false;
+				}
+				else
+				{
+					DelegateStrongReference = delegateSource;
+					_isStrongReference = true;
+				}
+			}
+
+			public object Target => _isStrongReference ? DelegateStrongReference : DelegateWeakReference.Target;
+			public bool IsAlive => _isStrongReference || DelegateWeakReference.IsAlive;
+		}
+
+		class Subscription : Tuple<WeakReference, MaybeWeakReference, MethodInfo, Filter>
+		{
+			public Subscription(object subscriber, object delegateSource, MethodInfo methodInfo, Filter filter)
+				: base(new WeakReference(subscriber), new MaybeWeakReference(subscriber, delegateSource), methodInfo, filter)
+			{
+			}
+
+			public WeakReference Subscriber => Item1;
+			MaybeWeakReference DelegateSource => Item2;
+			MethodInfo MethodInfo => Item3;
+			Filter Filter => Item4;
+
+			public void InvokeCallback(object sender, object args)
+			{
+				if (!Filter(sender))
+				{
+					return;
+				}
+
+				if (MethodInfo.IsStatic)
+				{
+					MethodInfo.Invoke(null, MethodInfo.GetParameters().Length == 1 ? new[] { sender } : new[] { sender, args });
+					return;
+				}
+
+				var target = DelegateSource.Target;
+
+				if (target == null)
+				{
+					return; // Collected 
+				}
+
+				MethodInfo.Invoke(target, MethodInfo.GetParameters().Length == 1 ? new[] { sender } : new[] { sender, args });
+			}
+
+			public bool CanBeRemoved()
+			{
+				return !Subscriber.IsAlive || !DelegateSource.IsAlive;
+			}
+		}
+
+		static readonly Dictionary<Sender, List<Subscription>> s_subscriptions =
+			new Dictionary<Sender, List<Subscription>>();
 
 		public static void Send<TSender, TArgs>(TSender sender, string message, TArgs args) where TSender : class
 		{
 			if (sender == null)
-				throw new ArgumentNullException("sender");
+				throw new ArgumentNullException(nameof(sender));
 			InnerSend(message, typeof(TSender), typeof(TArgs), sender, args);
 		}
 
 		public static void Send<TSender>(TSender sender, string message) where TSender : class
 		{
 			if (sender == null)
-				throw new ArgumentNullException("sender");
+				throw new ArgumentNullException(nameof(sender));
 			InnerSend(message, typeof(TSender), null, sender, null);
 		}
 
 		public static void Subscribe<TSender, TArgs>(object subscriber, string message, Action<TSender, TArgs> callback, TSender source = null) where TSender : class
 		{
 			if (subscriber == null)
-				throw new ArgumentNullException("subscriber");
+				throw new ArgumentNullException(nameof(subscriber));
 			if (callback == null)
-				throw new ArgumentNullException("callback");
+				throw new ArgumentNullException(nameof(callback));
 
-			Action<object, object> wrap = (sender, args) =>
+			var target = callback.Target;
+
+			Filter filter = sender =>
 			{
 				var send = (TSender)sender;
-				if (source == null || send == source)
-					callback((TSender)sender, (TArgs)args);
+				return (source == null || send == source);
 			};
 
-			InnerSubscribe(subscriber, message, typeof(TSender), typeof(TArgs), wrap);
+			InnerSubscribe(subscriber, message, typeof(TSender), typeof(TArgs), target, callback.GetMethodInfo(), filter);
 		}
 
 		public static void Subscribe<TSender>(object subscriber, string message, Action<TSender> callback, TSender source = null) where TSender : class
 		{
 			if (subscriber == null)
-				throw new ArgumentNullException("subscriber");
+				throw new ArgumentNullException(nameof(subscriber));
 			if (callback == null)
-				throw new ArgumentNullException("callback");
+				throw new ArgumentNullException(nameof(callback));
+
+			var target = callback.Target;
 
-			Action<object, object> wrap = (sender, args) =>
+			Filter filter = sender =>
 			{
 				var send = (TSender)sender;
-				if (source == null || send == source)
-					callback((TSender)sender);
+				return (source == null || send == source);
 			};
 
-			InnerSubscribe(subscriber, message, typeof(TSender), null, wrap);
+			InnerSubscribe(subscriber, message, typeof(TSender), null, target, callback.GetMethodInfo(), filter);
 		}
 
 		public static void Unsubscribe<TSender, TArgs>(object subscriber, string message) where TSender : class
@@ -69,18 +149,18 @@ namespace Xamarin.Forms
 
 		internal static void ClearSubscribers()
 		{
-			s_callbacks.Clear();
+			s_subscriptions.Clear();
 		}
 
 		static void InnerSend(string message, Type senderType, Type argType, object sender, object args)
 		{
 			if (message == null)
-				throw new ArgumentNullException("message");
-			var key = new Tuple<string, Type, Type>(message, senderType, argType);
-			if (!s_callbacks.ContainsKey(key))
+				throw new ArgumentNullException(nameof(message));
+			var key = new Sender(message, senderType, argType);
+			if (!s_subscriptions.ContainsKey(key))
 				return;
-			List<Tuple<WeakReference, Action<object, object>>> actions = s_callbacks[key];
-			if (actions == null || !actions.Any())
+			List<Subscription> subcriptions = s_subscriptions[key];
+			if (subcriptions == null || !subcriptions.Any())
 				return; // should not be reachable
 
 			// ok so this code looks a bit funky but here is the gist of the problem. It is possible that in the course
@@ -88,44 +168,46 @@ namespace Xamarin.Forms
 			// the callback. This would invalidate the enumerator. To work around this we make a copy. However if you unsubscribe 
 			// from a message you can fairly reasonably expect that you will therefor not receive a call. To fix this we then
 			// check that the item we are about to send the message to actually exists in the live list.
-			List<Tuple<WeakReference, Action<object, object>>> actionsCopy = actions.ToList();
-			foreach (Tuple<WeakReference, Action<object, object>> action in actionsCopy)
+			List<Subscription> subscriptionsCopy = subcriptions.ToList();
+			foreach (Subscription subscription in subscriptionsCopy)
 			{
-				if (action.Item1.Target != null && actions.Contains(action))
-					action.Item2(sender, args);
+				if (subscription.Subscriber.Target != null && subcriptions.Contains(subscription))
+				{
+					subscription.InvokeCallback(sender, args);
+				}
 			}
 		}
 
-		static void InnerSubscribe(object subscriber, string message, Type senderType, Type argType, Action<object, object> callback)
+		static void InnerSubscribe(object subscriber, string message, Type senderType, Type argType, object target, MethodInfo methodInfo, Filter filter)
 		{
 			if (message == null)
-				throw new ArgumentNullException("message");
-			var key = new Tuple<string, Type, Type>(message, senderType, argType);
-			var value = new Tuple<WeakReference, Action<object, object>>(new WeakReference(subscriber), callback);
-			if (s_callbacks.ContainsKey(key))
+				throw new ArgumentNullException(nameof(message));
+			var key = new Sender(message, senderType, argType);
+			var value = new Subscription(subscriber, target, methodInfo, filter);
+			if (s_subscriptions.ContainsKey(key))
 			{
-				s_callbacks[key].Add(value);
+				s_subscriptions[key].Add(value);
 			}
 			else
 			{
-				var list = new List<Tuple<WeakReference, Action<object, object>>> { value };
-				s_callbacks[key] = list;
+				var list = new List<Subscription> { value };
+				s_subscriptions[key] = list;
 			}
 		}
 
 		static void InnerUnsubscribe(string message, Type senderType, Type argType, object subscriber)
 		{
 			if (subscriber == null)
-				throw new ArgumentNullException("subscriber");
+				throw new ArgumentNullException(nameof(subscriber));
 			if (message == null)
-				throw new ArgumentNullException("message");
+				throw new ArgumentNullException(nameof(message));
 
-			var key = new Tuple<string, Type, Type>(message, senderType, argType);
-			if (!s_callbacks.ContainsKey(key))
+			var key = new Sender(message, senderType, argType);
+			if (!s_subscriptions.ContainsKey(key))
 				return;
-			s_callbacks[key].RemoveAll(tuple => !tuple.Item1.IsAlive || tuple.Item1.Target == subscriber);
-			if (!s_callbacks[key].Any())
-				s_callbacks.Remove(key);
+			s_subscriptions[key].RemoveAll(sub => !sub.CanBeRemoved() || sub.Subscriber.Target == subscriber);
+			if (!s_subscriptions[key].Any())
+				s_subscriptions.Remove(key);
 		}
 	}
 }
\ No newline at end of file