diff options
5 files changed, 404 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 new file mode 100644 index 00000000..0006d2e8 --- /dev/null +++ b/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 ()); + } + } +}
\ 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 e0af0ca2..36ed04cb 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 @@ -479,6 +479,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"> diff --git a/Xamarin.Forms.Core.UnitTests/MessagingCenterTests.cs b/Xamarin.Forms.Core.UnitTests/MessagingCenterTests.cs index 46dfdcdb..12f2fea3 100644 --- a/Xamarin.Forms.Core.UnitTests/MessagingCenterTests.cs +++ b/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 @@ namespace Xamarin.Forms.Core.UnitTests 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; + } + } } } 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 diff --git a/Xamarin.Forms.Platform.iOS/FormsApplicationDelegate.cs b/Xamarin.Forms.Platform.iOS/FormsApplicationDelegate.cs index 0aacfd06..da51718c 100644 --- a/Xamarin.Forms.Platform.iOS/FormsApplicationDelegate.cs +++ b/Xamarin.Forms.Platform.iOS/FormsApplicationDelegate.cs @@ -165,9 +165,11 @@ namespace Xamarin.Forms.Platform.iOS 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 |