Allow subscriber to be collected if MessagingCenter is the only reference to it

Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla45926.cs

@@ -0,0 +1,111 @@
+using System;
+using System.Threading;
+using Xamarin.Forms.CustomAttributes;
+using Xamarin.Forms.Internals;
+
+#if UITEST
+using Xamarin.UITest;
+using NUnit.Framework;
+#endif
+
+namespace Xamarin.Forms.Controls.Issues
+{
+
+	[Preserve(AllMembers = true)]
+	[Issue(IssueTracker.Bugzilla, 45926, "MessagingCenter prevents subscriber from being collected", PlatformAffected.All)]
+	public class Bugzilla45926 : TestNavigationPage
+	{
+		protected override void Init()
+		{
+			Button createPage, sendMessage, doGC;
+
+			Label instanceCount = new Label();
+			Label messageCount = new Label();
+
+			instanceCount.Text = $"Instances: {_45926SecondPage.InstanceCounter.ToString()}";
+			messageCount.Text = $"Messages: {_45926SecondPage.MessageCounter.ToString()}";
+
+			var content = new ContentPage {
+				Title = "Test",
+				Content = new StackLayout {
+					VerticalOptions = LayoutOptions.Center,
+					Children = {
+						(createPage = new Button { Text = "New Page" }),
+						(sendMessage = new Button { Text = "Send Message" }),
+						(doGC = new Button { Text = "Do GC" }),
+						instanceCount, messageCount
+					}
+				}
+			};
+
+			createPage.Clicked += (s, e) => PushAsync (new _45926SecondPage ());
+			sendMessage.Clicked += (s, e) =>
+			{
+				MessagingCenter.Send (this, "Test");
+			};
+
+			doGC.Clicked += (sender, e) => {
+				GC.Collect ();
+				GC.WaitForPendingFinalizers();
+				instanceCount.Text = $"Instances: {_45926SecondPage.InstanceCounter.ToString()}";
+				messageCount.Text = $"Messages: {_45926SecondPage.MessageCounter.ToString()}";
+			};
+
+			PushAsync(content);
+		}
+
+#if UITEST
+		[Test]
+		public void Issue45926Test ()
+		{
+			RunningApp.WaitForElement (q => q.Marked ("New Page"));
+
+			RunningApp.Tap (q => q.Marked ("New Page"));
+			RunningApp.Back();
+			RunningApp.Tap(q => q.Marked("Do GC"));
+			RunningApp.Tap(q => q.Marked("Send Message"));
+			RunningApp.Tap(q => q.Marked("Do GC"));
+
+			RunningApp.WaitForElement (q => q.Marked ("Instances: 0"));
+			RunningApp.WaitForElement (q => q.Marked ("Messages: 0"));
+		}
+#endif
+	}
+
+	[Preserve(AllMembers = true)]
+	public class _45926SecondPage : ContentPage
+	{
+		public static int InstanceCounter = 0;
+		public static int MessageCounter = 0;
+
+		public _45926SecondPage ()
+		{
+			Interlocked.Increment(ref InstanceCounter);
+
+			Content = new Label {
+				HorizontalOptions = LayoutOptions.Center,
+				VerticalOptions = LayoutOptions.Center,
+				Text = "Second Page #" + (InstanceCounter)
+			};
+
+			MessagingCenter.Subscribe<Bugzilla45926> (this, "Test", OnMessage);
+		}
+
+		protected override void OnDisappearing ()
+		{
+			base.OnDisappearing ();
+		}
+
+		void OnMessage (Bugzilla45926 app)
+		{
+			System.Diagnostics.Debug.WriteLine ("Got Test message!");
+			Interlocked.Increment(ref MessageCounter);
+		}
+
+		~_45926SecondPage ()
+		{
+			Interlocked.Decrement(ref InstanceCounter);
+			System.Diagnostics.Debug.WriteLine ("~SecondPage: {0}", GetHashCode ());
+		}
+	}
+}

Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems

@@ -468,6 +468,7 @@
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla36802.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla35736.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla48158.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)Bugzilla45926.cs" />
   </ItemGroup>
   <ItemGroup>
     <EmbeddedResource Include="$(MSBuildThisFileDirectory)Bugzilla22229.xaml">

Xamarin.Forms.Core.UnitTests/MessagingCenterTests.cs

@@ -1,7 +1,6 @@
 using System;
 using NUnit.Framework;
 
-
 namespace Xamarin.Forms.Core.UnitTests
 {
 	[TestFixture]
@@ -187,5 +186,171 @@ public void UnsubscribeInCallback ()
 
 			Assert.AreEqual (1, messageCount);
 		}
+
+		[Test]
+		public void SubscriberShouldBeCollected()
+		{
+			new Action(() =>
+			{
+				var subscriber = new TestSubcriber();
+				MessagingCenter.Subscribe<TestPublisher>(subscriber, "test", p => Assert.Fail());
+			})();
+
+			GC.Collect();
+			GC.WaitForPendingFinalizers();
+
+			var pub = new TestPublisher();
+			pub.Test(); // Assert.Fail() shouldn't be called, because the TestSubcriber object should have ben GCed
+		}
+
+		[Test]
+		public void ShouldBeCollectedIfCallbackTargetIsSubscriber()
+		{
+			WeakReference wr = null;
+
+			new Action(() =>
+			{
+				var subscriber = new TestSubcriber();
+
+				wr = new WeakReference(subscriber);
+
+				subscriber.SubscribeToTestMessages();
+			})();
+
+			GC.Collect();
+			GC.WaitForPendingFinalizers();
+
+			var pub = new TestPublisher();
+			pub.Test();
+
+			Assert.IsFalse(wr.IsAlive); // The Action target and subscriber were the same object, so both could be collected
+		}
+
+		[Test]
+		public void NotCollectedIfSubscriberIsNotTheCallbackTarget()
+		{
+			WeakReference wr = null;
+
+			new Action(() =>
+			{
+				var subscriber = new TestSubcriber();
+
+				wr = new WeakReference(subscriber);
+
+				// This creates a closure, so the callback target is not 'subscriber', but an instancce of a compiler generated class 
+				// So MC has to keep a strong reference to it, and 'subscriber' won't be collectable
+				MessagingCenter.Subscribe<TestPublisher>(subscriber, "test", p => subscriber.SetSuccess());
+			})();
+
+			GC.Collect();
+			GC.WaitForPendingFinalizers();
+
+			Assert.IsTrue(wr.IsAlive); // The closure in Subscribe should be keeping the subscriber alive
+			Assert.IsNotNull(wr.Target as TestSubcriber);
+
+			Assert.IsFalse(((TestSubcriber)wr.Target).Successful);
+
+			var pub = new TestPublisher();
+			pub.Test();
+
+			Assert.IsTrue(((TestSubcriber)wr.Target).Successful);  // Since it's still alive, the subscriber should still have received the message and updated the property
+		}
+
+		[Test]
+		public void SubscriberCollectableAfterUnsubscribeEvenIfHeldByClosure()
+		{
+			WeakReference wr = null;
+
+			new Action(() =>
+			{
+				var subscriber = new TestSubcriber();
+
+				wr = new WeakReference(subscriber);
+
+				MessagingCenter.Subscribe<TestPublisher>(subscriber, "test", p => subscriber.SetSuccess());
+			})();
+
+			Assert.IsNotNull(wr.Target as TestSubcriber); 
+
+			MessagingCenter.Unsubscribe<TestPublisher>(wr.Target, "test");
+
+			GC.Collect();
+			GC.WaitForPendingFinalizers();
+
+			Assert.IsFalse(wr.IsAlive); // The Action target and subscriber were the same object, so both could be collected
+		}
+
+		[Test]
+		public void StaticCallback()
+		{
+			int i = 4;
+
+			var subscriber = new TestSubcriber();
+
+			MessagingCenter.Subscribe<TestPublisher>(subscriber, "test", p => MessagingCenterTestsCallbackSource.Increment(ref i));
+
+			GC.Collect();
+			GC.WaitForPendingFinalizers();
+
+			var pub = new TestPublisher();
+			pub.Test();
+
+			Assert.IsTrue(i == 5, "The static method should have incremented 'i'"); 
+		}
+
+		[Test]
+		public void NothingShouldBeCollected()
+		{
+			var success = false;
+
+			var subscriber = new TestSubcriber();
+
+			var source = new MessagingCenterTestsCallbackSource();
+			MessagingCenter.Subscribe<TestPublisher>(subscriber, "test", p => source.SuccessCallback(ref success));
+
+			GC.Collect();
+			GC.WaitForPendingFinalizers();
+
+			var pub = new TestPublisher();
+			pub.Test(); 
+
+			Assert.True(success); // TestCallbackSource.SuccessCallback() should be invoked to make success == true
+		}
+
+		class TestSubcriber
+		{
+			public void SetSuccess()
+			{
+				Successful = true;
+			}
+
+			public bool Successful { get; private set; }
+
+			public void SubscribeToTestMessages()
+			{
+				MessagingCenter.Subscribe<TestPublisher>(this, "test", p => SetSuccess());
+			}
+		}
+
+		class TestPublisher
+		{
+			public void Test()
+			{
+				MessagingCenter.Send(this, "test");
+			}
+		}
+
+		public class MessagingCenterTestsCallbackSource
+		{
+			public void SuccessCallback(ref bool success)
+			{
+				success = true;
+			}
+
+			public static void Increment(ref int i)
+			{
+				i = i + 1;
+			}
+		}
 	}
 }