Skip to content

Commit

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

Context: dotnet#23164

Just the same way as `NavigationPage` in dotnet#23164, `TabbedPage` also has
a memory leak caused by the cycle:

* `TabbedPage` -> `TabbedRenderer` -> `VisualElement _element;` -> `TabbedPage

I could add a new `[Theory]` in `MemoryTests.cs` to see the issue.

This PR fixes the memory leak by breaking the cycle in `TabbedRenderer`.

* Ignore test on Windows

I also cleaned up the Task.Delay()
  • Loading branch information
@jonathanpeppers
jonathanpeppers authored Jun 21, 2024
1 parent 07fd2ca commit 4f8918b
Show file tree
Hide file tree
Showing 2 changed files with 78 additions and 40 deletions.
98 changes: 60 additions & 38 deletions src/Controls/src/Core/Compatibility/Handlers/TabbedPage/iOS/TabbedRenderer.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ public class TabbedRenderer : UITabBarController, IPlatformViewHandler
bool? _defaultBarTranslucent;
IMauiContext _mauiContext;
UITabBarAppearance _tabBarAppearance;
VisualElement _element;
WeakReference<VisualElement> _element;

IMauiContext MauiContext => _mauiContext;
public static IPropertyMapper<TabbedPage, TabbedRenderer> Mapper = new PropertyMapper<TabbedPage, TabbedRenderer>(TabbedViewHandler.ViewMapper);
Expand Down Expand Up @@ -57,7 +57,7 @@ protected TabbedPage Tabbed
get { return (TabbedPage)Element; }
}

public VisualElement Element => _viewHandlerWrapper.Element ?? _element;
public VisualElement Element => _viewHandlerWrapper.Element ?? _element?.GetTargetOrDefault();

public event EventHandler<VisualElementChangedEventArgs> ElementChanged;

Expand All @@ -72,11 +72,14 @@ public UIView NativeView
public void SetElement(VisualElement element)
{
_viewHandlerWrapper.SetVirtualView(element, OnElementChanged, false);
_element = element;
_element = element is null ? null : new(element);

FinishedCustomizingViewControllers += HandleFinishedCustomizingViewControllers;
Tabbed.PropertyChanged += OnPropertyChanged;
Tabbed.PagesChanged += OnPagesChanged;
if (element is TabbedPage tabbed)
{
tabbed.PropertyChanged += OnPropertyChanged;
tabbed.PagesChanged += OnPagesChanged;
}

OnPagesChanged(null, new NotifyCollectionChangedEventArgs(NotifyCollectionChangedAction.Reset));

Expand Down Expand Up @@ -134,8 +137,11 @@ protected override void Dispose(bool disposing)

Page?.SendDisappearing();

Tabbed.PropertyChanged -= OnPropertyChanged;
Tabbed.PagesChanged -= OnPagesChanged;
if (Tabbed is TabbedPage tabbed)
{
tabbed.PropertyChanged -= OnPropertyChanged;
tabbed.PagesChanged -= OnPagesChanged;
}

FinishedCustomizingViewControllers -= HandleFinishedCustomizingViewControllers;
}
Expand Down Expand Up @@ -188,8 +194,8 @@ void OnPagesChanged(object sender, NotifyCollectionChangedEventArgs e)
SetControllers();

UIViewController controller = null;
if (Tabbed.CurrentPage != null)
controller = GetViewController(Tabbed.CurrentPage);
if (Tabbed?.CurrentPage is Page currentPage)
controller = GetViewController(currentPage);
if (controller != null && controller != base.SelectedViewController)
base.SelectedViewController = controller;

Expand All @@ -202,7 +208,7 @@ void OnPropertyChanged(object sender, PropertyChangedEventArgs e)
{
if (e.PropertyName == nameof(TabbedPage.CurrentPage))
{
var current = Tabbed.CurrentPage;
var current = Tabbed?.CurrentPage;
if (current == null)
return;

Expand Down Expand Up @@ -244,23 +250,28 @@ public override UIViewController ChildViewControllerForStatusBarHidden()

void UpdateCurrentPagePreferredStatusBarUpdateAnimation()
{
PageUIStatusBarAnimation animation = ((Page)Element).OnThisPlatform().PreferredStatusBarUpdateAnimation();
Tabbed.CurrentPage.OnThisPlatform().SetPreferredStatusBarUpdateAnimation(animation);
if (Page is Page page)
{
PageUIStatusBarAnimation animation = page.OnThisPlatform().PreferredStatusBarUpdateAnimation();
Tabbed?.CurrentPage?.OnThisPlatform().SetPreferredStatusBarUpdateAnimation(animation);
}
}

void UpdatePrefersStatusBarHiddenOnPages()
{
if (Tabbed is not TabbedPage tabbed)
return;
for (var i = 0; i < ViewControllers.Length; i++)
{
Tabbed.GetPageByIndex(i).OnThisPlatform().SetPrefersStatusBarHidden(Tabbed.OnThisPlatform().PrefersStatusBarHidden());
tabbed.GetPageByIndex(i).OnThisPlatform().SetPrefersStatusBarHidden(tabbed.OnThisPlatform().PrefersStatusBarHidden());
}
}

public override UIViewController ChildViewControllerForHomeIndicatorAutoHidden
{
get
{
var current = Tabbed.CurrentPage;
var current = Tabbed?.CurrentPage;
if (current == null)
return null;

Expand All @@ -276,15 +287,19 @@ void UpdatePageSpecifics()

void Reset()
{
if (Tabbed is not TabbedPage tabbed)
return;
var i = 0;
foreach (var page in Tabbed.Children)
foreach (var page in tabbed.Children)
SetupPage(page, i++);
}

void SetControllers()
{
if (Tabbed is not TabbedPage tabbed)
return;
var list = new List<UIViewController>();
var pages = Tabbed.InternalChildren;
var pages = tabbed.InternalChildren;
for (var i = 0; i < pages.Count; i++)
{
var child = pages[i];
Expand Down Expand Up @@ -315,10 +330,10 @@ void TeardownPage(Page page, int index)

void UpdateBarBackgroundColor()
{
if (Tabbed == null || TabBar == null)
if (Tabbed is not TabbedPage tabbed || TabBar == null)
return;

var barBackgroundColor = Tabbed.BarBackgroundColor;
var barBackgroundColor = tabbed.BarBackgroundColor;
var isDefaultColor = barBackgroundColor == null;

if (isDefaultColor && !_barBackgroundColorWasSet)
Expand All @@ -342,20 +357,20 @@ void UpdateBarBackgroundColor()

void UpdateBarBackground()
{
if (Tabbed == null || TabBar == null)
if (Tabbed is not TabbedPage tabbed || TabBar == null)
return;

var barBackground = Tabbed.BarBackground;
var barBackground = tabbed.BarBackground;

TabBar.UpdateBackground(barBackground);
}

void UpdateBarTextColor()
{
if (Tabbed == null || TabBar == null || TabBar.Items == null)
if (Tabbed is not TabbedPage tabbed || TabBar == null || TabBar.Items == null)
return;

var barTextColor = Tabbed.BarTextColor;
var barTextColor = tabbed.BarTextColor;
var isDefaultColor = barTextColor == null;

if (isDefaultColor && !_barTextColorWasSet)
Expand Down Expand Up @@ -395,11 +410,11 @@ void UpdateBarTextColor()

void UpdateBarTranslucent()
{
if (Tabbed == null || TabBar == null || Element == null)
if (Tabbed == null || TabBar == null || Element is not VisualElement element)
return;

_defaultBarTranslucent = _defaultBarTranslucent ?? TabBar.Translucent;
switch (TabbedPageConfiguration.GetTranslucencyMode(Element))
switch (TabbedPageConfiguration.GetTranslucencyMode(element))
{
case TranslucencyMode.Translucent:
TabBar.Translucent = true;
Expand All @@ -415,22 +430,27 @@ void UpdateBarTranslucent()

void UpdateChildrenOrderIndex(UIViewController[] viewControllers)
{
if (Tabbed is not TabbedPage tabbed)
return;
for (var i = 0; i < viewControllers.Length; i++)
{
var originalIndex = -1;
if (int.TryParse(viewControllers[i].TabBarItem.Tag.ToString(), out originalIndex))
{
var page = (Page)Tabbed.InternalChildren[originalIndex];
var page = (Page)tabbed.InternalChildren[originalIndex];
TabbedPage.SetIndex(page, i);
}
}
}

void UpdateCurrentPage()
{
var count = Tabbed.InternalChildren.Count;
var index = (int)SelectedIndex;
((TabbedPage)Element).CurrentPage = index >= 0 && index < count ? Tabbed.GetPageByIndex(index) : null;
if (Tabbed is TabbedPage tabbed)
{
var count = tabbed.InternalChildren.Count;
var index = (int)SelectedIndex;
tabbed.CurrentPage = index >= 0 && index < count ? tabbed.GetPageByIndex(index) : null;
}
}

async void SetTabBarItem(IPlatformViewHandler renderer)
Expand All @@ -442,7 +462,7 @@ async void SetTabBarItem(IPlatformViewHandler renderer)
var icons = await GetIcon(page);
renderer.ViewController.TabBarItem = new UITabBarItem(page.Title, icons?.Item1, icons?.Item2)
{
Tag = Tabbed.Children.IndexOf(page),
Tag = Tabbed?.Children.IndexOf(page) ?? -1,
AccessibilityIdentifier = page.AutomationId
};
icons?.Item1?.Dispose();
Expand All @@ -451,12 +471,12 @@ async void SetTabBarItem(IPlatformViewHandler renderer)

void UpdateSelectedTabColors()
{
if (Tabbed == null || TabBar == null || TabBar.Items == null)
if (Tabbed is not TabbedPage tabbed || TabBar == null || TabBar.Items == null)
return;

if (Tabbed.IsSet(TabbedPage.SelectedTabColorProperty) && Tabbed.SelectedTabColor != null)
if (tabbed.IsSet(TabbedPage.SelectedTabColorProperty) && tabbed.SelectedTabColor != null)
{
TabBar.TintColor = Tabbed.SelectedTabColor.ToPlatform();
TabBar.TintColor = tabbed.SelectedTabColor.ToPlatform();
}
else
{
Expand All @@ -467,8 +487,8 @@ void UpdateSelectedTabColors()
UpdateiOS15TabBarAppearance();
else
{
if (Tabbed.IsSet(TabbedPage.UnselectedTabColorProperty) && Tabbed.UnselectedTabColor != null)
TabBar.UnselectedItemTintColor = Tabbed.UnselectedTabColor.ToPlatform();
if (tabbed.IsSet(TabbedPage.UnselectedTabColorProperty) && tabbed.UnselectedTabColor != null)
TabBar.UnselectedItemTintColor = tabbed.UnselectedTabColor.ToPlatform();
else
TabBar.UnselectedItemTintColor = UITabBar.Appearance.TintColor;
}
Expand Down Expand Up @@ -501,14 +521,16 @@ protected virtual Task<Tuple<UIImage, UIImage>> GetIcon(Page page)
[System.Runtime.Versioning.SupportedOSPlatform("tvos15.0")]
void UpdateiOS15TabBarAppearance()
{
if (Tabbed is not TabbedPage tabbed)
return;
TabBar.UpdateiOS15TabBarAppearance(
ref _tabBarAppearance,
_defaultBarColor,
_defaultBarTextColor,
Tabbed.IsSet(TabbedPage.SelectedTabColorProperty) ? Tabbed.SelectedTabColor : null,
Tabbed.IsSet(TabbedPage.UnselectedTabColorProperty) ? Tabbed.UnselectedTabColor : null,
Tabbed.IsSet(TabbedPage.BarBackgroundColorProperty) ? Tabbed.BarBackgroundColor : null,
Tabbed.IsSet(TabbedPage.BarTextColorProperty) ? Tabbed.BarTextColor : null,
tabbed.IsSet(TabbedPage.SelectedTabColorProperty) ? tabbed.SelectedTabColor : null,
tabbed.IsSet(TabbedPage.UnselectedTabColorProperty) ? tabbed.UnselectedTabColor : null,
tabbed.IsSet(TabbedPage.BarBackgroundColorProperty) ? tabbed.BarBackgroundColor : null,
tabbed.IsSet(TabbedPage.BarTextColorProperty) ? tabbed.BarTextColor : null,
null);
}

Expand Down
20 changes: 18 additions & 2 deletions src/Controls/tests/DeviceTests/Memory/MemoryTests.cs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,10 @@ void SetupBuilder()
handlers.AddHandler<ViewCell, ViewCellRenderer>();
#if IOS || MACCATALYST
handlers.AddHandler<NavigationPage, NavigationRenderer>();
handlers.AddHandler<TabbedPage, TabbedRenderer>();
#else
handlers.AddHandler<NavigationPage, NavigationViewHandler>();
handlers.AddHandler<TabbedPage, TabbedViewHandler>();
#endif
});
});
Expand All @@ -71,8 +73,15 @@ void SetupBuilder()
[Theory("Pages Do Not Leak")]
[InlineData(typeof(ContentPage))]
[InlineData(typeof(NavigationPage))]
[InlineData(typeof(TabbedPage))]
public async Task PagesDoNotLeak(Type type)
{
#if WINDOWS
// FIXME: there is still an issue with TabbedPage on Windows
if (type == typeof(TabbedPage))
return;
#endif

SetupBuilder();

WeakReference viewReference = null;
Expand All @@ -84,13 +93,20 @@ public async Task PagesDoNotLeak(Type type)
await CreateHandlerAndAddToWindow(new Window(navPage), async () =>
{
var page = (Page)Activator.CreateInstance(type);
var pageToWaitFor = page;
if (page is ContentPage contentPage)
{
contentPage.Content = new Label();
}
else if (page is NavigationPage navigationPage)
{
await navigationPage.PushAsync(new ContentPage { Content = new Label() });
pageToWaitFor = new ContentPage { Content = new Label() };
await navigationPage.PushAsync(pageToWaitFor);
}
else if (page is TabbedPage tabbedPage)
{
pageToWaitFor = new ContentPage { Content = new Label() };
tabbedPage.Children.Add(pageToWaitFor);
}
await navPage.Navigation.PushModalAsync(page);
Expand All @@ -100,7 +116,7 @@ await CreateHandlerAndAddToWindow(new Window(navPage), async () =>
platformViewReference = new WeakReference(page.Handler.PlatformView);
// Windows requires Loaded event to fire before unloading
await Task.Delay(500);
await OnLoadedAsync(pageToWaitFor);
await navPage.Navigation.PopModalAsync();
});

Expand Down

0 comments on commit 4f8918b

Please sign in to comment.