diff options
author | adrianknight89 <adrianknight89@outlook.com> | 2017-01-26 08:24:31 -0600 |
---|---|---|
committer | Kangho Hur <kangho.hur@samsung.com> | 2017-03-24 13:15:52 +0900 |
commit | 4ad3748724c4c36e99fa55e8dc8c2626aa27625b (patch) | |
tree | 070725853971caf8e7465392321d266f22883ca0 | |
parent | 1bec0cd791b2a7288fcae5232b2fad7371fbcf9d (diff) | |
download | xamarin-forms-4ad3748724c4c36e99fa55e8dc8c2626aa27625b.tar.gz xamarin-forms-4ad3748724c4c36e99fa55e8dc8c2626aa27625b.tar.bz2 xamarin-forms-4ad3748724c4c36e99fa55e8dc8c2626aa27625b.zip |
[iOS/Critical] Fix ListView memory leaks (#524)
* fix memory leaks
added sample code
changed page name
Changed page title
add screen instructions
fix copy paste error
change subview dispose logic
Fix context action memory leak
add sample code
change custom page names
Revert "change custom page names"
This reverts commit 7aaab2625d9526080ca5c51c02e909f09ee3f610.
changed class names
changes
UI test for 32206
update ui test
fix whitespace
UITests done
* run UI tests on iOS only
* fix code style
* dispose prototype
* fix child renderer issue
* add null check
6 files changed, 414 insertions, 49 deletions
diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla32206.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla32206.cs new file mode 100644 index 00000000..428538f6 --- /dev/null +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla32206.cs @@ -0,0 +1,142 @@ +using System; +using System.Collections.Generic; +using Xamarin.Forms.CustomAttributes; +using Xamarin.Forms.Internals; +using System.Threading; + +#if UITEST +using Xamarin.UITest; +using NUnit.Framework; +#endif + +namespace Xamarin.Forms.Controls.Issues +{ + [Preserve(AllMembers = true)] + [Issue(IssueTracker.Bugzilla, 32206, "ContextActions cause memory leak: Page is never destroyed", PlatformAffected.iOS)] + public class Bugzilla32206 : TestNavigationPage + { + protected override void Init() + { + PushAsync(new LandingPage32206()); + } + +#if UITEST && __IOS__ + [Test] + public void Bugzilla32206Test() + { + for (var n = 0; n < 10; n++) + { + RunningApp.WaitForElement(q => q.Marked("Push")); + RunningApp.Tap(q => q.Marked("Push")); + + RunningApp.WaitForElement(q => q.Marked("ListView")); + RunningApp.Back(); + } + + // At this point, the counter can be any value, but it's most likely not zero. + // Invoking GC once is enough to clean up all garbage data and set counter to zero + RunningApp.WaitForElement(q => q.Marked("GC")); + RunningApp.Tap(q => q.Marked("GC")); + + RunningApp.WaitForElement(q => q.Marked("Counter: 0")); + } +#endif + } + + [Preserve(AllMembers = true)] + public class LandingPage32206 : ContentPage + { + public static int Counter; + public Label Label; + + public LandingPage32206() + { + Label = new Label + { + Text = "Counter: " + Counter, + HorizontalTextAlignment = TextAlignment.Center, + VerticalTextAlignment = TextAlignment.Center + }; + + Content = new StackLayout + { + Orientation = StackOrientation.Vertical, + HorizontalOptions = LayoutOptions.Center, + VerticalOptions = LayoutOptions.Center, + Spacing = 15, + Children = + { + new Label + { + Text = "Click Push to show a ListView. When you hit the Back button, Counter will show the number of pages that have not been finalized yet." + + " If you click GC, the counter should be 0." + }, + Label, + new Button + { + Text = "GC", + AutomationId = "GC", + Command = new Command(o => + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + Label.Text = "Counter: " + Counter; + }) + }, + new Button + { + Text = "Push", + AutomationId = "Push", + Command = new Command(async o => + { + await Navigation.PushAsync(new ContentPage32206()); + }) + } + } + }; + } + + protected override void OnAppearing() + { + base.OnAppearing(); + + if (Label != null) + Label.Text = "Counter: " + Counter; + } + } + + [Preserve(AllMembers = true)] + public class ContentPage32206 : ContentPage + { + public ContentPage32206() + { + Interlocked.Increment(ref LandingPage32206.Counter); + System.Diagnostics.Debug.WriteLine("Page: " + LandingPage32206.Counter); + + Content = new ListView + { + ItemsSource = new List<string> { "Apple", "Banana", "Cherry" }, + ItemTemplate = new DataTemplate(typeof(ViewCell32206)), + AutomationId = "ListView" + }; + } + + ~ContentPage32206() + { + Interlocked.Decrement(ref LandingPage32206.Counter); + System.Diagnostics.Debug.WriteLine("Page: " + LandingPage32206.Counter); + } + } + + [Preserve(AllMembers = true)] + public class ViewCell32206 : ViewCell + { + public ViewCell32206() + { + View = new Label(); + View.SetBinding(Label.TextProperty, "."); + ContextActions.Add(new MenuItem { Text = "Delete" }); + } + } +}
\ No newline at end of file diff --git a/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla43941.cs b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla43941.cs new file mode 100644 index 00000000..b69e968a --- /dev/null +++ b/Xamarin.Forms.Controls.Issues/Xamarin.Forms.Controls.Issues.Shared/Bugzilla43941.cs @@ -0,0 +1,136 @@ +using System; +using System.Collections.Generic; +using Xamarin.Forms.CustomAttributes; +using Xamarin.Forms.Internals; +using System.Threading; + +#if UITEST +using Xamarin.UITest; +using NUnit.Framework; +#endif + +namespace Xamarin.Forms.Controls.Issues +{ + [Preserve(AllMembers = true)] + [Issue(IssueTracker.Bugzilla, 43941, "Memory leak with ListView's RecycleElement on iOS", PlatformAffected.iOS)] + public class Bugzilla43941 : TestNavigationPage + { + protected override void Init() + { + PushAsync(new LandingPage43941()); + } + +#if UITEST && __IOS__ + [Test] + public void Bugzilla43941Test() + { + for (var n = 0; n < 10; n++) + { + RunningApp.WaitForElement(q => q.Marked("Push")); + RunningApp.Tap(q => q.Marked("Push")); + + RunningApp.WaitForElement(q => q.Marked("ListView")); + RunningApp.Back(); + } + + // At this point, the counter can be any value, but it's most likely not zero. + // Invoking GC once is enough to clean up all garbage data and set counter to zero + RunningApp.WaitForElement(q => q.Marked("GC")); + RunningApp.Tap(q => q.Marked("GC")); + + RunningApp.WaitForElement(q => q.Marked("Counter: 0")); + } +#endif + } + + [Preserve(AllMembers = true)] + public class ContentPage43941 : ContentPage + { + public ContentPage43941() + { + Interlocked.Increment(ref LandingPage43941.Counter); + System.Diagnostics.Debug.WriteLine("Page: " + LandingPage43941.Counter); + + var list = new List<int>(); + for (var i = 0; i < 30; i++) + list.Add(i); + + Title = "ContentPage43941"; + Content = new ListView + { + HasUnevenRows = true, + ItemsSource = list, + AutomationId = "ListView" + }; + } + + ~ContentPage43941() + { + Interlocked.Decrement(ref LandingPage43941.Counter); + System.Diagnostics.Debug.WriteLine("Page: " + LandingPage43941.Counter); + } + } + + [Preserve(AllMembers = true)] + public class LandingPage43941 : ContentPage + { + public static int Counter; + public Label Label; + + public LandingPage43941() + { + Label = new Label + { + Text = "Counter: " + Counter, + HorizontalTextAlignment = TextAlignment.Center, + VerticalTextAlignment = TextAlignment.Center + }; + + Content = new StackLayout + { + Orientation = StackOrientation.Vertical, + HorizontalOptions = LayoutOptions.Center, + VerticalOptions = LayoutOptions.Center, + Spacing = 15, + Children = + { + new Label + { + Text = "Click Push to show a ListView. When you hit the Back button, Counter will show the number of pages that have not been finalized yet." + + " If you click GC, the counter should be 0." + }, + Label, + new Button + { + Text = "GC", + AutomationId = "GC", + Command = new Command(o => + { + GC.Collect(); + GC.WaitForPendingFinalizers(); + GC.Collect(); + Label.Text = "Counter: " + Counter; + }) + }, + new Button + { + Text = "Push", + AutomationId = "Push", + Command = new Command(async o => + { + await Navigation.PushAsync(new ContentPage43941()); + }) + } + } + }; + } + + protected override void OnAppearing() + { + base.OnAppearing(); + + if (Label != null) + Label.Text = "Counter: " + Counter; + } + } +}
\ 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 fab19248..2d6ddb58 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 @@ -51,6 +51,7 @@ <Compile Include="$(MSBuildThisFileDirectory)Bugzilla31964.cs" /> <Compile Include="$(MSBuildThisFileDirectory)Bugzilla32033.cs" /> <Compile Include="$(MSBuildThisFileDirectory)Bugzilla32034.cs" /> + <Compile Include="$(MSBuildThisFileDirectory)Bugzilla32206.cs" /> <Compile Include="$(MSBuildThisFileDirectory)Bugzilla32776.cs" /> <Compile Include="$(MSBuildThisFileDirectory)Bugzilla32842.xaml.cs"> <DependentUpon>Bugzilla32842.xaml</DependentUpon> @@ -139,6 +140,7 @@ <Compile Include="$(MSBuildThisFileDirectory)Bugzilla43313.cs" /> <Compile Include="$(MSBuildThisFileDirectory)Bugzilla43469.cs" /> <Compile Include="$(MSBuildThisFileDirectory)Bugzilla43516.cs" /> + <Compile Include="$(MSBuildThisFileDirectory)Bugzilla43941.cs" /> <Compile Include="$(MSBuildThisFileDirectory)Bugzilla43663.cs" /> <Compile Include="$(MSBuildThisFileDirectory)Bugzilla43735.cs" /> <Compile Include="$(MSBuildThisFileDirectory)Bugzilla44453.cs" /> diff --git a/Xamarin.Forms.Platform.iOS/Cells/CellTableViewCell.cs b/Xamarin.Forms.Platform.iOS/Cells/CellTableViewCell.cs index e32e86cd..b42f69f4 100644 --- a/Xamarin.Forms.Platform.iOS/Cells/CellTableViewCell.cs +++ b/Xamarin.Forms.Platform.iOS/Cells/CellTableViewCell.cs @@ -7,8 +7,8 @@ namespace Xamarin.Forms.Platform.iOS public class CellTableViewCell : UITableViewCell, INativeElementView { Cell _cell; - public Action<object, PropertyChangedEventArgs> PropertyChanged; + bool _disposed; public CellTableViewCell(UITableViewCellStyle style, string key) : base(style, key) { @@ -95,5 +95,21 @@ namespace Xamarin.Forms.Platform.iOS return nativeCell; } + + protected override void Dispose(bool disposing) + { + if (_disposed) + return; + + if (disposing) + { + PropertyChanged = null; + _cell = null; + } + + _disposed = true; + + base.Dispose(disposing); + } } }
\ No newline at end of file diff --git a/Xamarin.Forms.Platform.iOS/Cells/ViewCellRenderer.cs b/Xamarin.Forms.Platform.iOS/Cells/ViewCellRenderer.cs index 8f8a92ce..30c17234 100644 --- a/Xamarin.Forms.Platform.iOS/Cells/ViewCellRenderer.cs +++ b/Xamarin.Forms.Platform.iOS/Cells/ViewCellRenderer.cs @@ -48,9 +48,12 @@ namespace Xamarin.Forms.Platform.iOS internal class ViewTableCell : UITableViewCell, INativeElementView { WeakReference<IVisualElementRenderer> _rendererRef; - ViewCell _viewCell; + Element INativeElementView.Element => ViewCell; + internal bool SupressSeparator { get; set; } + bool _disposed; + public ViewTableCell(string key) : base(UITableViewCellStyle.Default, key) { } @@ -66,10 +69,6 @@ namespace Xamarin.Forms.Platform.iOS } } - Element INativeElementView.Element => ViewCell; - - internal bool SupressSeparator { get; set; } - public override void LayoutSubviews() { //This sets the content views frame. @@ -115,6 +114,9 @@ namespace Xamarin.Forms.Platform.iOS protected override void Dispose(bool disposing) { + if (_disposed) + return; + if (disposing) { IVisualElementRenderer renderer; @@ -126,8 +128,12 @@ namespace Xamarin.Forms.Platform.iOS _rendererRef = null; } + + _viewCell = null; } + _disposed = true; + base.Dispose(disposing); } diff --git a/Xamarin.Forms.Platform.iOS/Renderers/ListViewRenderer.cs b/Xamarin.Forms.Platform.iOS/Renderers/ListViewRenderer.cs index 3c235806..fd0f15f6 100644 --- a/Xamarin.Forms.Platform.iOS/Renderers/ListViewRenderer.cs +++ b/Xamarin.Forms.Platform.iOS/Renderers/ListViewRenderer.cs @@ -29,6 +29,7 @@ namespace Xamarin.Forms.Platform.iOS IListViewController Controller => Element; ITemplatedItemsView<Cell> TemplatedItemsView => Element; public override UIViewController ViewController => _tableViewController; + bool _disposed; public override SizeRequest GetDesiredSize(double widthConstraint, double heightConstraint) { @@ -90,28 +91,31 @@ namespace Xamarin.Forms.Platform.iOS } } + void DisposeSubviews(UIView view) + { + foreach (UIView subView in view.Subviews) + DisposeSubviews(subView); + + view.RemoveFromSuperview(); + view.Dispose(); + } + protected override void Dispose(bool disposing) { - // check inset tracker for null to - if (disposing && _insetTracker != null) - { - _insetTracker.Dispose(); - _insetTracker = null; + if (_disposed) + return; - var viewsToLookAt = new Stack<UIView>(Subviews); - while (viewsToLookAt.Count > 0) + if (disposing) + { + if (_insetTracker != null) { - var view = viewsToLookAt.Pop(); - var viewCellRenderer = view as ViewCellRenderer.ViewTableCell; - if (viewCellRenderer != null) - viewCellRenderer.Dispose(); - else - { - foreach (var child in view.Subviews) - viewsToLookAt.Push(child); - } + _insetTracker.Dispose(); + _insetTracker = null; } + foreach (UIView subview in Subviews) + DisposeSubviews(subview); + if (Element != null) { var templatedItems = TemplatedItemsView.TemplatedItems; @@ -119,27 +123,28 @@ namespace Xamarin.Forms.Platform.iOS templatedItems.GroupedCollectionChanged -= OnGroupedCollectionChanged; } + if (_dataSource != null) + { + _dataSource.Dispose(); + _dataSource = null; + } + if (_tableViewController != null) { _tableViewController.Dispose(); _tableViewController = null; } - } - if (disposing) - { if (_headerRenderer != null) { - var platform = _headerRenderer.Element.Platform as Platform; - if (platform != null) - platform.DisposeModelAndChildrenRenderers(_headerRenderer.Element); + var platform = _headerRenderer.Element?.Platform as Platform; + platform?.DisposeModelAndChildrenRenderers(_headerRenderer.Element); _headerRenderer = null; } if (_footerRenderer != null) { - var platform = _footerRenderer.Element.Platform as Platform; - if (platform != null) - platform.DisposeModelAndChildrenRenderers(_footerRenderer.Element); + var platform = _footerRenderer.Element?.Platform as Platform; + platform?.DisposeModelAndChildrenRenderers(_footerRenderer.Element); _footerRenderer = null; } @@ -154,6 +159,8 @@ namespace Xamarin.Forms.Platform.iOS Control?.TableFooterView?.Dispose(); } + _disposed = true; + base.Dispose(disposing); } @@ -602,6 +609,7 @@ namespace Xamarin.Forms.Platform.iOS internal class UnevenListViewDataSource : ListViewDataSource { IVisualElementRenderer _prototype; + bool _disposed; public UnevenListViewDataSource(ListView list, FormsUITableViewController uiTableViewController) : base(list, uiTableViewController) { @@ -661,27 +669,47 @@ namespace Xamarin.Forms.Platform.iOS { var target = viewCell.View; if (_prototype == null) - { _prototype = Platform.CreateRenderer(target); - Platform.SetRenderer(target, _prototype); - } else - { _prototype.SetElement(target); - Platform.SetRenderer(target, _prototype); - } + + Platform.SetRenderer(target, _prototype); var req = target.Measure(tableView.Frame.Width, double.PositiveInfinity, MeasureFlags.IncludeMargins); target.ClearValue(Platform.RendererProperty); - foreach (var descendant in target.Descendants()) + foreach (Element descendant in target.Descendants()) + { + IVisualElementRenderer renderer = Platform.GetRenderer(descendant as VisualElement); descendant.ClearValue(Platform.RendererProperty); + renderer?.Dispose(); + } return (nfloat)req.Request.Height; } + var renderHeight = cell.RenderHeight; return renderHeight > 0 ? (nfloat)renderHeight : DefaultRowHeight; } + + protected override void Dispose(bool disposing) + { + if (_disposed) + return; + + _disposed = true; + + if (disposing) + { + if (_prototype != null) + { + _prototype.Dispose(); + _prototype = null; + } + } + + base.Dispose(disposing); + } } internal class ListViewDataSource : UITableViewSource @@ -689,14 +717,15 @@ namespace Xamarin.Forms.Platform.iOS const int DefaultItemTemplateId = 1; static int s_dataTemplateIncrementer = 2; // lets start at not 0 because readonly nfloat _defaultSectionHeight; - readonly Dictionary<DataTemplate, int> _templateToId = new Dictionary<DataTemplate, int>(); - readonly UITableView _uiTableView; - readonly FormsUITableViewController _uiTableViewController; - protected readonly ListView List; + Dictionary<DataTemplate, int> _templateToId = new Dictionary<DataTemplate, int>(); + UITableView _uiTableView; + FormsUITableViewController _uiTableViewController; + protected ListView List; IListViewController Controller => List; protected ITemplatedItemsView<Cell> TemplatedItemsView => List; bool _isDragging; bool _selectionFromNative; + bool _disposed; public ListViewDataSource(ListViewDataSource source) { @@ -1016,6 +1045,29 @@ namespace Xamarin.Forms.Platform.iOS ((INotifyCollectionChanged)templatedList.ShortNames).CollectionChanged -= OnShortNamesCollectionChanged; } } + + protected override void Dispose(bool disposing) + { + if (_disposed) + return; + + if (disposing) + { + if (List != null) + { + List.ItemSelected -= OnItemSelected; + List = null; + } + + _templateToId = null; + _uiTableView = null; + _uiTableViewController = null; + } + + _disposed = true; + + base.Dispose(disposing); + } } } @@ -1031,11 +1083,12 @@ namespace Xamarin.Forms.Platform.iOS internal class FormsUITableViewController : UITableViewController { - readonly ListView _list; + ListView _list; IListViewController Controller => _list; UIRefreshControl _refresh; bool _refreshAdded; + bool _disposed; public FormsUITableViewController(ListView element) { @@ -1117,15 +1170,25 @@ namespace Xamarin.Forms.Platform.iOS protected override void Dispose(bool disposing) { - base.Dispose(disposing); + if (_disposed) + return; - if (disposing && _refresh != null) + if (disposing) { - _refresh.ValueChanged -= OnRefreshingChanged; - _refresh.EndRefreshing(); - _refresh.Dispose(); - _refresh = null; + if (_refresh != null) + { + _refresh.ValueChanged -= OnRefreshingChanged; + _refresh.EndRefreshing(); + _refresh.Dispose(); + _refresh = null; + } + + _list = null; } + + _disposed = true; + + base.Dispose(disposing); } void CheckContentSize() |