Skip to content

Commit

Permalink
[ios] fix memory leaks in GestureRecognizers
Browse files Browse the repository at this point in the history 
Context: dotnet#20023 (comment)

One of the underlying reasons `RadioButton` appears to leak, is that it
happens to use a `TapGestureRecognizer` internally. When testing,
@tj-devel709 and I found we could add pretty much *any* recognizer to
a simple control like `BoxView`, and it would cause the `BoxView` to
live forever.

I could reproduce this in a new unit test that I setup for each type of
gesture recognizer.

`GesturePlatformManager.iOS.cs` had two "cycles":

* `*Handler` -> ... -> `GesturePlatformManager` -> `*Handler`

* `*GestureRecognizer` -> `GesturePlatformManager` -> `*GestureRecognizer`

To fix these, we can make `GesturePlatformManager` not have strong
references pointing *back* to its parent classes:

* Make `_handler` a `WeakReference<IPlatformViewHandler>`

* Make `_gestureRecognizers` a `ConditionalWeakTable`

With these changes the tests pass, except for `PanGestureRecognizer`,
which had a "closure" in using the `panRecognizer` variable.

I fixed this one with:

* `((PanGestureRecognizer)panGestureRecognizer)` as `panGestureRecognizer`
  was backed by a `WeakReference`.
  • Loading branch information
@jonathanpeppers
jonathanpeppers committed Mar 7, 2024
1 parent 7639a2c commit 8d65cde
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 28 deletions.
65 changes: 37 additions & 28 deletions src/Controls/src/Core/Platform/GestureManager/GesturePlatformManager.iOS.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
using System.Collections.ObjectModel;
using System.Collections.Specialized;
using System.Reflection;
using System.Runtime.CompilerServices;
using System.Runtime.Versioning;
using CoreGraphics;
using Foundation;
Expand All @@ -20,9 +21,9 @@ class GesturePlatformManager : IDisposable
{
readonly NotifyCollectionChangedEventHandler _collectionChangedHandler;

readonly Dictionary<IGestureRecognizer, List<UIGestureRecognizer?>> _gestureRecognizers = new Dictionary<IGestureRecognizer, List<UIGestureRecognizer?>>();
readonly ConditionalWeakTable<IGestureRecognizer, List<UIGestureRecognizer?>> _gestureRecognizers = new();
readonly List<INativeObject> _interactions = new List<INativeObject>();
readonly IPlatformViewHandler _handler;
readonly WeakReference<IPlatformViewHandler> _handler;

bool _disposed;
WeakReference<PlatformView>? _platformView;
Expand All @@ -38,17 +39,18 @@ public GesturePlatformManager(IViewHandler handler)
if (handler == null)
throw new ArgumentNullException(nameof(handler));

_handler = (IPlatformViewHandler)handler;
var platformHandler = (IPlatformViewHandler)handler;

if (_handler?.ToPlatform() is not PlatformView target)
if (platformHandler?.ToPlatform() is not PlatformView target)
throw new ArgumentNullException(nameof(handler.PlatformView));

_handler = new WeakReference<IPlatformViewHandler>(platformHandler);
_platformView = new WeakReference<PlatformView>(target);

_collectionChangedHandler = GestureRecognizersOnCollectionChanged;

// In XF this was called inside ViewDidLoad
if (_handler.VirtualView is View view)
if (platformHandler.VirtualView is View view)
OnElementChanged(this, new VisualElementChangedEventArgs(null, view));
else
throw new ArgumentNullException(nameof(handler.VirtualView));
Expand All @@ -64,8 +66,14 @@ protected virtual PlatformView? PlatformView
}
}

IPlatformViewHandler? Handler => _handler.TryGetTarget(out var handler) ? handler : null;

IView? VirtualView => Handler?.VirtualView;

UIWindow? PlatformWindow => Handler?.MauiContext?.GetPlatformWindow();

ObservableCollection<IGestureRecognizer>? ElementGestureRecognizers =>
(_handler.VirtualView as Element)?.GetCompositeGestureRecognizers() as ObservableCollection<IGestureRecognizer>;
(VirtualView as Element)?.GetCompositeGestureRecognizers() as ObservableCollection<IGestureRecognizer>;

internal void Disconnect()
{
Expand Down Expand Up @@ -142,7 +150,7 @@ static void ProcessRecognizerHandlerTap(
{
var recognizer = weakRecognizer.Target as IGestureRecognizer;
var eventTracker = weakEventTracker.Target as GesturePlatformManager;
var view = eventTracker?._handler?.VirtualView as View;
var view = eventTracker?.VirtualView as View;

WeakReference? weakPlatformRecognizer = null;
if (uITapGestureRecognizer != null)
Expand Down Expand Up @@ -178,7 +186,7 @@ static void ProcessRecognizerHandlerTap(
static Point? CalculatePosition(IElement? element, CGPoint originPoint, WeakReference? weakPlatformRecognizer, WeakReference weakEventTracker)
{
var eventTracker = weakEventTracker.Target as GesturePlatformManager;
var virtualView = eventTracker?._handler?.VirtualView as View;
var virtualView = eventTracker?.VirtualView as View;
var platformRecognizer = weakPlatformRecognizer?.Target as UIGestureRecognizer;
var platformView = element?.ToPlatform();

Expand Down Expand Up @@ -257,7 +265,7 @@ static void ProcessRecognizerHandlerTap(
{
var swipeGestureRecognizer = weakRecognizer.Target as SwipeGestureRecognizer;
var eventTracker = weakEventTracker.Target as GesturePlatformManager;
var view = eventTracker?._handler.VirtualView as View;
var view = eventTracker?.VirtualView as View;

if (swipeGestureRecognizer != null && view != null)
swipeGestureRecognizer.SendSwiped(view, direction);
Expand All @@ -274,7 +282,7 @@ static void ProcessRecognizerHandlerTap(
{
if (weakRecognizer.Target is IPinchGestureController pinchGestureRecognizer &&
weakEventTracker.Target is GesturePlatformManager eventTracker &&
eventTracker._handler?.VirtualView is View view &&
eventTracker?.VirtualView is View view &&
UIApplication.SharedApplication.GetKeyWindow() is UIWindow window)
{
var oldScale = eventTracker._previousScale;
Expand Down Expand Up @@ -332,20 +340,20 @@ weakEventTracker.Target is GesturePlatformManager eventTracker &&
var uiRecognizer = CreatePanRecognizer(panRecognizer.TouchPoints, r =>
{
var eventTracker = weakEventTracker.Target as GesturePlatformManager;
var view = eventTracker?._handler?.VirtualView as View;
var view = eventTracker?.VirtualView as View;

var panGestureRecognizer = weakRecognizer.Target as IPanGestureController;
if (panGestureRecognizer != null && view != null)
{
switch (r.State)
{
case UIGestureRecognizerState.Began:
if (r.NumberOfTouches != panRecognizer.TouchPoints)
if (r.NumberOfTouches != ((PanGestureRecognizer)panGestureRecognizer).TouchPoints)
return;
panGestureRecognizer.SendPanStarted(view, PanGestureRecognizer.CurrentId.Value);
break;
case UIGestureRecognizerState.Changed:
if (r.NumberOfTouches != panRecognizer.TouchPoints)
if (r.NumberOfTouches != ((PanGestureRecognizer)panGestureRecognizer).TouchPoints)
{
r.State = UIGestureRecognizerState.Ended;
panGestureRecognizer.SendPanCompleted(view, PanGestureRecognizer.CurrentId.Value);
Expand All @@ -361,7 +369,7 @@ weakEventTracker.Target is GesturePlatformManager eventTracker &&
PanGestureRecognizer.CurrentId.Increment();
break;
case UIGestureRecognizerState.Ended:
if (r.NumberOfTouches != panRecognizer.TouchPoints)
if (r.NumberOfTouches != ((PanGestureRecognizer)panGestureRecognizer).TouchPoints)
{
panGestureRecognizer.SendPanCompleted(view, PanGestureRecognizer.CurrentId.Value);
PanGestureRecognizer.CurrentId.Increment();
Expand Down Expand Up @@ -412,8 +420,8 @@ UISwipeGestureRecognizer CreateSwipeRecognizer(SwipeDirection direction, Action<
{
if (weakRecognizer.Target is PointerGestureRecognizer pointerGestureRecognizer &&
weakEventTracker.Target is GesturePlatformManager eventTracker &&
eventTracker._handler?.VirtualView is View view &&
eventTracker._handler?.MauiContext?.GetPlatformWindow() is UIWindow window)
eventTracker?.VirtualView is View view &&
eventTracker?.PlatformWindow is UIWindow window)
{
var originPoint = pointerGesture.LocationInView(eventTracker?.PlatformView);
var platformPointerArgs = new PlatformPointerEventArgs(pointerGesture.View, pointerGesture);
Expand Down Expand Up @@ -592,7 +600,7 @@ void LoadRecognizers()
bool dropFound = false;

if (PlatformView != null &&
_handler.VirtualView is View v &&
VirtualView is View v &&
v.TapGestureRecognizerNeedsDelegate() &&
(PlatformView.AccessibilityTraits & UIAccessibilityTrait.Button) != UIAccessibilityTrait.Button)
{
Expand All @@ -615,7 +623,7 @@ _handler.VirtualView is View v &&
{
IGestureRecognizer recognizer = ElementGestureRecognizers[i];

if (_gestureRecognizers.ContainsKey(recognizer))
if (_gestureRecognizers.TryGetValue(recognizer, out _))
continue;

if (TryGetTapGestureRecognizer(recognizer, out TapGestureRecognizer? tapGestureRecognizer) &&
Expand All @@ -637,7 +645,7 @@ _handler.VirtualView is View v &&
if (OperatingSystem.IsIOSVersionAtLeast(11) && recognizer is DragGestureRecognizer)
{
dragFound = true;
_dragAndDropDelegate = _dragAndDropDelegate ?? new DragAndDropDelegate(_handler);
_dragAndDropDelegate = _dragAndDropDelegate ?? new DragAndDropDelegate(Handler);
if (uIDragInteraction == null && PlatformView != null)
{
var interaction = new UIDragInteraction(_dragAndDropDelegate);
Expand All @@ -650,7 +658,7 @@ _handler.VirtualView is View v &&
if (OperatingSystem.IsIOSVersionAtLeast(11) && recognizer is DropGestureRecognizer)
{
dropFound = true;
_dragAndDropDelegate = _dragAndDropDelegate ?? new DragAndDropDelegate(_handler);
_dragAndDropDelegate = _dragAndDropDelegate ?? new DragAndDropDelegate(Handler);
if (uIDropInteraction == null && PlatformView != null)
{
var interaction = new UIDropInteraction(_dragAndDropDelegate);
Expand All @@ -664,7 +672,7 @@ _handler.VirtualView is View v &&
if (nativeRecognizers is null)
continue;

_gestureRecognizers[recognizer] = nativeRecognizers;
_gestureRecognizers.AddOrUpdate(recognizer, nativeRecognizers);
foreach (UIGestureRecognizer? nativeRecognizer in nativeRecognizers)
{
if (nativeRecognizer != null && PlatformView != null)
Expand All @@ -685,18 +693,19 @@ _handler.VirtualView is View v &&
PlatformView.RemoveInteraction(uIDropInteraction);
}

var toRemove = new List<IGestureRecognizer>();
var toRemove = new List<KeyValuePair<IGestureRecognizer, List<UIGestureRecognizer?>>>();

foreach (var key in _gestureRecognizers.Keys)
foreach (var pair in _gestureRecognizers)
{
if (!ElementGestureRecognizers.Contains(key))
toRemove.Add(key);
if (!ElementGestureRecognizers.Contains(pair.Key))
toRemove.Add(pair);
}

for (int i = 0; i < toRemove.Count; i++)
{
IGestureRecognizer gestureRecognizer = toRemove[i];
var uiRecognizers = _gestureRecognizers[gestureRecognizer];
var pair = toRemove[i];
IGestureRecognizer gestureRecognizer = pair.Key;
var uiRecognizers = pair.Value;
_gestureRecognizers.Remove(gestureRecognizer);

foreach (var uiRecognizer in uiRecognizers)
Expand Down Expand Up @@ -741,7 +750,7 @@ void OnTapGestureRecognizerPropertyChanged(object? sender, System.ComponentModel
bool ShouldReceiveTouch(UIGestureRecognizer recognizer, UITouch touch)
{
var platformView = PlatformView;
var virtualView = _handler?.VirtualView;
var virtualView = VirtualView;

if (virtualView == null || platformView == null)
{
Expand Down
30 changes: 30 additions & 0 deletions src/Controls/tests/DeviceTests/Memory/MemoryTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,36 @@ await InvokeOnMainThreadAsync(async () =>
await AssertionExtensions.WaitForGC(viewReference, handlerReference, platformViewReference);
}

[Theory("Gesture Does Not Leak")]
[InlineData(typeof(DragGestureRecognizer))]
[InlineData(typeof(DropGestureRecognizer))]
[InlineData(typeof(PanGestureRecognizer))]
[InlineData(typeof(PinchGestureRecognizer))]
[InlineData(typeof(PointerGestureRecognizer))]
[InlineData(typeof(SwipeGestureRecognizer))]
[InlineData(typeof(TapGestureRecognizer))]
public async Task GestureDoesNotLeak(Type type)
{
SetupBuilder();

WeakReference viewReference = null;
WeakReference platformViewReference = null;
WeakReference handlerReference = null;

await InvokeOnMainThreadAsync(() =>
{
var view = new Label(); // Just using simplest view
view.GestureRecognizers.Add((GestureRecognizer)Activator.CreateInstance(type));

var handler = CreateHandler<LabelHandler>(view);
viewReference = new WeakReference(view);
handlerReference = new WeakReference(view.Handler);
platformViewReference = new WeakReference(view.Handler.PlatformView);
});

await AssertionExtensions.WaitForGC(viewReference, handlerReference, platformViewReference);
}

#if IOS
[Fact]
public async Task ResignFirstResponderTouchGestureRecognizer()
Expand Down

0 comments on commit 8d65cde

Please sign in to comment.