From 7e0125948a75088b21b758547757babed0ff8942 Mon Sep 17 00:00:00 2001 From: "E.Z. Hart" Date: Tue, 6 Dec 2016 15:08:36 -0700 Subject: [PATCH 1/9] Repro --- .../Bugzilla45926.cs | 103 ++++++++++++++++++ ...rin.Forms.Controls.Issues.Shared.projitems | 1 + 2 files changed, 104 insertions(+) create mode 100644 Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla45926.cs diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla45926.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla45926.cs new file mode 100644 index 00000000000..d2ab214cf98 --- /dev/null +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla45926.cs @@ -0,0 +1,103 @@ +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 Issue1Test () + //{ + // RunningApp.Screenshot ("I am at Issue 1"); + // RunningApp.WaitForElement (q => q.Marked ("IssuePageLabel")); + // RunningApp.Screenshot ("I see the Label"); + //} +#endif + } + + 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 (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 ()); + } + } +} \ No newline at end of file diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems index 21d7a4404f2..2e9e4808279 100644 --- a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems @@ -468,6 +468,7 @@ + From bf543c3bc3fccd65bb22374805b0e2bbc5638485 Mon Sep 17 00:00:00 2001 From: "E.Z. Hart" Date: Tue, 6 Dec 2016 17:03:15 -0700 Subject: [PATCH 2/9] Make messaging center callbacks weak references --- .../Bugzilla45926.cs | 23 ++-- Xamarin.Forms.Core/MessagingCenter.cs | 105 ++++++++++++------ 2 files changed, 85 insertions(+), 43 deletions(-) diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla45926.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla45926.cs index d2ab214cf98..fda0e0b6331 100644 --- a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla45926.cs +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla45926.cs @@ -54,14 +54,21 @@ protected override void Init() PushAsync(content); } - #if UITEST - // [Test] - //public void Issue1Test () - //{ - // RunningApp.Screenshot ("I am at Issue 1"); - // RunningApp.WaitForElement (q => q.Marked ("IssuePageLabel")); - // RunningApp.Screenshot ("I see the Label"); - //} +#if UITEST + [Test] + public void Issue1Test () + { + 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 } diff --git a/Xamarin.Forms.Core/MessagingCenter.cs b/Xamarin.Forms.Core/MessagingCenter.cs index 0efb3a37599..fcd6dc43e47 100644 --- a/Xamarin.Forms.Core/MessagingCenter.cs +++ b/Xamarin.Forms.Core/MessagingCenter.cs @@ -6,31 +6,60 @@ namespace Xamarin.Forms { public static class MessagingCenter { - static readonly Dictionary, List>>> s_callbacks = - new Dictionary, List>>>(); + class Sender : Tuple + { + public Sender(string message, Type senderType, Type argType) : base(message, senderType, argType) + { + } + + public string Message => Item1; + public Type SenderType => Item2; + public Type ArgType => Item3; + } + + delegate void Callback(object sender, object args); + + class Subscription : Tuple> + { + protected Subscription(WeakReference subscriber, WeakReference callback) + : base(subscriber, callback) + { + } + + public Subscription(object subscriber, Callback callback) + : this(new WeakReference(subscriber), new WeakReference(callback)) + { + } + + public WeakReference Subscriber => Item1; + public WeakReference Callback => Item2; + } + + static readonly Dictionary> s_subscriptions = + new Dictionary>(); public static void Send(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 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(object subscriber, string message, Action 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 wrap = (sender, args) => + Callback wrap = (sender, args) => { var send = (TSender)sender; if (source == null || send == source) @@ -43,11 +72,11 @@ public static class MessagingCenter public static void Subscribe(object subscriber, string message, Action 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 wrap = (sender, args) => + Callback wrap = (sender, args) => { var send = (TSender)sender; if (source == null || send == source) @@ -69,18 +98,18 @@ public static class MessagingCenter 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(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>> actions = s_callbacks[key]; - if (actions == null || !actions.Any()) + List 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 +117,50 @@ static void InnerSend(string message, Type senderType, Type argType, object send // 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>> actionsCopy = actions.ToList(); - foreach (Tuple> action in actionsCopy) + List 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)) + { + Callback callback; + if(subscription.Callback.TryGetTarget(out callback)) + { + callback(sender, args); + } + } } } - static void InnerSubscribe(object subscriber, string message, Type senderType, Type argType, Action callback) + static void InnerSubscribe(object subscriber, string message, Type senderType, Type argType, Callback callback) { if (message == null) - throw new ArgumentNullException("message"); - var key = new Tuple(message, senderType, argType); - var value = new Tuple>(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, callback); + if (s_subscriptions.ContainsKey(key)) { - s_callbacks[key].Add(value); + s_subscriptions[key].Add(value); } else { - var list = new List>> { value }; - s_callbacks[key] = list; + var list = new List { 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(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(tuple => !tuple.Subscriber.IsAlive || tuple.Subscriber.Target == subscriber); + if (!s_subscriptions[key].Any()) + s_subscriptions.Remove(key); } } } \ No newline at end of file From a7bddd36450d34e062a4c5dc63c52539c30af278 Mon Sep 17 00:00:00 2001 From: "E.Z. Hart" Date: Tue, 6 Dec 2016 17:04:34 -0700 Subject: [PATCH 3/9] Preserve attribute --- .../Xamarin.Forms.Controls.Issues.Shared/Bugzilla45926.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla45926.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla45926.cs index fda0e0b6331..78e4deab108 100644 --- a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla45926.cs +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla45926.cs @@ -72,6 +72,7 @@ public void Issue1Test () #endif } + [Preserve(AllMembers = true)] public class _45926SecondPage : ContentPage { public static int InstanceCounter = 0; From 2aa436546822e6e88cfdaea4e2e44b769134e3ad Mon Sep 17 00:00:00 2001 From: "E.Z. Hart" Date: Tue, 6 Dec 2016 17:12:55 -0700 Subject: [PATCH 4/9] Fix test method name --- .../Xamarin.Forms.Controls.Issues.Shared/Bugzilla45926.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla45926.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla45926.cs index 78e4deab108..0006d2e808f 100644 --- a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla45926.cs +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla45926.cs @@ -56,7 +56,7 @@ protected override void Init() #if UITEST [Test] - public void Issue1Test () + public void Issue45926Test () { RunningApp.WaitForElement (q => q.Marked ("New Page")); From ff8ef8e85715a3e4e83f067e52fe63825c36c67f Mon Sep 17 00:00:00 2001 From: "E.Z. Hart" Date: Thu, 8 Dec 2016 10:12:08 -0700 Subject: [PATCH 5/9] Watch for collection of actual delegate target instead of wrapper delegate --- .../MessagingCenterTests.cs | 101 ++++++++++++++++++ Xamarin.Forms.Core/MessagingCenter.cs | 76 ++++++++----- 2 files changed, 149 insertions(+), 28 deletions(-) diff --git a/Xamarin.Forms.Core.UnitTests/MessagingCenterTests.cs b/Xamarin.Forms.Core.UnitTests/MessagingCenterTests.cs index 46dfdcdbb82..4b76fa1e164 100644 --- a/Xamarin.Forms.Core.UnitTests/MessagingCenterTests.cs +++ b/Xamarin.Forms.Core.UnitTests/MessagingCenterTests.cs @@ -1,4 +1,5 @@ using System; +using System.Runtime.Remoting; using NUnit.Framework; @@ -187,5 +188,105 @@ public void UnsubscribeInCallback () Assert.AreEqual (1, messageCount); } + + [Test] + public void SubscriberShouldBeCollected() + { + new Action(() => + { + var subscriber = new TestSubcriber(); + MessagingCenter.Subscribe(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 CallbackTargetShouldBeCollected() + { + var subscriber = new TestSubcriber(); + new Action(() => + { + var source = new MessagingCenterTestsCallbackSource(); + MessagingCenter.Subscribe(subscriber, "test", p => source.FailCallback()); + })(); + + GC.Collect(); + GC.WaitForPendingFinalizers(); + + var pub = new TestPublisher(); + pub.Test(); // source.FailCallback() shouldn't be called, because the TestCallbackSource object should have ben GCed + } + + [Test] + public void StaticCallback() + { + int i = 4; + + var subscriber = new TestSubcriber(); + + MessagingCenter.Subscribe(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(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 + { + } + + class TestPublisher + { + public void Test() + { + MessagingCenter.Send(this, "test"); + } + } + + public class MessagingCenterTestsCallbackSource + { + public void FailCallback() + { + Assert.Fail(); + } + + public void SuccessCallback(ref bool success) + { + success = true; + } + + public static void Increment(ref int i) + { + i = i + 1; + } + } } } diff --git a/Xamarin.Forms.Core/MessagingCenter.cs b/Xamarin.Forms.Core/MessagingCenter.cs index fcd6dc43e47..1b8d8af4e38 100644 --- a/Xamarin.Forms.Core/MessagingCenter.cs +++ b/Xamarin.Forms.Core/MessagingCenter.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Linq; +using System.Reflection; namespace Xamarin.Forms { @@ -11,28 +12,49 @@ class Sender : Tuple public Sender(string message, Type senderType, Type argType) : base(message, senderType, argType) { } - - public string Message => Item1; - public Type SenderType => Item2; - public Type ArgType => Item3; } - delegate void Callback(object sender, object args); + delegate bool Filter(object sender); - class Subscription : Tuple> + class Subscription : Tuple { - protected Subscription(WeakReference subscriber, WeakReference callback) - : base(subscriber, callback) + public Subscription(object subscriber, object delegateSource, MethodInfo methodInfo, Filter filter) + : base(new WeakReference(subscriber), new WeakReference(delegateSource), methodInfo, filter) { } - public Subscription(object subscriber, Callback callback) - : this(new WeakReference(subscriber), new WeakReference(callback)) + public WeakReference Subscriber => Item1; + WeakReference 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 WeakReference Subscriber => Item1; - public WeakReference Callback => Item2; + public bool CanBeRemoved() + { + return !Subscriber.IsAlive || !DelegateSource.IsAlive; + } } static readonly Dictionary> s_subscriptions = @@ -59,14 +81,15 @@ public Subscription(object subscriber, Callback callback) if (callback == null) throw new ArgumentNullException(nameof(callback)); - Callback 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(object subscriber, string message, Action callback, TSender source = null) where TSender : class @@ -76,14 +99,15 @@ public Subscription(object subscriber, Callback callback) if (callback == null) throw new ArgumentNullException(nameof(callback)); - Callback wrap = (sender, args) => + var target = callback.Target; + + 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(object subscriber, string message) where TSender : class @@ -122,21 +146,17 @@ static void InnerSend(string message, Type senderType, Type argType, object send { if (subscription.Subscriber.Target != null && subcriptions.Contains(subscription)) { - Callback callback; - if(subscription.Callback.TryGetTarget(out callback)) - { - callback(sender, args); - } + subscription.InvokeCallback(sender, args); } } } - static void InnerSubscribe(object subscriber, string message, Type senderType, Type argType, Callback callback) + static void InnerSubscribe(object subscriber, string message, Type senderType, Type argType, object target, MethodInfo methodInfo, Filter filter) { if (message == null) throw new ArgumentNullException(nameof(message)); var key = new Sender(message, senderType, argType); - var value = new Subscription(subscriber, callback); + var value = new Subscription(subscriber, target, methodInfo, filter); if (s_subscriptions.ContainsKey(key)) { s_subscriptions[key].Add(value); @@ -158,7 +178,7 @@ static void InnerUnsubscribe(string message, Type senderType, Type argType, obje var key = new Sender(message, senderType, argType); if (!s_subscriptions.ContainsKey(key)) return; - s_subscriptions[key].RemoveAll(tuple => !tuple.Subscriber.IsAlive || tuple.Subscriber.Target == subscriber); + s_subscriptions[key].RemoveAll(sub => !sub.CanBeRemoved() || sub.Subscriber.Target == subscriber); if (!s_subscriptions[key].Any()) s_subscriptions.Remove(key); } From d64e1e67ce4d220fdca87cb181dd9cb47f0c3b99 Mon Sep 17 00:00:00 2001 From: "E.Z. Hart" Date: Thu, 8 Dec 2016 15:27:27 -0700 Subject: [PATCH 6/9] Preserve the original platform instance when changing main page --- Xamarin.Forms.Platform.iOS/PageExtensions.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/Xamarin.Forms.Platform.iOS/PageExtensions.cs b/Xamarin.Forms.Platform.iOS/PageExtensions.cs index 51a5a33e3c5..78e4f016baa 100644 --- a/Xamarin.Forms.Platform.iOS/PageExtensions.cs +++ b/Xamarin.Forms.Platform.iOS/PageExtensions.cs @@ -10,13 +10,20 @@ public static UIViewController CreateViewController(this Page view) if (!Forms.IsInitialized) throw new InvalidOperationException("call Forms.Init() before this"); + Platform.iOS.Platform currentPlatform = null; + + if (Application.Current.MainPage != null && Application.Current.MainPage.Platform != null) + { + currentPlatform = Application.Current.MainPage.Platform as Platform.iOS.Platform; + } + if (!(view.RealParent is Application)) { Application app = new DefaultApplication(); app.MainPage = view; } - var result = new Platform.iOS.Platform(); + var result = currentPlatform ?? new Platform.iOS.Platform(); result.SetPage(view); return result.ViewController; } From 88f275a3634eeb4bbf263033fda68a50dee1dfef Mon Sep 17 00:00:00 2001 From: "E.Z. Hart" Date: Tue, 13 Dec 2016 10:58:49 -0700 Subject: [PATCH 7/9] Better tests for lambda situations --- .../MessagingCenterTests.cs | 53 ++++++++++++++++++- Xamarin.Forms.Core/MessagingCenter.cs | 3 +- Xamarin.Forms.Platform.iOS/PageExtensions.cs | 9 +--- 3 files changed, 54 insertions(+), 11 deletions(-) diff --git a/Xamarin.Forms.Core.UnitTests/MessagingCenterTests.cs b/Xamarin.Forms.Core.UnitTests/MessagingCenterTests.cs index 4b76fa1e164..3b7d43d53fc 100644 --- a/Xamarin.Forms.Core.UnitTests/MessagingCenterTests.cs +++ b/Xamarin.Forms.Core.UnitTests/MessagingCenterTests.cs @@ -1,8 +1,6 @@ using System; -using System.Runtime.Remoting; using NUnit.Framework; - namespace Xamarin.Forms.Core.UnitTests { [TestFixture] @@ -240,6 +238,36 @@ public void StaticCallback() Assert.IsTrue(i == 5, "The static method should have incremented 'i'"); } + [Test] + public void CompilerGeneratedClosuresAreNotCollected() + { + var source = new MessagingCenterTestsCallbackSource(); + source.SubscribeAnonymousDelegateWithClosure(); + + GC.Collect(); + GC.WaitForPendingFinalizers(); + + var pub = new TestPublisher(); + pub.Test(); + + Assert.True(source.Successful); // the anonymous delegate should be invoked to make success == true + } + + [Test] + public void LambdasAreNotCollected() + { + var source = new MessagingCenterTestsCallbackSource(); + source.SubscribeAnonymousDelegateSansClosure(); + + GC.Collect(); + GC.WaitForPendingFinalizers(); + + var pub = new TestPublisher(); + pub.Test(); + + Assert.True(source.Successful); // the anonymous delegate should be invoked to make success == true + } + [Test] public void NothingShouldBeCollected() { @@ -287,6 +315,27 @@ public static void Increment(ref int i) { i = i + 1; } + + public bool Successful { get; private set; } + + void SetToSuccessful(int x) + { + if (x > 10) + { + Successful = true; + } + } + + public void SubscribeAnonymousDelegateWithClosure() + { + var x = 12; + MessagingCenter.Subscribe(this, "test", p => SetToSuccessful(x)); + } + + public void SubscribeAnonymousDelegateSansClosure() + { + MessagingCenter.Subscribe(this, "test", p => SetToSuccessful(12)); + } } } } diff --git a/Xamarin.Forms.Core/MessagingCenter.cs b/Xamarin.Forms.Core/MessagingCenter.cs index 1b8d8af4e38..dd1ddb0960f 100644 --- a/Xamarin.Forms.Core/MessagingCenter.cs +++ b/Xamarin.Forms.Core/MessagingCenter.cs @@ -2,6 +2,7 @@ using System.Collections.Generic; using System.Linq; using System.Reflection; +using System.Runtime.CompilerServices; namespace Xamarin.Forms { @@ -41,7 +42,7 @@ public void InvokeCallback(object sender, object args) return; } - var target = DelegateSource.Target; + var target = DelegateSource.Target ; if (target == null) { diff --git a/Xamarin.Forms.Platform.iOS/PageExtensions.cs b/Xamarin.Forms.Platform.iOS/PageExtensions.cs index 78e4f016baa..51a5a33e3c5 100644 --- a/Xamarin.Forms.Platform.iOS/PageExtensions.cs +++ b/Xamarin.Forms.Platform.iOS/PageExtensions.cs @@ -10,20 +10,13 @@ public static UIViewController CreateViewController(this Page view) if (!Forms.IsInitialized) throw new InvalidOperationException("call Forms.Init() before this"); - Platform.iOS.Platform currentPlatform = null; - - if (Application.Current.MainPage != null && Application.Current.MainPage.Platform != null) - { - currentPlatform = Application.Current.MainPage.Platform as Platform.iOS.Platform; - } - if (!(view.RealParent is Application)) { Application app = new DefaultApplication(); app.MainPage = view; } - var result = currentPlatform ?? new Platform.iOS.Platform(); + var result = new Platform.iOS.Platform(); result.SetPage(view); return result.ViewController; } From ed8fc78fcced243c035de920568946b06d1a9bda Mon Sep 17 00:00:00 2001 From: "E.Z. Hart" Date: Tue, 13 Dec 2016 13:52:33 -0700 Subject: [PATCH 8/9] Update tests, make callback target a weakreference if it's the subscriber --- .../MessagingCenterTests.cs | 109 ++++++++++-------- Xamarin.Forms.Core/MessagingCenter.cs | 36 +++++- 2 files changed, 93 insertions(+), 52 deletions(-) diff --git a/Xamarin.Forms.Core.UnitTests/MessagingCenterTests.cs b/Xamarin.Forms.Core.UnitTests/MessagingCenterTests.cs index 3b7d43d53fc..12f2fea35cc 100644 --- a/Xamarin.Forms.Core.UnitTests/MessagingCenterTests.cs +++ b/Xamarin.Forms.Core.UnitTests/MessagingCenterTests.cs @@ -204,60 +204,90 @@ public void SubscriberShouldBeCollected() } [Test] - public void CallbackTargetShouldBeCollected() + public void ShouldBeCollectedIfCallbackTargetIsSubscriber() { - var subscriber = new TestSubcriber(); + WeakReference wr = null; + new Action(() => { - var source = new MessagingCenterTestsCallbackSource(); - MessagingCenter.Subscribe(subscriber, "test", p => source.FailCallback()); + var subscriber = new TestSubcriber(); + + wr = new WeakReference(subscriber); + + subscriber.SubscribeToTestMessages(); })(); GC.Collect(); GC.WaitForPendingFinalizers(); var pub = new TestPublisher(); - pub.Test(); // source.FailCallback() shouldn't be called, because the TestCallbackSource object should have ben GCed + pub.Test(); + + Assert.IsFalse(wr.IsAlive); // The Action target and subscriber were the same object, so both could be collected } [Test] - public void StaticCallback() + public void NotCollectedIfSubscriberIsNotTheCallbackTarget() { - int i = 4; + WeakReference wr = null; - var subscriber = new TestSubcriber(); + new Action(() => + { + var subscriber = new TestSubcriber(); - MessagingCenter.Subscribe(subscriber, "test", p => MessagingCenterTestsCallbackSource.Increment(ref i)); + 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(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(i == 5, "The static method should have incremented 'i'"); + 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 CompilerGeneratedClosuresAreNotCollected() + public void SubscriberCollectableAfterUnsubscribeEvenIfHeldByClosure() { - var source = new MessagingCenterTestsCallbackSource(); - source.SubscribeAnonymousDelegateWithClosure(); + WeakReference wr = null; + + new Action(() => + { + var subscriber = new TestSubcriber(); + wr = new WeakReference(subscriber); + + MessagingCenter.Subscribe(subscriber, "test", p => subscriber.SetSuccess()); + })(); + + Assert.IsNotNull(wr.Target as TestSubcriber); + + MessagingCenter.Unsubscribe(wr.Target, "test"); + GC.Collect(); GC.WaitForPendingFinalizers(); - var pub = new TestPublisher(); - pub.Test(); - - Assert.True(source.Successful); // the anonymous delegate should be invoked to make success == true + Assert.IsFalse(wr.IsAlive); // The Action target and subscriber were the same object, so both could be collected } [Test] - public void LambdasAreNotCollected() + public void StaticCallback() { - var source = new MessagingCenterTestsCallbackSource(); - source.SubscribeAnonymousDelegateSansClosure(); + int i = 4; + + var subscriber = new TestSubcriber(); + + MessagingCenter.Subscribe(subscriber, "test", p => MessagingCenterTestsCallbackSource.Increment(ref i)); GC.Collect(); GC.WaitForPendingFinalizers(); @@ -265,7 +295,7 @@ public void LambdasAreNotCollected() var pub = new TestPublisher(); pub.Test(); - Assert.True(source.Successful); // the anonymous delegate should be invoked to make success == true + Assert.IsTrue(i == 5, "The static method should have incremented 'i'"); } [Test] @@ -289,6 +319,17 @@ public void NothingShouldBeCollected() class TestSubcriber { + public void SetSuccess() + { + Successful = true; + } + + public bool Successful { get; private set; } + + public void SubscribeToTestMessages() + { + MessagingCenter.Subscribe(this, "test", p => SetSuccess()); + } } class TestPublisher @@ -301,11 +342,6 @@ public void Test() public class MessagingCenterTestsCallbackSource { - public void FailCallback() - { - Assert.Fail(); - } - public void SuccessCallback(ref bool success) { success = true; @@ -315,27 +351,6 @@ public static void Increment(ref int i) { i = i + 1; } - - public bool Successful { get; private set; } - - void SetToSuccessful(int x) - { - if (x > 10) - { - Successful = true; - } - } - - public void SubscribeAnonymousDelegateWithClosure() - { - var x = 12; - MessagingCenter.Subscribe(this, "test", p => SetToSuccessful(x)); - } - - public void SubscribeAnonymousDelegateSansClosure() - { - MessagingCenter.Subscribe(this, "test", p => SetToSuccessful(12)); - } } } } diff --git a/Xamarin.Forms.Core/MessagingCenter.cs b/Xamarin.Forms.Core/MessagingCenter.cs index dd1ddb0960f..5e60046d09f 100644 --- a/Xamarin.Forms.Core/MessagingCenter.cs +++ b/Xamarin.Forms.Core/MessagingCenter.cs @@ -17,15 +17,41 @@ public Sender(string message, Type senderType, Type argType) : base(message, sen delegate bool Filter(object sender); - class Subscription : Tuple + 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 { public Subscription(object subscriber, object delegateSource, MethodInfo methodInfo, Filter filter) - : base(new WeakReference(subscriber), new WeakReference(delegateSource), methodInfo, filter) + : base(new WeakReference(subscriber), new MaybeWeakReference(subscriber, delegateSource), methodInfo, filter) { } public WeakReference Subscriber => Item1; - WeakReference DelegateSource => Item2; + MaybeWeakReference DelegateSource => Item2; MethodInfo MethodInfo => Item3; Filter Filter => Item4; @@ -42,11 +68,11 @@ public void InvokeCallback(object sender, object args) return; } - var target = DelegateSource.Target ; + var target = DelegateSource.Target; if (target == null) { - return; // Collected + return; // Collected } MethodInfo.Invoke(target, MethodInfo.GetParameters().Length == 1 ? new[] { sender } : new[] { sender, args }); From ab60c3de24f136bd6ae904cee8fc0869d9f796fc Mon Sep 17 00:00:00 2001 From: "E.Z. Hart" Date: Wed, 14 Dec 2016 11:27:51 -0700 Subject: [PATCH 9/9] Ensure old Platform MessagingCenter subs are gone before creating new Platform --- Xamarin.Forms.Platform.iOS/FormsApplicationDelegate.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/Xamarin.Forms.Platform.iOS/FormsApplicationDelegate.cs b/Xamarin.Forms.Platform.iOS/FormsApplicationDelegate.cs index 0aacfd06163..da51718cd21 100644 --- a/Xamarin.Forms.Platform.iOS/FormsApplicationDelegate.cs +++ b/Xamarin.Forms.Platform.iOS/FormsApplicationDelegate.cs @@ -165,9 +165,11 @@ void UpdateMainPage() return; var platformRenderer = (PlatformRenderer)_window.RootViewController; - _window.RootViewController = _application.MainPage.CreateViewController(); + if (platformRenderer != null) ((IDisposable)platformRenderer.Platform).Dispose(); + + _window.RootViewController = _application.MainPage.CreateViewController(); } } } \ No newline at end of file