From a589b120ce7d6875b6182ebbf0d7b8a99aba8884 Mon Sep 17 00:00:00 2001 From: Jonathan Peppers Date: Mon, 30 Oct 2023 17:22:53 -0500 Subject: [PATCH] [ios] fix memory leaks in various controls (#18434) Fixes: * `ActivityIndicator` * `BoxView` * `Polygon` * `Polyline` * `IndicatorView` Some of these were based on `PlatformGraphicsView`, which had a circular reference, for example: * `BoxView` -> `BoxViewHandler`/`PlatformGraphicsViewHandler` -> `PlatformGraphicsView` -> `BoxView` * It appears both `IDrawable` and `IGraphicsRenderer` values in `PlatformGraphicsView` were circular references. Various shapes were based on `PlatformGraphicsView`. We don't have the iOS memory analyzer running on `Microsoft.Maui.Graphics.dll` yet. Similarly: * `ActivityIndicator` -> `ActivityIndicatorHandler` -> `MauiActivityIndicator` -> `ActivityIndicator` * `IndicatorView` -> `IndicatorViewHandler` -> `MauiPageControl` -> `IndicatorView` *Open question*: Why didn't the analyzer catch the issues with `ActivityIndicator` and `IndicatorView`? Investigating why. --- .../tests/DeviceTests/Memory/MemoryTests.cs | 14 +++++ .../src/Platform/iOS/MauiActivityIndicator.cs | 15 +++-- src/Core/src/Platform/iOS/MauiPageControl.cs | 19 +++--- .../Platforms/iOS/PlatformGraphicsView.cs | 62 ++++++++++--------- 4 files changed, 64 insertions(+), 46 deletions(-) diff --git a/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs b/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs index 61ce141b7..b0ef9b940 100644 --- a/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs +++ b/src/Controls/tests/DeviceTests/Memory/MemoryTests.cs @@ -1,7 +1,9 @@ using System; using System.Threading.Tasks; using Microsoft.Maui.Controls; +using Microsoft.Maui.Controls.Handlers; using Microsoft.Maui.Controls.Handlers.Compatibility; +using Microsoft.Maui.Controls.Shapes; using Microsoft.Maui.Handlers; using Microsoft.Maui.Hosting; using Xunit; @@ -17,7 +19,9 @@ public class MemoryTests : ControlsHandlerTestBase { builder.ConfigureMauiHandlers(handlers => { + handlers.AddHandler(); handlers.AddHandler(); + handlers.AddHandler(); handlers.AddHandler(); handlers.AddHandler(); handlers.AddHandler(); @@ -26,18 +30,24 @@ public class MemoryTests : ControlsHandlerTestBase handlers.AddHandler(); handlers.AddHandler(); handlers.AddHandler(); + handlers.AddHandler(); + handlers.AddHandler(); handlers.AddHandler(); handlers.AddHandler(); + handlers.AddHandler(); handlers.AddHandler(); handlers.AddHandler(); handlers.AddHandler(); handlers.AddHandler(); + handlers.AddHandler(); }); }); } [Theory("Handler Does Not Leak")] + [InlineData(typeof(ActivityIndicator))] [InlineData(typeof(Border))] + [InlineData(typeof(BoxView))] [InlineData(typeof(ContentView))] [InlineData(typeof(CheckBox))] [InlineData(typeof(DatePicker))] @@ -45,12 +55,16 @@ public class MemoryTests : ControlsHandlerTestBase [InlineData(typeof(Editor))] [InlineData(typeof(GraphicsView))] [InlineData(typeof(Image))] + [InlineData(typeof(IndicatorView))] [InlineData(typeof(Label))] [InlineData(typeof(Picker))] + [InlineData(typeof(Polygon))] + [InlineData(typeof(Polyline))] [InlineData(typeof(RefreshView))] [InlineData(typeof(ScrollView))] [InlineData(typeof(SwipeView))] [InlineData(typeof(TimePicker))] + [InlineData(typeof(WebView))] public async Task HandlerDoesNotLeak(Type type) { SetupBuilder(); diff --git a/src/Core/src/Platform/iOS/MauiActivityIndicator.cs b/src/Core/src/Platform/iOS/MauiActivityIndicator.cs index 99210ef72..32ad6011c 100644 --- a/src/Core/src/Platform/iOS/MauiActivityIndicator.cs +++ b/src/Core/src/Platform/iOS/MauiActivityIndicator.cs @@ -8,18 +8,21 @@ namespace Microsoft.Maui.Platform { public class MauiActivityIndicator : UIActivityIndicatorView, IUIViewLifeCycleEvents { - IActivityIndicator? _virtualView; + readonly WeakReference? _virtualView; + + bool IsRunning => _virtualView is not null && _virtualView.TryGetTarget(out var a) ? a.IsRunning : false; public MauiActivityIndicator(CGRect rect, IActivityIndicator? virtualView) : base(rect) { - _virtualView = virtualView; + if (virtualView is not null) + _virtualView = new(virtualView); } public override void Draw(CGRect rect) { base.Draw(rect); - if (_virtualView?.IsRunning == true) + if (IsRunning) StartAnimating(); else StopAnimating(); @@ -29,7 +32,7 @@ namespace Microsoft.Maui.Platform { base.LayoutSubviews(); - if (_virtualView?.IsRunning == true) + if (IsRunning) StartAnimating(); else StopAnimating(); @@ -38,10 +41,10 @@ namespace Microsoft.Maui.Platform protected override void Dispose(bool disposing) { base.Dispose(disposing); - - _virtualView = null; } + readonly WeakEventManager _weakEventManager = new(); + [UnconditionalSuppressMessage("Memory", "MA0002", Justification = IUIViewLifeCycleEvents.UnconditionalSuppressMessage)] EventHandler? _movedToWindow; event EventHandler IUIViewLifeCycleEvents.MovedToWindow diff --git a/src/Core/src/Platform/iOS/MauiPageControl.cs b/src/Core/src/Platform/iOS/MauiPageControl.cs index f1a26ef50..5afe28c52 100644 --- a/src/Core/src/Platform/iOS/MauiPageControl.cs +++ b/src/Core/src/Platform/iOS/MauiPageControl.cs @@ -1,6 +1,5 @@ using System; using System.Diagnostics.CodeAnalysis; -using System.Reflection.Metadata; using CoreGraphics; using ObjCRuntime; using UIKit; @@ -11,7 +10,7 @@ namespace Microsoft.Maui.Platform { const int DefaultIndicatorSize = 6; - IIndicatorView? _indicatorView; + WeakReference? _indicatorView; bool _updatingPosition; public MauiPageControl() @@ -30,7 +29,7 @@ namespace Microsoft.Maui.Platform { ValueChanged -= MauiPageControlValueChanged; } - _indicatorView = indicatorView; + _indicatorView = indicatorView is null ? null : new(indicatorView); } @@ -81,11 +80,11 @@ namespace Microsoft.Maui.Platform int GetCurrentPage() { - if (_indicatorView == null) + if (_indicatorView is null || !_indicatorView.TryGetTarget(out var indicatorView)) return -1; - var maxVisible = _indicatorView.GetMaximumVisible(); - var position = _indicatorView.Position; + var maxVisible = indicatorView.GetMaximumVisible(); + var position = indicatorView.Position; var index = position >= maxVisible ? maxVisible - 1 : position; return index; } @@ -93,9 +92,9 @@ namespace Microsoft.Maui.Platform public void UpdateIndicatorCount() { - if (_indicatorView == null) + if (_indicatorView is null || !_indicatorView.TryGetTarget(out var indicatorView)) return; - this.UpdatePages(_indicatorView.GetMaximumVisible()); + this.UpdatePages(indicatorView.GetMaximumVisible()); UpdatePosition(); } @@ -136,10 +135,10 @@ namespace Microsoft.Maui.Platform void MauiPageControlValueChanged(object? sender, System.EventArgs e) { - if (_updatingPosition || _indicatorView == null) + if (_updatingPosition || _indicatorView is null || !_indicatorView.TryGetTarget(out var indicatorView)) return; - _indicatorView.Position = (int)CurrentPage; + indicatorView.Position = (int)CurrentPage; //if we are iOS13 or lower and we are using a Square shape //we need to update the CornerRadius of the new shape. if (IsSquare && !(OperatingSystem.IsIOSVersionAtLeast(14) || OperatingSystem.IsTvOSVersionAtLeast(14))) diff --git a/src/Graphics/src/Graphics/Platforms/iOS/PlatformGraphicsView.cs b/src/Graphics/src/Graphics/Platforms/iOS/PlatformGraphicsView.cs index 1252babaa..ceced303c 100644 --- a/src/Graphics/src/Graphics/Platforms/iOS/PlatformGraphicsView.cs +++ b/src/Graphics/src/Graphics/Platforms/iOS/PlatformGraphicsView.cs @@ -1,5 +1,4 @@ using System; -using System.Drawing; using CoreGraphics; using Foundation; using UIKit; @@ -9,9 +8,9 @@ namespace Microsoft.Maui.Graphics.Platform [Register(nameof(PlatformGraphicsView))] public class PlatformGraphicsView : UIView { - private IGraphicsRenderer _renderer; + private WeakReference _renderer; private CGColorSpace _colorSpace; - private IDrawable _drawable; + private WeakReference _drawable; private CGRect _lastBounds; public PlatformGraphicsView(CGRect frame, IDrawable drawable = null, IGraphicsRenderer renderer = null) : base(frame) @@ -35,57 +34,53 @@ namespace Microsoft.Maui.Graphics.Platform public IGraphicsRenderer Renderer { - get => _renderer; + get => _renderer is not null && _renderer.TryGetTarget(out var r) ? r : null; set { - if (_renderer != null) + var renderer = Renderer; + if (renderer != null) { - _renderer.Drawable = null; - _renderer.GraphicsView = null; - _renderer.Dispose(); + renderer.Drawable = null; + renderer.GraphicsView = null; + renderer.Dispose(); } - _renderer = value ?? new DirectRenderer(); + renderer = value ?? new DirectRenderer(); + _renderer = new(renderer); - _renderer.GraphicsView = this; - _renderer.Drawable = _drawable; + renderer.GraphicsView = this; + renderer.Drawable = Drawable; var bounds = Bounds; - _renderer.SizeChanged((float)bounds.Width, (float)bounds.Height); + renderer.SizeChanged((float)bounds.Width, (float)bounds.Height); } } public IDrawable Drawable { - get => _drawable; + get => _drawable is not null && _drawable.TryGetTarget(out var d) ? d : null; set { - _drawable = value; - if (_renderer != null) + _drawable = new(value); + if (Renderer is IGraphicsRenderer renderer) { - _renderer.Drawable = _drawable; - _renderer.Invalidate(); + renderer.Drawable = value; + renderer.Invalidate(); } } } - public void InvalidateDrawable() - { - _renderer.Invalidate(); - } + public void InvalidateDrawable() => Renderer?.Invalidate(); - public void InvalidateDrawable(float x, float y, float w, float h) - { - _renderer.Invalidate(x, y, w, h); - } + public void InvalidateDrawable(float x, float y, float w, float h) => Renderer?.Invalidate(x, y, w, h); public override void WillMoveToSuperview(UIView newSuperview) { base.WillMoveToSuperview(newSuperview); - if (newSuperview == null) + if (newSuperview == null && Renderer is IGraphicsRenderer renderer) { - _renderer.Detached(); + renderer.Detached(); } } @@ -107,7 +102,10 @@ namespace Microsoft.Maui.Graphics.Platform coreGraphics.SetStrokeColorSpace(_colorSpace); coreGraphics.SetPatternPhase(PatternPhase); - _renderer.Draw(coreGraphics, dirtyRect.AsRectangleF()); + if (Renderer is IGraphicsRenderer renderer) + { + renderer.Draw(coreGraphics, dirtyRect.AsRectangleF()); + } } public override CGRect Bounds @@ -120,8 +118,12 @@ namespace Microsoft.Maui.Graphics.Platform if (_lastBounds.Width != newBounds.Width || _lastBounds.Height != newBounds.Height) { base.Bounds = value; - _renderer.SizeChanged((float)newBounds.Width, (float)newBounds.Height); - _renderer.Invalidate(); + + if (Renderer is IGraphicsRenderer renderer) + { + renderer.SizeChanged((float)newBounds.Width, (float)newBounds.Height); + renderer.Invalidate(); + } _lastBounds = newBounds; }