From 589adbd3ef145ec85f9fe64eda008251c1cdb745 Mon Sep 17 00:00:00 2001 From: "E.Z. Hart" Date: Mon, 27 Jun 2016 09:20:47 -0600 Subject: [PATCH] [Android] Memory leak when MasterDetailPage Detail set to NavigationPage (#239) * Create repro * Remove unnecessary cast * Add null checks on weak references in PageContainer * Remove master/detail fragments from manager when switching master/detail pages Separate renderer ViewGroup removal from renderer disposal in FragmentContainer Separate PageContainer disposal from renderer disposal in FragmentContainer Remove Drawer Listener for NavigationPageRenderer in Dispose * Fix missing spaces; Add explicit SPACE_BEFORE_IF_PARENTHESES settings to DotSettings file * Remove javascript rules * Remove usage of .ForEach() --- .../Xamarin.Forms.ControlGallery.Android.csproj | 1 - .../Bugzilla40955.cs | 140 +++++++++++++++++++++ .../Xamarin.Forms.Controls.Issues.Shared.projitems | 1 + .../AppCompat/FragmentContainer.cs | 30 +++-- .../AppCompat/MasterDetailContainer.cs | 26 +++- .../AppCompat/MasterDetailPageRenderer.cs | 2 +- .../AppCompat/NavigationPageRenderer.cs | 37 ++++-- .../Renderers/NavigationRenderer.cs | 27 ++-- Xamarin.Forms.sln.DotSettings | 2 + 9 files changed, 234 insertions(+), 32 deletions(-) create mode 100644 Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla40955.cs diff --git a/Xamarin.Forms.ControlGallery.Android/Xamarin.Forms.ControlGallery.Android.csproj b/Xamarin.Forms.ControlGallery.Android/Xamarin.Forms.ControlGallery.Android.csproj index 1c32b5c..25f896f 100644 --- a/Xamarin.Forms.ControlGallery.Android/Xamarin.Forms.ControlGallery.Android.csproj +++ b/Xamarin.Forms.ControlGallery.Android/Xamarin.Forms.ControlGallery.Android.csproj @@ -45,7 +45,6 @@ False Full 1G - True False diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla40955.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla40955.cs new file mode 100644 index 0000000..543a4ca --- /dev/null +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla40955.cs @@ -0,0 +1,140 @@ +using System; +using System.Collections.Generic; +using System.Diagnostics; +using Xamarin.Forms.CustomAttributes; +using Xamarin.Forms.Internals; + +namespace Xamarin.Forms.Controls.Issues +{ + [Preserve(AllMembers = true)] + [Issue(IssueTracker.Bugzilla, 40955, "Memory leak with FormsAppCompatActivity and NavigationPage", PlatformAffected.Android)] + public class Bugzilla40955 : TestMasterDetailPage + { + const string DestructorMessage = "NavigationPageEx Destructor called"; + const string Page1Title = "Page1"; + const string Page2Title = "Page2"; + const string Page3Title = "Page3"; + + protected override void Init() + { + var masterPage = new MasterPage(); + Master = masterPage; + masterPage.ListView.ItemSelected += (sender, e) => + { + var item = e.SelectedItem as MasterPageItem; + if (item != null) + { + Detail = new NavigationPageEx((Page)Activator.CreateInstance(item.TargetType)); + masterPage.ListView.SelectedItem = null; + IsPresented = false; + } + }; + + Detail = new NavigationPageEx(new _409555_Page1()); + } + + [Preserve(AllMembers = true)] + public class MasterPageItem + { + public string IconSource { get; set; } + + public Type TargetType { get; set; } + + public string Title { get; set; } + } + + [Preserve(AllMembers = true)] + public class MasterPage : ContentPage + { + public MasterPage() + { + Title = "Menu"; + ListView = new ListView { VerticalOptions = LayoutOptions.FillAndExpand, SeparatorVisibility = SeparatorVisibility.None }; + + ListView.ItemTemplate = new DataTemplate(() => + { + var ic = new ImageCell(); + ic.SetBinding(TextCell.TextProperty, "Title"); + return ic; + }); + + Content = new StackLayout + { + Children = { ListView } + }; + + var masterPageItems = new List(); + masterPageItems.Add(new MasterPageItem + { + Title = Page1Title, + TargetType = typeof(_409555_Page1) + }); + masterPageItems.Add(new MasterPageItem + { + Title = Page2Title, + TargetType = typeof(_409555_Page2) + }); + masterPageItems.Add(new MasterPageItem + { + Title = Page3Title, + TargetType = typeof(_409555_Page3) + }); + + ListView.ItemsSource = masterPageItems; + } + + public ListView ListView { get; } + } + + [Preserve(AllMembers = true)] + public class NavigationPageEx : NavigationPage + { + public NavigationPageEx(Page root) : base(root) + { + } + + ~NavigationPageEx() + { + Debug.WriteLine(DestructorMessage); + } + } + + [Preserve(AllMembers = true)] + public class _409555_Page1 : ContentPage + { + public _409555_Page1() + { + Title = Page1Title; + Content = new StackLayout { Children = { new Label { Text = "Open the drawer menu and select Page2" } } }; + } + } + + [Preserve(AllMembers = true)] + public class _409555_Page2 : ContentPage + { + public _409555_Page2() + { + Title = Page2Title; + Content = new StackLayout { Children = { new Label { Text = "Open the drawer menu and select Page3" } } }; + } + } + + [Preserve(AllMembers = true)] + public class _409555_Page3 : ContentPage + { + public _409555_Page3() + { + Title = Page3Title; + Content = new StackLayout { Children = { new Label { Text = $"The console should have displayed the text '{DestructorMessage}' at least once. If not, this test has failed." } } }; + } + + protected override void OnAppearing() + { + base.OnAppearing(); + GC.Collect(); + GC.Collect(); + GC.Collect(); + } + } + } +} \ No newline at end of file diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems index eac49a1..af05253 100644 --- a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems @@ -104,6 +104,7 @@ + diff --git a/Xamarin.Forms.Platform.Android/AppCompat/FragmentContainer.cs b/Xamarin.Forms.Platform.Android/AppCompat/FragmentContainer.cs index 34abfda..3d85ee0 100644 --- a/Xamarin.Forms.Platform.Android/AppCompat/FragmentContainer.cs +++ b/Xamarin.Forms.Platform.Android/AppCompat/FragmentContainer.cs @@ -75,25 +75,31 @@ namespace Xamarin.Forms.Platform.Android.AppCompat return null; } - + public override void OnDestroyView() { if (Page != null) { - IVisualElementRenderer renderer = _visualElementRenderer; - PageContainer container = _pageContainer; - - if (container.Handle != IntPtr.Zero && renderer.ViewGroup.Handle != IntPtr.Zero) + if (_visualElementRenderer != null) { - container.RemoveFromParent(); - renderer.ViewGroup.RemoveFromParent(); - Page.ClearValue(Android.Platform.RendererProperty); + if (_visualElementRenderer.ViewGroup.Handle != IntPtr.Zero) + { + _visualElementRenderer.ViewGroup.RemoveFromParent(); + } - container.Dispose(); - renderer.Dispose(); + _visualElementRenderer.Dispose(); } + + if (_pageContainer != null && _pageContainer.Handle != IntPtr.Zero) + { + _pageContainer.RemoveFromParent(); + _pageContainer.Dispose(); + } + + Page?.ClearValue(Android.Platform.RendererProperty); } + _onCreateCallback = null; _visualElementRenderer = null; _pageContainer = null; @@ -108,9 +114,9 @@ namespace Xamarin.Forms.Platform.Android.AppCompat return; if (hidden) - PageController.SendDisappearing(); + PageController?.SendDisappearing(); else - PageController.SendAppearing(); + PageController?.SendAppearing(); } public override void OnPause() diff --git a/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailContainer.cs b/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailContainer.cs index e001389..102bac5 100644 --- a/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailContainer.cs +++ b/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailContainer.cs @@ -12,6 +12,7 @@ namespace Xamarin.Forms.Platform.Android.AppCompat FragmentManager _fragmentManager; readonly bool _isMaster; readonly MasterDetailPage _parent; + Fragment _currentFragment; public MasterDetailContainer(MasterDetailPage parent, bool isMaster, Context context) : base(parent, isMaster, context) { @@ -59,17 +60,30 @@ namespace Xamarin.Forms.Platform.Android.AppCompat if (page == null) { - // Not a NavigationPage or TabbedPage? Just do the normal thing + // The thing we're adding is not a NavigationPage or TabbedPage, so we can just use the old AddChildView + + if (_currentFragment != null) + { + // But first, if the previous occupant of this container was a fragment, we need to remove it properly + FragmentTransaction transaction = FragmentManager.BeginTransaction(); + transaction.DisallowAddToBackStack(); + transaction.Remove(_currentFragment); + transaction.SetTransition((int)FragmentTransit.None); + transaction.Commit(); + + _currentFragment = null; + } + base.AddChildView(childView); } else { // The renderers for NavigationPage and TabbedPage both host fragments, so they need to be wrapped in a // FragmentContainer in order to get isolated fragment management - Fragment fragment = FragmentContainer.CreateInstance(page); var fc = fragment as FragmentContainer; + fc?.SetOnCreateCallback(pc => { _pageContainer = pc; @@ -78,9 +92,17 @@ namespace Xamarin.Forms.Platform.Android.AppCompat FragmentTransaction transaction = FragmentManager.BeginTransaction(); transaction.DisallowAddToBackStack(); + + if (_currentFragment != null) + { + transaction.Remove(_currentFragment); + } + transaction.Add(Id, fragment); transaction.SetTransition((int)FragmentTransit.FragmentOpen); transaction.Commit(); + + _currentFragment = fragment; } } diff --git a/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailPageRenderer.cs b/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailPageRenderer.cs index 50e99e5..6740285 100644 --- a/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailPageRenderer.cs +++ b/Xamarin.Forms.Platform.Android/AppCompat/MasterDetailPageRenderer.cs @@ -268,7 +268,7 @@ namespace Xamarin.Forms.Platform.Android.AppCompat UpdateMaster(); else if (e.PropertyName == "Detail") UpdateDetail(); - else if (e.PropertyName == "IsGestureEnabled") + else if (e.PropertyName == MasterDetailPage.IsGestureEnabledProperty.PropertyName) SetGestureState(); else if (e.PropertyName == MasterDetailPage.IsPresentedProperty.PropertyName) { diff --git a/Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs b/Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs index cb4fcdb..c833e0c 100644 --- a/Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs +++ b/Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs @@ -132,11 +132,14 @@ namespace Xamarin.Forms.Platform.Android.AppCompat if (Element != null) { - for (var i = 0; i < PageController.InternalChildren.Count; i++) + foreach(Element element in PageController.InternalChildren) { - var child = PageController.InternalChildren[i] as VisualElement; + var child = element as VisualElement; if (child == null) + { continue; + } + IVisualElementRenderer renderer = Android.Platform.GetRenderer(child); renderer?.Dispose(); } @@ -148,7 +151,6 @@ namespace Xamarin.Forms.Platform.Android.AppCompat navController.PopToRootRequested -= OnPoppedToRoot; navController.InsertPageBeforeRequested -= OnInsertPageBeforeRequested; navController.RemovePageRequested -= OnRemovePageRequested; - PageController.SendDisappearing(); } if (_toolbarTracker != null) @@ -165,6 +167,13 @@ namespace Xamarin.Forms.Platform.Android.AppCompat _toolbar = null; } + if (_drawerLayout != null && _drawerListener != null) + { + _drawerLayout.RemoveDrawerListener(_drawerListener); + } + + _drawerToggle = null; + Current = null; Device.Info.PropertyChanged -= DeviceInfoPropertyChanged; @@ -237,7 +246,10 @@ namespace Xamarin.Forms.Platform.Android.AppCompat navController.RemovePageRequested += OnRemovePageRequested; // If there is already stuff on the stack we need to push it - ((INavigationPageController)e.NewElement).StackCopy.Reverse().ForEach(p => PushViewAsync(p, false)); + foreach (Page page in navController.StackCopy.Reverse()) + { + PushViewAsync(page, false); + } } } @@ -447,6 +459,9 @@ namespace Xamarin.Forms.Platform.Android.AppCompat RemovePage(e.Page); } + private DrawerMultiplexedListener _drawerListener; + private DrawerLayout _drawerLayout; + void RegisterToolbar() { Context context = Context; @@ -478,13 +493,20 @@ namespace Xamarin.Forms.Platform.Android.AppCompat if (renderer == null) return; - var drawerLayout = (DrawerLayout)renderer; - _drawerToggle = new ActionBarDrawerToggle((Activity)context, drawerLayout, bar, global::Android.Resource.String.Ok, global::Android.Resource.String.Ok) + _drawerLayout = (DrawerLayout)renderer; + _drawerToggle = new ActionBarDrawerToggle((Activity)context, _drawerLayout, bar, global::Android.Resource.String.Ok, global::Android.Resource.String.Ok) { ToolbarNavigationClickListener = new ClickListener(Element) }; - drawerLayout.AddDrawerListener(new DrawerMultiplexedListener { Listeners = { _drawerToggle, renderer } }); + if (_drawerListener != null) + { + _drawerLayout.RemoveDrawerListener(_drawerListener); + } + + _drawerListener = new DrawerMultiplexedListener { Listeners = { _drawerToggle, renderer } }; + _drawerLayout.AddDrawerListener(_drawerListener); + _drawerToggle.DrawerIndicatorEnabled = true; } @@ -542,7 +564,6 @@ namespace Xamarin.Forms.Platform.Android.AppCompat Task SwitchContentAsync(Page view, bool animated, bool removed = false, bool popToRoot = false) { - var activity = (FormsAppCompatActivity)Context; var tcs = new TaskCompletionSource(); Fragment fragment = FragmentContainer.CreateInstance(view); FragmentManager fm = FragmentManager; diff --git a/Xamarin.Forms.Platform.Android/Renderers/NavigationRenderer.cs b/Xamarin.Forms.Platform.Android/Renderers/NavigationRenderer.cs index f326cb3..3fbfa09 100644 --- a/Xamarin.Forms.Platform.Android/Renderers/NavigationRenderer.cs +++ b/Xamarin.Forms.Platform.Android/Renderers/NavigationRenderer.cs @@ -14,6 +14,7 @@ namespace Xamarin.Forms.Platform.Android static ViewPropertyAnimator s_currentAnimation; Page _current; + bool _disposed; public NavigationRenderer() { @@ -39,17 +40,24 @@ namespace Xamarin.Forms.Platform.Android protected override void Dispose(bool disposing) { - if (disposing) + if (disposing && !_disposed) { - foreach (VisualElement child in PageController.InternalChildren) - { - IVisualElementRenderer renderer = Platform.GetRenderer(child); - if (renderer != null) - renderer.Dispose(); - } + _disposed = true; if (Element != null) { + foreach (Element element in PageController.InternalChildren) + { + var child = (VisualElement)element; + if (child == null) + { + continue; + } + + IVisualElementRenderer renderer = Platform.GetRenderer(child); + renderer?.Dispose(); + } + var navController = (INavigationPageController)Element; navController.PushRequested -= OnPushed; @@ -101,7 +109,10 @@ namespace Xamarin.Forms.Platform.Android newNavController.RemovePageRequested += OnRemovePageRequested; // If there is already stuff on the stack we need to push it - newNavController.StackCopy.Reverse().ForEach(p => PushViewAsync(p, false)); + foreach(Page page in newNavController.StackCopy.Reverse()) + { + PushViewAsync(page, false); + } } protected override void OnLayout(bool changed, int l, int t, int r, int b) diff --git a/Xamarin.Forms.sln.DotSettings b/Xamarin.Forms.sln.DotSettings index 3fc7e73..597fd71 100644 --- a/Xamarin.Forms.sln.DotSettings +++ b/Xamarin.Forms.sln.DotSettings @@ -17,6 +17,8 @@ True True False + True + True False False -- 2.7.4