From 0608778c92625530171a5d246510ef27d3a3d0cb Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Wed, 17 Apr 2024 14:10:52 -0500 Subject: [PATCH 1/2] [ios/catalyst] fix memory leak in gestures Related: https://github.com/dotnet/maui/pull/21089 Context: https://github.com/jonathanpeppers/iOSMauiCollectionView/commit/547b7fa6774af4cefddcccab66b7111b67cec07e Doing something like this and running on iOS or Catalyst: 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. --- .../GesturePlatformManager.iOS.cs | 75 ++++++++++--------- .../tests/DeviceTests/Memory/MemoryTests.cs | 56 ++++++++++++++ 2 files changed, 96 insertions(+), 35 deletions(-) diff --git a/src/Controls/src/Core/Platform/GestureManager/GesturePlatformManager.iOS.cs b/src/Controls/src/Core/Platform/GestureManager/GesturePlatformManager.iOS.cs index 736a686ecfd4..758342768947 100644 --- a/src/Controls/src/Core/Platform/GestureManager/GesturePlatformManager.iOS.cs +++ b/src/Controls/src/Core/Platform/GestureManager/GesturePlatformManager.iOS.cs @@ -30,7 +30,7 @@ class GesturePlatformManager : IDisposable bool? _defaultAccessibilityRespondsToUserInteraction; double _previousScale = 1.0; - UITouchEventArgs? _shouldReceiveTouch; + ShouldReceiveTouchProxy? _proxy; DragAndDropDelegate? _dragAndDropDelegate; public GesturePlatformManager(IViewHandler handler) @@ -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: @@ -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(); @@ -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; @@ -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); } @@ -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 _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) diff --git a/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs b/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs index 477daad4e72b..b66c4e8bfedd 100644 --- a/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs +++ b/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs @@ -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; @@ -51,6 +52,11 @@ void SetupBuilder() handlers.AddHandler(); handlers.AddHandler(); handlers.AddHandler(); +#if IOS || MACCATALYST + handlers.AddHandler(typeof(NavigationPage), typeof(NavigationRenderer)); +#else + handlers.AddHandler(typeof(NavigationPage), typeof(NavigationViewHandler)); +#endif }); }); } @@ -140,6 +146,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 { 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() From 586ca26c4e06100578698899236b325d553f8c45 Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Thu, 18 Apr 2024 09:18:45 -0500 Subject: [PATCH 2/2] Fix test on Windows --- src/Controls/tests/DeviceTests/Memory/MemoryTests.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs b/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs index b66c4e8bfedd..68908bcafbc3 100644 --- a/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs +++ b/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs @@ -51,11 +51,12 @@ void SetupBuilder() handlers.AddHandler(); handlers.AddHandler(); handlers.AddHandler(); + handlers.AddHandler(); handlers.AddHandler(); #if IOS || MACCATALYST - handlers.AddHandler(typeof(NavigationPage), typeof(NavigationRenderer)); + handlers.AddHandler(); #else - handlers.AddHandler(typeof(NavigationPage), typeof(NavigationViewHandler)); + handlers.AddHandler(); #endif }); });