Skip to content

Commit

Permalink
[ios/catalyst] fix memory leak in gestures (#21887)
Browse files Browse the repository at this point in the history 
* [ios/catalyst] fix memory leak in gestures

Related: #21089
Context: jonathanpeppers/iOSMauiCollectionView@547b7fa

Doing something like this and running on iOS or Catalyst:

    <CollectionView.ItemTemplate>
        <DataTemplate>
            <Label Text="{Binding Name}">
                <Label.GestureRecognizers>
                    <TapGestureRecognizer Command="{Binding BindingContext.Command}" />
                </Label.GestureRecognizers>
            </Label>
        </DataTemplate>
    </CollectionView.ItemTemplate>

Causes a memory leak.

I could reproduce this in a test that is a bit more elaborate than #21089,
showing issues for different types of gestures:

* Usage of `ShouldReceiveTouch` creates a cycle:

  * `GesturePlatformManager` -> `UIGestureRecognizer.ShouldReceiveTouch` -> `GesturePlatformManager`

  * We can move `ShouldReceiveTouch` to a "proxy" class, and avoid the cycle.

* `PanGestureRecognizer` has closures that cause the same cycle:

  * `this.PlatformView` -> `eventTracker?.PlatformView`

  * `panRecognizer` -> `((PanGestureRecognizer)panGestureRecognizer)`

  * These already had a `WeakReference` to use instead, but we could just make use of them.

There *may* be a general problem with `CollectionView` in how it doesn't
tear down `GesturePlatformManager` in the same way for children. But
in either case, the problems above are cycles that should be broken.

* Fix test on Windows
  • Loading branch information
@jonathanpeppers
jonathanpeppers authored Apr 20, 2024
1 parent b0e562e commit be17057
Show file tree
Hide file tree
Showing 2 changed files with 97 additions and 35 deletions.
75 changes: 40 additions & 35 deletions src/Controls/src/Core/Platform/GestureManager/GesturePlatformManager.iOS.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ class GesturePlatformManager : IDisposable
bool? _defaultAccessibilityRespondsToUserInteraction;

double _previousScale = 1.0;
UITouchEventArgs? _shouldReceiveTouch;
ShouldReceiveTouchProxy? _proxy;
DragAndDropDelegate? _dragAndDropDelegate;

public GesturePlatformManager(IViewHandler handler)
Expand Down Expand Up @@ -340,19 +340,19 @@ weakEventTracker.Target is GesturePlatformManager eventTracker &&
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);
PanGestureRecognizer.CurrentId.Increment();
return;
}
var translationInView = r.TranslationInView(PlatformView);
var translationInView = r.TranslationInView(eventTracker?.PlatformView);
panGestureRecognizer.SendPan(view, translationInView.X, translationInView.Y, PanGestureRecognizer.CurrentId.Value);
break;
case UIGestureRecognizerState.Cancelled:
Expand All @@ -361,7 +361,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 @@ -564,12 +564,6 @@ void LoadRecognizers()
if (ElementGestureRecognizers == null)
return;

if (_shouldReceiveTouch == null)
{
// Cache this so we don't create a new UITouchEventArgs instance for every recognizer
_shouldReceiveTouch = ShouldReceiveTouch;
}

UIDragInteraction? uIDragInteraction = null;
UIDropInteraction? uIDropInteraction = null;

Expand Down Expand Up @@ -669,7 +663,8 @@ _handler.VirtualView is View v &&
{
if (nativeRecognizer != null && PlatformView != null)
{
nativeRecognizer.ShouldReceiveTouch = _shouldReceiveTouch;
_proxy ??= new ShouldReceiveTouchProxy(this);
nativeRecognizer.ShouldReceiveTouch = _proxy.ShouldReceiveTouch;
PlatformView.AddGestureRecognizer(nativeRecognizer);

}
Expand Down Expand Up @@ -738,37 +733,47 @@ void OnTapGestureRecognizerPropertyChanged(object? sender, System.ComponentModel
LoadRecognizers();
}

bool ShouldReceiveTouch(UIGestureRecognizer recognizer, UITouch touch)
class ShouldReceiveTouchProxy
{
var platformView = PlatformView;
var virtualView = _handler?.VirtualView;
readonly WeakReference<GesturePlatformManager> _manager;

if (virtualView == null || platformView == null)
{
return false;
}
public ShouldReceiveTouchProxy(GesturePlatformManager manager) => _manager = new(manager);

if (virtualView.InputTransparent)
public bool ShouldReceiveTouch(UIGestureRecognizer recognizer, UITouch touch)
{
return false;
}
if (!_manager.TryGetTarget(out var manager))
return false;

if (touch.View == platformView)
{
return true;
}
var platformView = manager.PlatformView;
var virtualView = manager._handler?.VirtualView;

// If the touch is coming from the UIView our handler is wrapping (e.g., if it's
// wrapping a UIView which already has a gesture recognizer), then we should let it through
// (This goes for children of that control as well)
if (virtualView == null || platformView == null)
{
return false;
}

if (touch.View.IsDescendantOfView(platformView) &&
(touch.View.GestureRecognizers?.Length > 0 || platformView.GestureRecognizers?.Length > 0))
{
return true;
}
if (virtualView.InputTransparent)
{
return false;
}

if (touch.View == platformView)
{
return true;
}

return false;
// If the touch is coming from the UIView our handler is wrapping (e.g., if it's
// wrapping a UIView which already has a gesture recognizer), then we should let it through
// (This goes for children of that control as well)

if (touch.View.IsDescendantOfView(platformView) &&
(touch.View.GestureRecognizers?.Length > 0 || platformView.GestureRecognizers?.Length > 0))
{
return true;
}

return false;
}
}

void GestureRecognizersOnCollectionChanged(object? sender, NotifyCollectionChangedEventArgs notifyCollectionChangedEventArgs)
Expand Down
57 changes: 57 additions & 0 deletions src/Controls/tests/DeviceTests/Memory/MemoryTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
using Microsoft.Maui.Controls.Handlers;
using Microsoft.Maui.Controls.Handlers.Compatibility;
using Microsoft.Maui.Controls.Handlers.Items;
using Microsoft.Maui.Controls.Platform;
using Microsoft.Maui.Controls.Shapes;
using Microsoft.Maui.Handlers;
using Microsoft.Maui.Hosting;
Expand Down Expand Up @@ -50,7 +51,13 @@ void SetupBuilder()
handlers.AddHandler<Switch, SwitchHandler>();
handlers.AddHandler<TableView, TableViewRenderer>();
handlers.AddHandler<TimePicker, TimePickerHandler>();
handlers.AddHandler<Toolbar, ToolbarHandler>();
handlers.AddHandler<WebView, WebViewHandler>();
#if IOS || MACCATALYST
handlers.AddHandler<NavigationPage, NavigationRenderer>();
#else
handlers.AddHandler<NavigationPage, NavigationViewHandler>();
#endif
});
});
}
Expand Down Expand Up @@ -140,6 +147,56 @@ 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 handlerReference = null;

var observable = new ObservableCollection<int> { 1 };
var navPage = new NavigationPage(new ContentPage { Title = "Page 1" });

await CreateHandlerAndAddToWindow(new Window(navPage), async () =>
{
await navPage.Navigation.PushAsync(new ContentPage
{
Content = new CollectionView
{
ItemTemplate = new DataTemplate(() =>
{
var view = new Label
{
GestureRecognizers =
{
(GestureRecognizer)Activator.CreateInstance(type)
}
};
view.SetBinding(Label.TextProperty, ".");

viewReference = new WeakReference(view);
handlerReference = new WeakReference(view.Handler);

return view;
}),
ItemsSource = observable
}
});

await navPage.Navigation.PopAsync();
});

await AssertionExtensions.WaitForGC(viewReference, handlerReference);
}

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

0 comments on commit be17057

Please sign in to comment.