[ios] fix memory leak in TimePicker

This is an alternate idea to: dotnet#16265
jonathanpeppers · Jul 21, 2023 · 39618bb · 39618bb
1 parent fe4bc5b
commit 39618bb
Show file tree

Hide file tree

Showing 26 changed files with 144 additions and 41 deletions.
diff --git a/src/Controls/src/Core/Compatibility/Handlers/Android/FrameRenderer.cs b/src/Controls/src/Core/Compatibility/Handlers/Android/FrameRenderer.cs
@@ -423,6 +423,8 @@ void IElementHandler.DisconnectHandler()
 			_viewHandlerWrapper.DisconnectHandler();
 		}
 
+		void IElementHandler.OnWindowChanged(object? oldValue, object? newValue) { }
+
 		#endregion
 	}
 }
diff --git a/src/Controls/src/Core/Compatibility/Handlers/Android/VisualElementRenderer.cs b/src/Controls/src/Core/Compatibility/Handlers/Android/VisualElementRenderer.cs
@@ -70,5 +70,7 @@ protected override void OnMeasure(int widthMeasureSpec, int heightMeasureSpec)
 
 			SetMeasuredDimension(0, 0);
 		}
+
+		void IElementHandler.OnWindowChanged(object? oldValue, object? newValue) { }
 	}
 }
diff --git a/src/Controls/src/Core/Compatibility/Handlers/FlyoutPage/iOS/PhoneFlyoutPageRenderer.cs b/src/Controls/src/Core/Compatibility/Handlers/FlyoutPage/iOS/PhoneFlyoutPageRenderer.cs
@@ -820,6 +820,8 @@ void IElementHandler.DisconnectHandler()
 		{
 			_viewHandlerWrapper.DisconnectHandler();
 		}
+
+		void IElementHandler.OnWindowChanged(object oldValue, object newValue) { }
 		#endregion
 	}
 }
diff --git a/src/Controls/src/Core/Compatibility/Handlers/NavigationPage/iOS/NavigationRenderer.cs b/src/Controls/src/Core/Compatibility/Handlers/NavigationPage/iOS/NavigationRenderer.cs
@@ -1669,6 +1669,8 @@ void IElementHandler.DisconnectHandler()
 			_viewHandlerWrapper.DisconnectHandler();
 		}
 
+		void IElementHandler.OnWindowChanged(object oldValue, object newValue) { }
+
 		internal class MauiControlsNavigationBar : UINavigationBar
 		{
 			[Microsoft.Maui.Controls.Internals.Preserve(Conditional = true)]

diff --git a/src/Controls/src/Core/Compatibility/Handlers/Shell/Android/ShellRenderer.cs b/src/Controls/src/Core/Compatibility/Handlers/Shell/Android/ShellRenderer.cs
@@ -340,5 +340,7 @@ void IElementHandler.DisconnectHandler()
 
 			_disposed = true;
 		}
+
+		void IElementHandler.OnWindowChanged(object oldValue, object newValue) { }
 	}
 }
diff --git a/src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellRenderer.cs b/src/Controls/src/Core/Compatibility/Handlers/Shell/iOS/ShellRenderer.cs
@@ -396,5 +396,7 @@ void IElementHandler.Invoke(string command, object args)
 		void IElementHandler.DisconnectHandler()
 		{
 		}
+
+		void IElementHandler.OnWindowChanged(object oldValue, object newValue) { }
 	}
 }
diff --git a/src/Controls/src/Core/Compatibility/Handlers/TabbedPage/iOS/TabbedRenderer.cs b/src/Controls/src/Core/Compatibility/Handlers/TabbedPage/iOS/TabbedRenderer.cs
@@ -556,6 +556,8 @@ void IElementHandler.DisconnectHandler()
 		{
 			_viewHandlerWrapper.DisconnectHandler();
 		}
+
+		void IElementHandler.OnWindowChanged(object oldValue, object newValue) { }
 		#endregion
 	}
 }
diff --git a/src/Controls/src/Core/Compatibility/Handlers/iOS/VisualElementRenderer.cs b/src/Controls/src/Core/Compatibility/Handlers/iOS/VisualElementRenderer.cs
@@ -90,5 +90,7 @@ void OnSizeChanged(object? sender, EventArgs e)
 		{
 			UpdateNativeWidget();
 		}
+
+		void IElementHandler.OnWindowChanged(object? oldValue, object? newValue) { }
 	}
 }
diff --git a/src/Controls/src/Core/VisualElement/VisualElement.cs b/src/Controls/src/Core/VisualElement/VisualElement.cs
@@ -1877,6 +1877,7 @@ static void OnWindowChanged(BindableObject bindable, object? oldValue, object? n
 			visualElement.UpdatePlatformUnloadedLoadedWiring(newWindow, oldWindow);
 			visualElement.InvalidateStateTriggers(newValue != null);
 			visualElement._windowChanged?.Invoke(visualElement, EventArgs.Empty);
+			visualElement.Handler?.OnWindowChanged(oldValue, newValue);
 		}
 
 		void OnWindowHandlerChanged(object? sender, EventArgs e)

diff --git a/src/Controls/tests/DeviceTests/Elements/TimePicker/TimePickerTests.cs b/src/Controls/tests/DeviceTests/Elements/TimePicker/TimePickerTests.cs
@@ -0,0 +1,49 @@
+using System;
+using System.Threading.Tasks;
+using Microsoft.Maui.Controls;
+using Microsoft.Maui.Handlers;
+using Microsoft.Maui.Hosting;
+using Xunit;
+
+namespace Microsoft.Maui.DeviceTests
+{
+	[Category(TestCategory.TimePicker)]
+	public class TimePickerTests : ControlsHandlerTestBase
+	{
+		void SetupBuilder()
+		{
+			EnsureHandlerCreated(builder =>
+			{
+				builder.ConfigureMauiHandlers(handlers =>
+				{
+					handlers.AddHandler<TimePicker, TimePickerHandler>();
+				});
+			});
+		}
+
+		[Fact(DisplayName = "Does Not Leak")]
+		public async Task DoesNotLeak()
+		{
+			SetupBuilder();
+			WeakReference viewReference = null;
+			WeakReference platformViewReference = null;
+			WeakReference handlerReference = null;
+
+			await InvokeOnMainThreadAsync(() =>
+			{
+				var layout = new Grid();
+				var picker = new TimePicker();
+				layout.Add(picker);
+				var handler = CreateHandler<LayoutHandler>(layout);
+				viewReference = new WeakReference(handler);
+				handlerReference = new WeakReference(picker.Handler);
+				platformViewReference = new WeakReference(picker.Handler.PlatformView);
+			});
+
+			await AssertionExtensions.WaitForGC(viewReference, handlerReference, platformViewReference);
+			Assert.False(viewReference.IsAlive, "TimePicker should not be alive!");
+			Assert.False(handlerReference.IsAlive, "Handler should not be alive!");
+			Assert.False(platformViewReference.IsAlive, "PlatformView should not be alive!");
+		}
+	}
+}
diff --git a/src/Controls/tests/DeviceTests/TestCategory.cs b/src/Controls/tests/DeviceTests/TestCategory.cs
@@ -39,6 +39,7 @@ public static class TestCategory
 		public const string SwipeView = "SwipeView";
 		public const string TabbedPage = "TabbedPage";
 		public const string TextInput = "TextInput";
+		public const string TimePicker = "TimePicker";
 		public const string Toolbar = "Toolbar";
 		public const string TemplatedView = "TemplatedView";
 		public const string View = "View";

diff --git a/src/Core/src/Handlers/Editor/EditorHandler.iOS.cs b/src/Core/src/Handlers/Editor/EditorHandler.iOS.cs
@@ -10,6 +10,10 @@ namespace Microsoft.Maui.Handlers
 	public partial class EditorHandler : ViewHandler<IEditor, MauiTextView>
 	{
 		bool _set;
+#if !MACCATALYST
+		// NOTE: keep the Action alive as long as EditorHandler
+		Action<object>? _onDone;
+#endif
 
 		protected override MauiTextView CreatePlatformView()
 		{
@@ -18,7 +22,7 @@ protected override MauiTextView CreatePlatformView()
 #if !MACCATALYST
 			var accessoryView = new MauiDoneAccessoryView();
 			accessoryView.SetDataContext(this);
-			accessoryView.SetDoneClicked(OnDoneClicked);
+			accessoryView.SetDoneClicked(_onDone ??= OnDoneClicked);
 			platformEditor.InputAccessoryView = accessoryView;
 #endif
 

diff --git a/src/Core/src/Handlers/Element/ElementHandler.cs b/src/Core/src/Handlers/Element/ElementHandler.cs
@@ -130,5 +130,7 @@ void IElementHandler.DisconnectHandler()
 				DisconnectHandler(oldPlatformView);
 			}
 		}
+
+		public virtual void OnWindowChanged(object? oldValue, object? newValue) { }
 	}
 }
diff --git a/src/Core/src/Handlers/IElementHandler.cs b/src/Core/src/Handlers/IElementHandler.cs
@@ -12,6 +12,8 @@ public interface IElementHandler
 
 		void DisconnectHandler();
 
+		void OnWindowChanged(object? oldValue, object? newValue);
+
 		object? PlatformView { get; }
 
 		IElement? VirtualView { get; }

diff --git a/src/Core/src/Handlers/Picker/PickerHandler.iOS.cs b/src/Core/src/Handlers/Picker/PickerHandler.iOS.cs
@@ -10,13 +10,16 @@ public partial class PickerHandler : ViewHandler<IPicker, MauiPicker>
 		UIPickerView? _pickerView;
 
 #if !MACCATALYST
+		// NOTE: keep the Action alive as long as MauiTimePicker
+		Action? _onDone;
+
 		protected override MauiPicker CreatePlatformView()
 		{
 			_pickerView = new UIPickerView();
 
 			var platformPicker = new MauiPicker(_pickerView) { BorderStyle = UITextBorderStyle.RoundedRect };
 			platformPicker.InputView = _pickerView;
-			platformPicker.InputAccessoryView = new MauiDoneAccessoryView(() =>
+			platformPicker.InputAccessoryView = new MauiDoneAccessoryView(_onDone ??= () =>
 			{
 				FinishSelectItem(_pickerView, platformPicker);
 			});

diff --git a/src/Core/src/Handlers/TimePicker/TimePickerHandler.iOS.cs b/src/Core/src/Handlers/TimePicker/TimePickerHandler.iOS.cs
@@ -7,11 +7,7 @@ public partial class TimePickerHandler : ViewHandler<ITimePicker, MauiTimePicker
 	{
 		protected override MauiTimePicker CreatePlatformView()
 		{
-			return new MauiTimePicker(() =>
-			{
-				SetVirtualViewTime();
-				PlatformView?.ResignFirstResponder();
-			});
+			return new MauiTimePicker();
 		}
 
 		internal bool UpdateImmediately { get; set; }
@@ -20,33 +16,37 @@ protected override void ConnectHandler(MauiTimePicker platformView)
 		{
 			base.ConnectHandler(platformView);
 
-			if (platformView != null)
+			platformView?.UpdateTime(VirtualView.Time);
+		}
+
+		protected override void DisconnectHandler(MauiTimePicker platformView)
+		{
+			base.DisconnectHandler(platformView);
+
+			platformView?.RemoveFromSuperview();
+		}
+
+		public override void OnWindowChanged(object? oldValue, object? newValue)
+		{
+			var platformView = PlatformView;
+			if (platformView is null)
+				return;
+
+			if (newValue is null)
 			{
 				platformView.EditingDidBegin += OnStarted;
 				platformView.EditingDidEnd += OnEnded;
 				platformView.ValueChanged += OnValueChanged;
 				platformView.DateSelected += OnDateSelected;
 				platformView.Picker.ValueChanged += OnValueChanged;
-
-				platformView.UpdateTime(VirtualView.Time);
 			}
-		}
-
-		protected override void DisconnectHandler(MauiTimePicker platformView)
-		{
-			base.DisconnectHandler(platformView);
-
-			if (platformView != null)
+			else
 			{
-				platformView.RemoveFromSuperview();
-
 				platformView.EditingDidBegin -= OnStarted;
 				platformView.EditingDidEnd -= OnEnded;
 				platformView.ValueChanged -= OnValueChanged;
 				platformView.DateSelected -= OnDateSelected;
 				platformView.Picker.ValueChanged -= OnValueChanged;
-
-				platformView.Dispose();
 			}
 		}
 

diff --git a/src/Core/src/Platform/iOS/MauiDatePicker.cs b/src/Core/src/Platform/iOS/MauiDatePicker.cs
@@ -7,6 +7,9 @@ namespace Microsoft.Maui.Platform
 	public class MauiDatePicker : NoCaretField
 	{
 #if !MACCATALYST
+		// NOTE: keep the Action alive as long as MauiDatePicker
+		readonly Action<object> _onDone;
+
 		public MauiDatePicker()
 		{
 			BorderStyle = UITextBorderStyle.RoundedRect;
@@ -22,7 +25,7 @@ public MauiDatePicker()
 			this.InputAccessoryView = accessoryView;
 
 			accessoryView.SetDataContext(this);
-			accessoryView.SetDoneClicked(OnDoneClicked);
+			accessoryView.SetDoneClicked(_onDone = OnDoneClicked);
 
 			this.InputView.AutoresizingMask = UIViewAutoresizing.FlexibleHeight;
 			this.InputAccessoryView.AutoresizingMask = UIViewAutoresizing.FlexibleHeight;

diff --git a/src/Core/src/Platform/iOS/MauiDoneAccessoryView.cs b/src/Core/src/Platform/iOS/MauiDoneAccessoryView.cs
@@ -7,26 +7,38 @@ namespace Microsoft.Maui.Platform
 {
 	internal class MauiDoneAccessoryView : UIToolbar
 	{
+		readonly WeakReference<Action>? _doneClicked;
 		WeakReference<object>? _data;
-		Action<object>? _doneClicked;
+		WeakReference<Action<object>>? _doneWithDataClicked;
 
 		public MauiDoneAccessoryView() : base(new CGRect(0, 0, UIScreen.MainScreen.Bounds.Width, 44))
 		{
 			BarStyle = UIBarStyle.Default;
 			Translucent = true;
 			var spacer = new UIBarButtonItem(UIBarButtonSystemItem.FlexibleSpace);
-			var doneButton = new UIBarButtonItem(UIBarButtonSystemItem.Done, OnClicked);
+			var doneButton = new UIBarButtonItem(UIBarButtonSystemItem.Done, OnDataClicked);
 
 			SetItems(new[] { spacer, doneButton }, false);
 		}
 
+		void OnDataClicked(object? sender, EventArgs e)
+		{
+			if (_doneWithDataClicked is not null && _doneWithDataClicked.TryGetTarget(out var handler))
+			{
+				if (_data is not null && _data.TryGetTarget(out var data))
+					handler(data);
+			}
+		}
+
 		void OnClicked(object? sender, EventArgs e)
 		{
-			if (_data?.TryGetTarget(out object? target) == true)
-				_doneClicked?.Invoke(target);
+			if (_doneClicked is not null && _doneClicked.TryGetTarget(out var handler))
+			{
+				handler();
+			}
 		}
 
-		internal void SetDoneClicked(Action<object>? value) => _doneClicked = value;
+		internal void SetDoneClicked(Action<object>? value) => _doneWithDataClicked = value is null ? null : new(value);
 		internal void SetDataContext(object? dataContext)
 		{
 			_data = null;
@@ -38,11 +50,12 @@ internal void SetDataContext(object? dataContext)
 
 		public MauiDoneAccessoryView(Action doneClicked) : base(new CGRect(0, 0, UIScreen.MainScreen.Bounds.Width, 44))
 		{
+			_doneClicked = new(doneClicked);
 			BarStyle = UIBarStyle.Default;
 			Translucent = true;
 
 			var spacer = new UIBarButtonItem(UIBarButtonSystemItem.FlexibleSpace);
-			var doneButton = new UIBarButtonItem(UIBarButtonSystemItem.Done, (o, a) => doneClicked?.Invoke());
+			var doneButton = new UIBarButtonItem(UIBarButtonSystemItem.Done, OnClicked);
 			SetItems(new[] { spacer, doneButton }, false);
 		}
 	}

diff --git a/src/Core/src/Platform/iOS/MauiTimePicker.cs b/src/Core/src/Platform/iOS/MauiTimePicker.cs
@@ -10,20 +10,16 @@ public class MauiTimePicker : NoCaretField
 		readonly UIDatePicker _picker;
 
 #if !MACCATALYST
-		readonly Action _dateSelected;
-		public MauiTimePicker(Action dateSelected)
-#else
-		public MauiTimePicker()
+		// NOTE: keep the Action alive as long as MauiTimePicker
+		readonly Action _onDone;
 #endif
+
+		public MauiTimePicker()
 		{
 			BorderStyle = UITextBorderStyle.RoundedRect;
 
 			_picker = new UIDatePicker { Mode = UIDatePickerMode.Time, TimeZone = new NSTimeZone("UTC") };
 
-#if !MACCATALYST
-			_dateSelected = dateSelected;
-#endif
-
 			if (OperatingSystem.IsIOSVersionAtLeast(14))
 			{
 				_picker.PreferredDatePickerStyle = UIDatePickerStyle.Wheels;
@@ -32,10 +28,10 @@ public MauiTimePicker()
 			InputView = _picker;
 
 #if !MACCATALYST
-			InputAccessoryView = new MauiDoneAccessoryView(() =>
+			InputAccessoryView = new MauiDoneAccessoryView(_onDone = () =>
 			{
 				DateSelected?.Invoke(this, EventArgs.Empty);
-				_dateSelected?.Invoke();
+				ResignFirstResponder();
 			});
 
 			InputAccessoryView.AutoresizingMask = UIViewAutoresizing.FlexibleHeight;

diff --git a/src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt b/src/Core/src/PublicAPI/net-android/PublicAPI.Unshipped.txt
@@ -15,6 +15,7 @@ Microsoft.Maui.ICommandMapper.Invoke(Microsoft.Maui.IElementHandler! viewHandler
 Microsoft.Maui.ICommandMapper<TVirtualView, TViewHandler>
 Microsoft.Maui.ICommandMapper<TVirtualView, TViewHandler>.Add(string! key, System.Action<TViewHandler, TVirtualView, object?>! action) -> void
 Microsoft.Maui.ICommandMapper<TVirtualView, TViewHandler>.Add(string! key, System.Action<TViewHandler, TVirtualView>! action) -> void
+Microsoft.Maui.IElementHandler.OnWindowChanged(object? oldValue, object? newValue) -> void
 Microsoft.Maui.ITextInput.IsSpellCheckEnabled.get -> bool
 Microsoft.Maui.IMenuElement.Accelerators.get -> System.Collections.Generic.IReadOnlyList<Microsoft.Maui.IAccelerator!>?
 Microsoft.Maui.Handlers.IImageSourcePartSetter
@@ -106,6 +107,7 @@ static Microsoft.Maui.SizeRequest.operator ==(Microsoft.Maui.SizeRequest left, M
 *REMOVED*override Microsoft.Maui.Handlers.ViewHandler.SetVirtualView(Microsoft.Maui.IElement! element) -> void
 override Microsoft.Maui.Platform.WrapperView.Visibility.get -> Android.Views.ViewStates
 override Microsoft.Maui.Platform.WrapperView.Visibility.set -> void
+virtual Microsoft.Maui.Handlers.ElementHandler.OnWindowChanged(object? oldValue, object? newValue) -> void
 ~override Microsoft.Maui.Platform.WrapperView.DrawShadow(Android.Graphics.Canvas canvas, int viewWidth, int viewHeight) -> void
 ~override Microsoft.Maui.Platform.WrapperView.GetClipPath(int width, int height) -> Android.Graphics.Path
 *REMOVED*Microsoft.Maui.IContentView.CrossPlatformArrange(Microsoft.Maui.Graphics.Rect bounds) -> Microsoft.Maui.Graphics.Size

diff --git a/src/Core/src/PublicAPI/net-ios/PublicAPI.Shipped.txt b/src/Core/src/PublicAPI/net-ios/PublicAPI.Shipped.txt
@@ -1663,7 +1663,6 @@ Microsoft.Maui.Platform.MauiTextView.VerticalTextAlignment.set -> void
 Microsoft.Maui.Platform.MauiTimePicker
 Microsoft.Maui.Platform.MauiTimePicker.Date.get -> Foundation.NSDate!
 Microsoft.Maui.Platform.MauiTimePicker.DateSelected -> System.EventHandler?
-Microsoft.Maui.Platform.MauiTimePicker.MauiTimePicker(System.Action! dateSelected) -> void
 Microsoft.Maui.Platform.MauiTimePicker.Picker.get -> UIKit.UIDatePicker!
 Microsoft.Maui.Platform.MauiTimePicker.UpdateTime(System.TimeSpan time) -> void
 Microsoft.Maui.Platform.MauiView
-Original file line number
+Diff line change
@@ Expand Up @@
     			SetMeasuredDimension(0, 0);
     		}
+    		void IElementHandler.OnWindowChanged(object? oldValue, object? newValue) { }
     	}
     }