[Android] Memory leak when MasterDetailPage Detail set to NavigationPage (#239)
authorE.Z. Hart <hartez@users.noreply.github.com>
Mon, 27 Jun 2016 15:20:47 +0000 (09:20 -0600)
committerGitHub <noreply@github.com>
Mon, 27 Jun 2016 15:20:47 +0000 (09:20 -0600)
* 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/Xamarin.Forms.ControlGallery.Android.csproj
Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla40955.cs [new file with mode: 0644]
Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Xamarin.Forms.Controls.Issues.Shared.projitems
Xamarin.Forms.Platform.Android/AppCompat/FragmentContainer.cs
Xamarin.Forms.Platform.Android/AppCompat/MasterDetailContainer.cs
Xamarin.Forms.Platform.Android/AppCompat/MasterDetailPageRenderer.cs
Xamarin.Forms.Platform.Android/AppCompat/NavigationPageRenderer.cs
Xamarin.Forms.Platform.Android/Renderers/NavigationRenderer.cs
Xamarin.Forms.sln.DotSettings

index 1c32b5c..25f896f 100644 (file)
@@ -45,7 +45,6 @@
     <AndroidUseSharedRuntime>False</AndroidUseSharedRuntime>
     <AndroidLinkMode>Full</AndroidLinkMode>
     <JavaMaximumHeapSize>1G</JavaMaximumHeapSize>
-    <AndroidLinkSkip />
     <EmbedAssembliesIntoApk>True</EmbedAssembliesIntoApk>
     <BundleAssemblies>False</BundleAssemblies>
     <CustomCommands>
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 (file)
index 0000000..543a4ca
--- /dev/null
@@ -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<MasterPageItem>();
+                               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
index eac49a1..af05253 100644 (file)
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla40185.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla40333.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla31806.cs" />
+    <Compile Include="$(MSBuildThisFileDirectory)Bugzilla40955.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla41078.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla40998.cs" />
     <Compile Include="$(MSBuildThisFileDirectory)Bugzilla41424.cs" />
index 34abfda..3d85ee0 100644 (file)
@@ -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()
index e001389..102bac5 100644 (file)
@@ -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;
                        }
                }
 
index 50e99e5..6740285 100644 (file)
@@ -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)
                        {
index cb4fcdb..c833e0c 100644 (file)
@@ -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<bool> SwitchContentAsync(Page view, bool animated, bool removed = false, bool popToRoot = false)
                {
-                       var activity = (FormsAppCompatActivity)Context;
                        var tcs = new TaskCompletionSource<bool>();
                        Fragment fragment = FragmentContainer.CreateInstance(view);
                        FragmentManager fm = FragmentManager;
index f326cb3..3fbfa09 100644 (file)
@@ -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)
index 3fc7e73..597fd71 100644 (file)
@@ -17,6 +17,8 @@
        <s:Boolean x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/PLACE_CATCH_ON_NEW_LINE/@EntryValue">True</s:Boolean>
        <s:Boolean x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/PLACE_FINALLY_ON_NEW_LINE/@EntryValue">True</s:Boolean>
        <s:Boolean x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/SPACE_AFTER_TYPECAST_PARENTHESES/@EntryValue">False</s:Boolean>
+       <s:Boolean x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/SPACE_BEFORE_IF_PARENTHESES/@EntryValue">True</s:Boolean>
+       
        <s:Boolean x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/SPACE_BEFORE_LOCK_PARENTHESES/@EntryValue">True</s:Boolean>
        <s:Boolean x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/SPACE_BEFORE_METHOD_CALL_PARENTHESES/@EntryValue">False</s:Boolean>
        <s:Boolean x:Key="/Default/CodeStyle/CodeFormatting/CSharpFormat/SPACE_BEFORE_EMPTY_METHOD_CALL_PARENTHESES/@EntryValue">False</s:Boolean>