Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[UIKit] UIGestureRecognizer, support a way of unsubscribing without creating cycles #4729

Merged
merged 6 commits into from
Oct 18, 2018
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 26 additions & 20 deletions src/UIKit/UIGestureRecognizer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -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<Token,IntPtr> recognizers = new Dictionary<Token,IntPtr> ();
const string tsel = "target";
internal const string parametrized_selector = "target:";
#if !XAMCORE_2_0
Expand All @@ -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;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that can cause an NRE here since the check (in this) would be done later.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You mean because the selector might be null? I do not follow the "would be done later" part

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This would call the constructor generated for the init: method, and that one should throw already if either sel or token are null in this case.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, the input can be null, directly or indirectly, e.g. Selector.FromHandle can return null

new UIGestureRecognizer (null, token); would throw the NullReferenceException before it calls this (token, sel) which has the null check (and would throw an ArgumentNullException)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spouliot the other (this) constructor is called first, not last.

MarkDirty ();
}

internal UIGestureRecognizer (IntPtr sel, Token token) : this (token, sel)
{
recognizers = token;
recognizers [token] = sel;
MarkDirty ();
}

Expand Down Expand Up @@ -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)
Expand All @@ -130,12 +132,16 @@ 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.Remove (token, out var sel))
RemoveTarget (token, sel);
}

//
// Used to enumerate all the registered handlers for this UIGestureRecognizer
//
public IEnumerable<Token> GetTargets ()
{
return (IEnumerable<Token>) recognizers?.Keys ?? Array.Empty<Token> ();
}
}

Expand Down
1 change: 1 addition & 0 deletions src/uikit.cs
Original file line number Diff line number Diff line change
Expand Up @@ -5537,6 +5537,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:")]
Expand Down
59 changes: 59 additions & 0 deletions tests/monotouch-test/UIKit/GestureRecognizerTest.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
#if !__WATCHOS__ && !MONOMAC

using System;
using System.Collections.Generic;
#if XAMCORE_2_0
using Foundation;
using UIKit;
Expand Down Expand Up @@ -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<UIGestureRecognizer> ();

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 ();
}
}
}
}

Expand Down