From d282075a36a7d1aacdaa0db57d4f521fbae0bb9e Mon Sep 17 00:00:00 2001 From: miguel Date: Wed, 29 Aug 2018 22:37:40 -0400 Subject: [PATCH 1/4] [UIKit] UIGestureRecognizer, support a way of unsubscribing without creating cycles This now tracks all the uses of AddTarget with delegates by recording the Token/Selector pair and making `Dispose()` release all the linkage as well as providing an enumerator that can be used to get all the registered Action handlers - this can then be used with .First() and then passed to `RemoveTarget`. This addresses https://github.com/xamarin/xamarin-macios/issues/4190 This initial patch is here for discussion of the approach, want to review and test this before we merge. --- src/UIKit/UIGestureRecognizer.cs | 52 ++++++++++++++++++++------------ src/uikit.cs | 1 + 2 files changed, 33 insertions(+), 20 deletions(-) diff --git a/src/UIKit/UIGestureRecognizer.cs b/src/UIKit/UIGestureRecognizer.cs index 9b9ebb984b7b..3d572850d293 100644 --- a/src/UIKit/UIGestureRecognizer.cs +++ b/src/UIKit/UIGestureRecognizer.cs @@ -12,13 +12,17 @@ using System; using System.Collections; +using System.Collections.Generic; using Foundation; using ObjCRuntime; using CoreGraphics; namespace UIKit { public partial class UIGestureRecognizer { - object recognizers; + // + // Tracks the targets (NSObject, which we always enforce to be Token) to the Selector the point to, used when disposing + // + Dictionary recognizers = new Dictionary (); const string tsel = "target"; internal const string parametrized_selector = "target:"; #if !XAMCORE_2_0 @@ -30,18 +34,26 @@ public partial class UIGestureRecognizer { { } + // Called by the Dispose() method + void OnDispose () + { + foreach (var kv in recognizers) + RemoveTarget (kv.Key, kv.Value); + recognizers = null; + } + // // Signature swapped, this is only used so we can store the "token" in recognizers // public UIGestureRecognizer (Selector sel, Token token) : this (token, sel) { - recognizers = token; + recognizers [token] = sel.Handle; MarkDirty (); } internal UIGestureRecognizer (IntPtr sel, Token token) : this (token, sel) { - recognizers = token; + recognizers [token] = sel; MarkDirty (); } @@ -111,17 +123,7 @@ void RegisterTarget (Token target, IntPtr sel) { AddTarget (target, sel); MarkDirty (); - if (recognizers == null) - recognizers = target; - else { - Hashtable table = recognizers as Hashtable; - if (table == null){ - table = new Hashtable (); - table [recognizers] = recognizers; - recognizers = table; - } - table [target] = target; - } + recognizers [target] = sel; } public void RemoveTarget (Token token) @@ -130,12 +132,22 @@ public void RemoveTarget (Token token) throw new ArgumentNullException ("token"); if (recognizers == null) return; - if (recognizers == token) - recognizers = null; - Hashtable asHash = recognizers as Hashtable; - if (asHash != null) - asHash.Remove (token); - RemoveTarget (token, token is ParametrizedDispatch ? Selector.GetHandle (parametrized_selector) : Selector.GetHandle (tsel)); + if (recognizers.ContainsKey (token)){ + var sel = recognizers [token]; + recognizers.Remove (token); + RemoveTarget (token, sel); + } + } + + // + // Used to enumerate all the registered handlers for this UIGestureRecognizer + // + public IEnumerable GetTargets () + { + if (recognizers == null) + yield break; + foreach (var kv in recognizers) + yield return kv.Key; } } diff --git a/src/uikit.cs b/src/uikit.cs index 19ce1d417717..f94c8444ec5d 100644 --- a/src/uikit.cs +++ b/src/uikit.cs @@ -5868,6 +5868,7 @@ partial interface UIFontDescriptor : NSSecureCoding, NSCopying { #if !WATCH [BaseType (typeof(NSObject), Delegates=new string [] {"WeakDelegate"}, Events=new Type[] {typeof (UIGestureRecognizerDelegate)})] + [Dispose ("OnDispose ();")] interface UIGestureRecognizer { [DesignatedInitializer] [Export ("initWithTarget:action:")] From f4a8e9b96f5ecb50994e38f26e86ccf85dc8b611 Mon Sep 17 00:00:00 2001 From: Rolf Bjarne Kvinge Date: Thu, 13 Sep 2018 12:19:26 +0200 Subject: [PATCH 2/4] Simplify code a little bit. --- src/UIKit/UIGestureRecognizer.cs | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/src/UIKit/UIGestureRecognizer.cs b/src/UIKit/UIGestureRecognizer.cs index 3d572850d293..2943e99a730d 100644 --- a/src/UIKit/UIGestureRecognizer.cs +++ b/src/UIKit/UIGestureRecognizer.cs @@ -132,11 +132,8 @@ public void RemoveTarget (Token token) throw new ArgumentNullException ("token"); if (recognizers == null) return; - if (recognizers.ContainsKey (token)){ - var sel = recognizers [token]; - recognizers.Remove (token); + if (recognizers.Remove (token, out var sel)) RemoveTarget (token, sel); - } } // @@ -144,10 +141,7 @@ public void RemoveTarget (Token token) // public IEnumerable GetTargets () { - if (recognizers == null) - yield break; - foreach (var kv in recognizers) - yield return kv.Key; + return (IEnumerable) recognizers?.Keys ?? Array.Empty (); } } From 440c38ae5ead11dead4e8587410127c68826ab0b Mon Sep 17 00:00:00 2001 From: Rolf Bjarne Kvinge Date: Mon, 15 Oct 2018 17:40:02 +0200 Subject: [PATCH 3/4] Add test. --- .../UIKit/GestureRecognizerTest.cs | 59 +++++++++++++++++++ 1 file changed, 59 insertions(+) diff --git a/tests/monotouch-test/UIKit/GestureRecognizerTest.cs b/tests/monotouch-test/UIKit/GestureRecognizerTest.cs index 0e2f9a1237d9..f607bc9ea231 100644 --- a/tests/monotouch-test/UIKit/GestureRecognizerTest.cs +++ b/tests/monotouch-test/UIKit/GestureRecognizerTest.cs @@ -10,6 +10,7 @@ #if !__WATCHOS__ && !MONOMAC using System; +using System.Collections.Generic; #if XAMCORE_2_0 using Foundation; using UIKit; @@ -37,6 +38,64 @@ public void Null () gr.RemoveTarget (null, null); } } + + [Test] + public void NoStrongCycles () + { + bool finalizedAnyCtor = false; + bool finalizedAnyAddTarget1 = false; + bool finalizedAnyAddTarget2 = false; + + // Add the gesture recognizers to a list so that they're not collected until after the test + // This is to avoid false positives (the callback should be collectible already after disposing the gesture recognizer). + var list = new List (); + + for (var k = 0; k < 10; k++) { + { + var notifier = new FinalizerNotifier (() => finalizedAnyCtor = true); + using (var gr = new UIGestureRecognizer (() => { + GC.KeepAlive (notifier); // Make sure the 'notifier' instance is only collected if the delegate to UIGestureRecognizer is collectable. + })) { + list.Add (gr); + } + } + { + var notifier = new FinalizerNotifier (() => finalizedAnyAddTarget1 = true); + using (var gr = new UIGestureRecognizer ()) { + gr.AddTarget (() => { GC.KeepAlive (notifier); }); + list.Add (gr); + } + } + { + var notifier = new FinalizerNotifier (() => finalizedAnyAddTarget2 = true); + using (var gr = new UIGestureRecognizer ()) { + gr.AddTarget ((obj) => { GC.KeepAlive (notifier); }); + list.Add (gr); + } + } + } + + TestRuntime.RunAsync (DateTime.Now.AddSeconds (1), () => { GC.Collect (); }, () => finalizedAnyCtor && finalizedAnyAddTarget1 && finalizedAnyAddTarget2); + Assert.IsTrue (finalizedAnyCtor, "Any finalized"); + Assert.IsTrue (finalizedAnyAddTarget1, "AddTarget1 finalized"); + Assert.IsTrue (finalizedAnyAddTarget2, "AddTarget2 finalized"); + + GC.KeepAlive (list); + } + + class FinalizerNotifier + { + public Action Action; + public FinalizerNotifier (Action action) + { + Action = action; + } + ~FinalizerNotifier () + { + if (Action != null) + Action (); + } + } } } From 5871aa818ab6a2d559deab1f79f317ef0fa9ce7e Mon Sep 17 00:00:00 2001 From: Rolf Bjarne Kvinge Date: Thu, 18 Oct 2018 09:33:45 +0200 Subject: [PATCH 4/4] [tests] Add an NSAutoreleasePool to make GestureRecognizerTest.NoStrongCycles happy on 32-bit. --- tests/monotouch-test/UIKit/GestureRecognizerTest.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/monotouch-test/UIKit/GestureRecognizerTest.cs b/tests/monotouch-test/UIKit/GestureRecognizerTest.cs index f607bc9ea231..9832001cc8aa 100644 --- a/tests/monotouch-test/UIKit/GestureRecognizerTest.cs +++ b/tests/monotouch-test/UIKit/GestureRecognizerTest.cs @@ -50,6 +50,7 @@ public void NoStrongCycles () // This is to avoid false positives (the callback should be collectible already after disposing the gesture recognizer). var list = new List (); + var pool = new NSAutoreleasePool (); for (var k = 0; k < 10; k++) { { var notifier = new FinalizerNotifier (() => finalizedAnyCtor = true); @@ -74,6 +75,7 @@ public void NoStrongCycles () } } } + pool.Dispose (); TestRuntime.RunAsync (DateTime.Now.AddSeconds (1), () => { GC.Collect (); }, () => finalizedAnyCtor && finalizedAnyAddTarget1 && finalizedAnyAddTarget2); Assert.IsTrue (finalizedAnyCtor, "Any finalized");