Skip to content

Commit

Permalink
[ios] fix memory leak in SwipeView (#16532)
Browse files Browse the repository at this point in the history 
Context: #16346

This addresses the memory leak discovered by:

    src/Core/src/Platform/iOS/MauiSwipeView.cs(20,10): error MA0002: Member '_contentView' could cause memory leaks in an NSObject subclass. Remove the member, store the value as a WeakReference, or add the [UnconditionalSuppressMessage("Memory", "MA0002")] attribute with a justification as to why the member will not leak.
    src/Core/src/Platform/iOS/MauiSwipeView.cs(21,15): error MA0002: Member '_actionView' could cause memory leaks in an NSObject subclass. Remove the member, store the value as a WeakReference, or add the [UnconditionalSuppressMessage("Memory", "MA0002")] attribute with a justification as to why the member will not leak.
    src/Core/src/Platform/iOS/MauiSwipeView.cs(18,26): error MA0002: Member '_tapGestureRecognizer' could cause memory leaks in an NSObject subclass. Remove the member, store the value as a WeakReference, or add the [UnconditionalSuppressMessage("Memory", "MA0002")] attribute with a justification as to why the member will not leak.
    src/Core/src/Platform/iOS/MauiSwipeView.cs(19,26): error MA0002: Member '_panGestureRecognizer' could cause memory leaks in an NSObject subclass. Remove the member, store the value as a WeakReference, or add the [UnconditionalSuppressMessage("Memory", "MA0002")] attribute with a justification as to why the member will not leak.

I could reproduce a leak in `MemoryTests.cs`:

    ++[InlineData(typeof(SwipeView))]
    public async Task HandlerDoesNotLeak(Type type)

Solved the problem by:

* Remove the backing field for `MauiSwipeView.Element`. We can use
  `CrossPlatformLayout`, which is already safe, and just cast to
  `ISwipeView` instead.

* Create a `SwipeRecognizerProxy` type for handling callbacks from
  gesture recognizers.

Other cleanup:

* Marked fields `readonly` as Roslyn suggested.

* Fixed a place `.Count() > 0` was used where Roslyn suggested to use
  `.Any()` instead.

* `GetIsVisible()` can also be `static` to avoid a closure on `this`:

    Any(s => this.GetIsVisible(s))
  • Loading branch information
@jonathanpeppers
jonathanpeppers committed Aug 4, 2023
1 parent 41af98e commit 45a5815
Show file tree
Hide file tree
Showing 3 changed files with 75 additions and 68 deletions.
2 changes: 2 additions & 0 deletions src/Controls/tests/DeviceTests/Memory/MemoryTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ void SetupBuilder()
handlers.AddHandler<Image, ImageHandler>();
handlers.AddHandler<RefreshView, RefreshViewHandler>();
handlers.AddHandler<IScrollView, ScrollViewHandler>();
handlers.AddHandler<SwipeView, SwipeViewHandler>();
});
});
}
Expand All @@ -38,6 +39,7 @@ void SetupBuilder()
[InlineData(typeof(Label))]
[InlineData(typeof(RefreshView))]
[InlineData(typeof(ScrollView))]
[InlineData(typeof(SwipeView))]
public async Task HandlerDoesNotLeak(Type type)
{
SetupBuilder();
Expand Down
12 changes: 1 addition & 11 deletions src/Core/src/Handlers/SwipeView/SwipeViewHandler.iOS.cs
View file
Original file line number Diff line number Diff line change
@@ -1,26 +1,16 @@
using System;
using System.Threading.Tasks;
using UIKit;

namespace Microsoft.Maui.Handlers
{
public partial class SwipeViewHandler : ViewHandler<ISwipeView, MauiSwipeView>
{
protected override MauiSwipeView CreatePlatformView()
{
return new MauiSwipeView
{
CrossPlatformLayout = VirtualView,
Element = VirtualView
};
}
protected override MauiSwipeView CreatePlatformView() => new() { CrossPlatformLayout = VirtualView };

public override void SetVirtualView(IView view)
{
base.SetVirtualView(view);
_ = PlatformView ?? throw new InvalidOperationException($"{nameof(PlatformView)} should have been set by base class.");

PlatformView.Element = VirtualView;
PlatformView.CrossPlatformLayout = VirtualView;
}

Expand Down
129 changes: 72 additions & 57 deletions src/Core/src/Platform/iOS/MauiSwipeView.cs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
using System;
using System.Collections.Generic;
using System.Diagnostics.CodeAnalysis;
using System.Linq;
using CoreGraphics;
using Foundation;
Expand All @@ -14,10 +15,15 @@ public class MauiSwipeView : ContentView
const float OpenSwipeThresholdPercentage = 0.6f; // 60%
const double SwipeAnimationDuration = 0.2;

readonly SwipeRecognizerProxy _proxy;
readonly Dictionary<ISwipeItem, object> _swipeItems;
UITapGestureRecognizer _tapGestureRecognizer;
UIPanGestureRecognizer _panGestureRecognizer;
[UnconditionalSuppressMessage("Memory", "MA0002", Justification = "Proven safe in test: MemoryTests.DoesNotLeak")]
readonly UITapGestureRecognizer _tapGestureRecognizer;
[UnconditionalSuppressMessage("Memory", "MA0002", Justification = "Proven safe in test: MemoryTests.DoesNotLeak")]
readonly UIPanGestureRecognizer _panGestureRecognizer;
[UnconditionalSuppressMessage("Memory", "MA0002", Justification = "Proven safe in test: MemoryTests.DoesNotLeak")]
UIView _contentView;
[UnconditionalSuppressMessage("Memory", "MA0002", Justification = "Proven safe in test: MemoryTests.DoesNotLeak")]
UIStackView _actionView;
SwipeTransitionMode _swipeTransitionMode;
SwipeDirection? _swipeDirection;
Expand All @@ -33,34 +39,34 @@ public class MauiSwipeView : ContentView
bool _isResettingSwipe;
bool _isOpen;
OpenSwipeItem _previousOpenSwipeItem;
internal ISwipeView? Element { get; set; }

internal ISwipeView? Element => CrossPlatformLayout as ISwipeView;

public MauiSwipeView()
{
_proxy = new(this);
_swipeItemsRect = new List<CGRect>();
_contentView = new UIView();
_actionView = new UIStackView();
_swipeItems = new Dictionary<ISwipeItem, object>();
_isScrollEnabled = true;

_tapGestureRecognizer = new UITapGestureRecognizer(HandleTap)
_tapGestureRecognizer = new UITapGestureRecognizer(_proxy.HandleTap)
{
CancelsTouchesInView = false,
DelaysTouchesBegan = false,
DelaysTouchesEnded = false
DelaysTouchesEnded = false,
ShouldReceiveTouch = _proxy.OnShouldReceiveTouch,
};

_tapGestureRecognizer.ShouldReceiveTouch = OnShouldReceiveTouch;

_panGestureRecognizer = new UIPanGestureRecognizer(HandlePan)
_panGestureRecognizer = new UIPanGestureRecognizer(_proxy.HandlePan)
{
CancelsTouchesInView = false,
DelaysTouchesBegan = false,
DelaysTouchesEnded = false
DelaysTouchesEnded = false,
ShouldRecognizeSimultaneously = (recognizer, gestureRecognizer) => true,
};

_panGestureRecognizer.ShouldRecognizeSimultaneously = (recognizer, gestureRecognizer) => true;

AddGestureRecognizer(_tapGestureRecognizer);
AddGestureRecognizer(_panGestureRecognizer);
}
Expand Down Expand Up @@ -144,11 +150,6 @@ public override UIView HitTest(CGPoint point, UIEvent? uievent)
return null;
}

bool OnShouldReceiveTouch(UIGestureRecognizer recognizer, UITouch touch)
{
return _swipeOffset != 0;
}

internal void UpdateContent(ISwipeView swipeView, IMauiContext mauiContext)
{
ClipsToBounds = true;
Expand All @@ -171,63 +172,77 @@ internal void UpdateContent(ISwipeView swipeView, IMauiContext mauiContext)
BringSubviewToFront(_contentView);
}

void HandleTap()
class SwipeRecognizerProxy
{
if (_tapGestureRecognizer == null)
return;
readonly WeakReference<MauiSwipeView> _view;

if (_isSwiping)
return;
public SwipeRecognizerProxy(MauiSwipeView view) => _view = new(view);

var state = _tapGestureRecognizer.State;
public bool OnShouldReceiveTouch(UIGestureRecognizer recognizer, UITouch touch)
{
return _view.TryGetTarget(out var view) && view._swipeOffset != 0;
}

if (state != UIGestureRecognizerState.Cancelled)
public void HandleTap(UITapGestureRecognizer recognizer)
{
if (_contentView == null)
if (!_view.TryGetTarget(out var view))
return;

var point = _tapGestureRecognizer.LocationInView(this);
if (view._isSwiping)
return;

if (_isOpen)
var state = recognizer.State;
if (state != UIGestureRecognizerState.Cancelled)
{
if (!TouchInsideContent(point))
ProcessTouchSwipeItems(point);
else
ResetSwipe();
if (view._contentView == null)
return;

var point = recognizer.LocationInView(view);

if (view._isOpen)
{
if (!view.TouchInsideContent(point))
view.ProcessTouchSwipeItems(point);
else
view.ResetSwipe();
}
}
}
}

void HandlePan(UIPanGestureRecognizer panGestureRecognizer)
{
if (_isSwipeEnabled && panGestureRecognizer != null)
public void HandlePan(UIPanGestureRecognizer panGestureRecognizer)
{
CGPoint point = panGestureRecognizer.LocationInView(this);
var navigationController = GetUINavigationController(GetViewController());
if (!_view.TryGetTarget(out var view))
return;

switch (panGestureRecognizer.State)
if (view._isSwipeEnabled && panGestureRecognizer != null)
{
case UIGestureRecognizerState.Began:
if (navigationController != null)
navigationController.InteractivePopGestureRecognizer.Enabled = false;
CGPoint point = panGestureRecognizer.LocationInView(view);
var navigationController = view.GetUINavigationController(view.GetViewController());

HandleTouchInteractions(GestureStatus.Started, point);
break;
case UIGestureRecognizerState.Changed:
HandleTouchInteractions(GestureStatus.Running, point);
break;
case UIGestureRecognizerState.Ended:
if (navigationController != null)
navigationController.InteractivePopGestureRecognizer.Enabled = true;
switch (panGestureRecognizer.State)
{
case UIGestureRecognizerState.Began:
if (navigationController != null)
navigationController.InteractivePopGestureRecognizer.Enabled = false;

HandleTouchInteractions(GestureStatus.Completed, point);
break;
case UIGestureRecognizerState.Cancelled:
if (navigationController != null)
navigationController.InteractivePopGestureRecognizer.Enabled = true;
view.HandleTouchInteractions(GestureStatus.Started, point);
break;
case UIGestureRecognizerState.Changed:
view.HandleTouchInteractions(GestureStatus.Running, point);
break;
case UIGestureRecognizerState.Ended:
if (navigationController != null)
navigationController.InteractivePopGestureRecognizer.Enabled = true;

HandleTouchInteractions(GestureStatus.Canceled, point);
break;
view.HandleTouchInteractions(GestureStatus.Completed, point);
break;
case UIGestureRecognizerState.Cancelled:
if (navigationController != null)
navigationController.InteractivePopGestureRecognizer.Enabled = true;

view.HandleTouchInteractions(GestureStatus.Canceled, point);
break;
}
}
}
}
Expand Down Expand Up @@ -261,7 +276,7 @@ bool IsHorizontalSwipe()

bool IsValidSwipeItems(ISwipeItems? swipeItems)
{
return swipeItems != null && swipeItems.Where(s => GetIsVisible(s)).Count() > 0;
return swipeItems != null && swipeItems.Any(GetIsVisible);
}

void UpdateSwipeItems()
Expand Down Expand Up @@ -854,7 +869,7 @@ void SwipeToThreshold(bool animated = true)
}

}
bool GetIsVisible(ISwipeItem swipeItem)
static bool GetIsVisible(ISwipeItem swipeItem)
{
if (swipeItem is IView view)
return view.Visibility == Maui.Visibility.Visible;
Expand Down

0 comments on commit 45a5815

Please sign in to comment.