[GTK] Fix several memory leaks in the GTK backend (#4112)
authorAndoni Morales Alastruey <ylatuya@gmail.com>
Tue, 30 Oct 2018 17:06:47 +0000 (18:06 +0100)
committerShane Neuville <shane94@hotmail.com>
Tue, 30 Oct 2018 17:06:47 +0000 (11:06 -0600)
* [GTK] Don't recreate master or detail if it didn't change

* [GTK] Fix several memory leaks using the Destroy pattern

Gtk objects must be disposed using the Destroy function that will
automatically iterate over the children and destroy them too:
https://developer.gnome.org/gtk2/stable/GtkWidget.html#gtk-widget-destroy

The gtk-sharp bindings discourage the use of Dispose and don't even call
the base class, leaving it without effect:
https://github.com/mono/gtk-sharp/blob/gtk-sharp-2-12-branch/gtk/Object.custom#L98

In the Controls, that inherits directly from native Gtk objects, the
overrides of the Dispose function are changed to override the Destroy
function.  In the Renderers, that inherit from VisualElementRenderer,
the Destroy funtion is called in the dispose implementation so
subclasses only have to override Dispose (bool disposing) as they do
now.

* [GTK] Don't recreate the toolbar on each change

* [GTK] Fix leak connecting to the PropertyChanged event twice

19 files changed:
Xamarin.Forms.Platform.GTK/Cells/ViewCell.cs
Xamarin.Forms.Platform.GTK/Controls/DatePicker.cs
Xamarin.Forms.Platform.GTK/Controls/ImageButton.cs
Xamarin.Forms.Platform.GTK/Controls/ListView.cs
Xamarin.Forms.Platform.GTK/Controls/NavigationChildPage.cs
Xamarin.Forms.Platform.GTK/Controls/OpenGLView.cs
Xamarin.Forms.Platform.GTK/Controls/Page.cs
Xamarin.Forms.Platform.GTK/Controls/SearchEntry.cs
Xamarin.Forms.Platform.GTK/Extensions/VisualElementExtensions.cs
Xamarin.Forms.Platform.GTK/GtkToolbarTracker.cs
Xamarin.Forms.Platform.GTK/Platform.cs
Xamarin.Forms.Platform.GTK/Renderers/AbstractPageRenderer.cs
Xamarin.Forms.Platform.GTK/Renderers/ListViewRenderer.cs
Xamarin.Forms.Platform.GTK/Renderers/MasterDetailPageRenderer.cs
Xamarin.Forms.Platform.GTK/Renderers/NavigationPageRenderer.cs
Xamarin.Forms.Platform.GTK/Renderers/ScrollViewRenderer.cs
Xamarin.Forms.Platform.GTK/ViewRenderer.cs
Xamarin.Forms.Platform.GTK/VisualElementRenderer.cs
Xamarin.Forms.Platform.GTK/VisualElementTracker.cs

index 0c63fe8..34ae34b 100644 (file)
@@ -51,7 +51,7 @@ namespace Xamarin.Forms.Platform.GTK.Cells
                        return request.Request.Height;
                }
 
-               public override void Dispose()
+               public override void Destroy()
                {
                        IVisualElementRenderer renderer;
                        if (_rendererRef != null && _rendererRef.TryGetTarget(out renderer) && renderer.Element != null)
@@ -59,7 +59,7 @@ namespace Xamarin.Forms.Platform.GTK.Cells
                                _rendererRef = null;
                        }
 
-                       base.Dispose();
+                       base.Destroy();
                }
 
                protected override void UpdateCell()
index eacfabe..86699b3 100644 (file)
@@ -153,8 +153,7 @@ namespace Xamarin.Forms.Platform.GTK.Controls
                private void Close()
                {
                        Helpers.GrabHelper.RemoveGrab(this);
-                       this.Dispose();
-                       this.Destroy();
+                       Destroy();
                }
 
                private void NotifyDateChanged()
index 37cb18e..e45f400 100644 (file)
@@ -117,10 +117,9 @@ namespace Xamarin.Forms.Platform.GTK.Controls
                        }
                }
 
-               public override void Dispose()
+               public override void Destroy()
                {
-                       base.Dispose();
-
+                       base.Destroy();
                        _label = null;
                        _image = null;
                        _imageAndLabelContainer = null;
index 76c34c3..01c0352 100644 (file)
@@ -115,6 +115,23 @@ namespace Xamarin.Forms.Platform.GTK.Controls
                        _selectionColor = DefaultSelectionColor;
                }
 
+               public override void Destroy()
+               {
+                       _store?.Dispose();
+                       _store = null;
+                       _root = null;
+                       _refreshButton = null;
+                       _refreshLabel = null;
+                       _headerContainer = null;
+                       _header = null;
+                       _list = null;
+                       _footerContainer = null;
+                       _footer = null;
+                       _viewPort = null;
+                       _refreshHeader = null;
+                       base.Destroy();
+               }
+
                public static Gdk.Color DefaultSelectionColor = Color.FromHex("#3498DB").ToGtkColor();
 
                public Widget Header
index 2d86121..952f8c9 100644 (file)
@@ -2,7 +2,7 @@
 
 namespace Xamarin.Forms.Platform.GTK.Controls
 {
-       public class NavigationChildPage : Gtk.Object
+       public class NavigationChildPage : IDisposable
        {
                bool _disposed;
 
@@ -12,15 +12,13 @@ namespace Xamarin.Forms.Platform.GTK.Controls
                        Identifier = Guid.NewGuid().ToString();
                }
 
-               public override void Dispose()
+               public void Dispose()
                {
                        if (!_disposed)
                        {
                                _disposed = true;
                                Page = null;
                        }
-
-                       base.Dispose();
                }
 
                public string Identifier { get; set; }
index d5c6dac..0fe836f 100644 (file)
@@ -72,13 +72,13 @@ namespace Xamarin.Forms.Platform.GTK.Controls
                        GLib.Source.Remove(_timerId);
                }
 
-               public override void Dispose()
+               public override void Destroy()
                {
-                       base.Dispose();
+                       base.Destroy();
 
                        if (_glWidget != null)
                        {
-                               _glWidget.Dispose();
+                               _glWidget.Destroy();
                        }
                }
 
index e2ca69b..0d7a651 100644 (file)
@@ -95,14 +95,19 @@ namespace Xamarin.Forms.Platform.GTK.Controls
                        Children.Last().Show();
                }
 
-               public override void Dispose()
+               public override void Destroy()
                {
-                       base.Dispose();
-
+                       base.Destroy();
                        if (_contentContainerWrapper != null)
                        {
                                _contentContainerWrapper.SizeAllocated -= OnContentContainerWrapperSizeAllocated;
+                               _contentContainerWrapper = null;
                        }
+                       _contentContainer = null;
+                       _image = null;
+                       _toolbar = null;
+                       _content = null;
+                       _headerContainer = null;
                }
 
                private void BuildPage()
@@ -135,7 +140,7 @@ namespace Xamarin.Forms.Platform.GTK.Controls
 
                private void RefreshToolbar(HBox newToolbar)
                {
-                       _headerContainer.RemoveFromContainer(_toolbar);
+                       _toolbar.Destroy();
                        _toolbar = newToolbar;
                        _headerContainer.Add(_toolbar);
                        _toolbar.ShowAll();
@@ -143,7 +148,7 @@ namespace Xamarin.Forms.Platform.GTK.Controls
 
                private void RefreshContent(GtkFormsContainer newContent)
                {
-                       _contentContainer.RemoveFromContainer(_content);
+                       _content.Destroy();
                        _content = newContent;
                        _contentContainer.Add(_content);
                        _content.ShowAll();
index 6c5f721..5c3cf0c 100644 (file)
@@ -109,9 +109,9 @@ namespace Xamarin.Forms.Platform.GTK.Controls
                        }
                }
 
-               public override void Dispose()
+               public override void Destroy()
                {
-                       base.Dispose();
+                       base.Destroy();
 
                        if (_entryWrapper?.Entry != null)
                        {
index 9b74d1d..8a5638c 100644 (file)
@@ -48,14 +48,14 @@ namespace Xamarin.Forms.Platform.GTK.Extensions
                                IVisualElementRenderer childRenderer = Platform.GetRenderer(visual);
                                if (childRenderer != null)
                                {
-                                       childRenderer.Dispose();
+                                       ((Gtk.Widget)childRenderer).Destroy();
                                        Platform.SetRenderer(visual, null);
                                }
                        }
 
                        if (renderer != null)
                        {
-                               renderer.Dispose();
+                               ((Gtk.Widget)renderer).Destroy();
                                Platform.SetRenderer(self, null);
                        }
                }
index 31698e6..96f434c 100644 (file)
@@ -209,6 +209,7 @@ namespace Xamarin.Forms.Platform.GTK
                {
                        foreach (var child in _toolbarSection.Children)
                        {
+                               child.Destroy();
                                _toolbarSection.Remove(child);
                        }
 
@@ -370,17 +371,20 @@ namespace Xamarin.Forms.Platform.GTK
 
                        if (NavigationPage.GetHasNavigationBar(currentPage))
                        {
-                               _toolbar = ConfigureToolbar();
-
-                               _toolbarIcon = new ImageControl
+                               if (_toolbar == null)
                                {
-                                       WidthRequest = 1,
-                                       Aspect = ImageAspect.AspectFit
-                               };
-                               _toolbarTitleSection.PackStart(_toolbarIcon, false, false, 8);
+                                       _toolbar = ConfigureToolbar();
+
+                                       _toolbarIcon = new ImageControl
+                                       {
+                                               WidthRequest = 1,
+                                               Aspect = ImageAspect.AspectFit
+                                       };
+                                       _toolbarTitleSection.PackStart(_toolbarIcon, false, false, 8);
 
-                               _toolbarTitle = new Gtk.Label();
-                               _toolbarTitleSection.PackEnd(_toolbarTitle, true, true, 0);
+                                       _toolbarTitle = new Gtk.Label();
+                                       _toolbarTitleSection.PackEnd(_toolbarTitle, true, true, 0);
+                               }
 
                                FindParentMasterDetail();
 
index 32b2fb3..ee85397 100644 (file)
@@ -58,8 +58,7 @@ namespace Xamarin.Forms.Platform.GTK
 
                        renderer = GetRenderer((VisualElement)view);
 
-                       renderer?.Dispose();
-
+                       (renderer as Widget)?.Destroy();
                        view.ClearValue(RendererProperty);
                }
 
@@ -106,11 +105,11 @@ namespace Xamarin.Forms.Platform.GTK
                        MessagingCenter.Unsubscribe<Page, AlertArguments>(this, Page.AlertSignalName);
                        MessagingCenter.Unsubscribe<Page, bool>(this, Page.BusySetSignalName);
 
-                       DisposeModelAndChildrenRenderers(Page);
                        foreach (var modal in _modals)
                                DisposeModelAndChildrenRenderers(modal);
+                       DisposeModelAndChildrenRenderers(Page);
 
-                       PlatformRenderer.Dispose();
+                       PlatformRenderer.Destroy();
                }
 
                internal void SetPage(Page newRoot)
index 72822d2..fdce872 100644 (file)
@@ -54,11 +54,6 @@ namespace Xamarin.Forms.Platform.GTK.Renderers
                        VisualElement oldElement = Element;
                        Element = element;
 
-                       if (element != null)
-                       {
-                               element.PropertyChanged += _propertyChangedHandler;
-                       }
-
                        OnElementChanged(new VisualElementChangedEventArgs(oldElement, element));
 
                        EffectUtilities.RegisterEffectControlProvider(this, oldElement, element);
@@ -84,9 +79,9 @@ namespace Xamarin.Forms.Platform.GTK.Renderers
                        return Container.GetDesiredSize(widthConstraint, heightConstraint);
                }
 
-               public override void Dispose()
+               public override void Destroy()
                {
-                       base.Dispose();
+                       base.Destroy();
 
                        if (!_disposed)
                        {
@@ -125,18 +120,6 @@ namespace Xamarin.Forms.Platform.GTK.Renderers
                        PageController.SendAppearing();
                }
 
-               protected override void OnDestroyed()
-               {
-                       base.OnDestroyed();
-
-                       if (!_appeared || _disposed)
-                               return;
-
-                       _appeared = false;
-
-                       PageController.SendDisappearing();
-               }
-
                protected override void OnSizeAllocated(Gdk.Rectangle allocation)
                {
                        base.OnSizeAllocated(allocation);
index 001ecde..ab70213 100644 (file)
@@ -128,6 +128,8 @@ namespace Xamarin.Forms.Platform.GTK.Renderers
                                {
                                        _listView.OnItemTapped -= OnItemTapped;
                                        _listView.OnRefresh -= OnRefresh;
+                                       _listView.Destroy();
+                                       _listView = null;
                                }
 
                                if (_footerRenderer != null)
index d85e93f..c235889 100644 (file)
@@ -10,6 +10,9 @@ namespace Xamarin.Forms.Platform.GTK.Renderers
 {
        public class MasterDetailPageRenderer : AbstractPageRenderer<Controls.MasterDetailPage, MasterDetailPage>
        {
+               Page _currentMaster;
+               Page _currentDetail;
+
                public MasterDetailPageRenderer()
                {
                        MessagingCenter.Subscribe(this, Forms.BarTextColor, (NavigationPage sender, Color color) =>
@@ -122,22 +125,29 @@ namespace Xamarin.Forms.Platform.GTK.Renderers
                {
                        Gtk.Application.Invoke(async delegate
                        {
-                               Page.Master.PropertyChanged -= HandleMasterPropertyChanged;
                                await UpdateHamburguerIconAsync();
-
-                               if (Platform.GetRenderer(Page.Master) == null)
-                                       Platform.SetRenderer(Page.Master, Platform.CreateRenderer(Page.Master));
-                               if (Platform.GetRenderer(Page.Detail) == null)
-                                       Platform.SetRenderer(Page.Detail, Platform.CreateRenderer(Page.Detail));
-
-                               Widget.Master = Platform.GetRenderer(Page.Master).Container;
-                               Widget.Detail = Platform.GetRenderer(Page.Detail).Container;
-                               Widget.MasterTitle = Page.Master?.Title ?? string.Empty;
-
+                               if (Page.Master != _currentMaster)
+                               {
+                                       if (_currentMaster != null)
+                                       {
+                                               _currentMaster.PropertyChanged -= HandleMasterPropertyChanged;
+                                       }
+                                       if (Platform.GetRenderer(Page.Master) == null)
+                                               Platform.SetRenderer(Page.Master, Platform.CreateRenderer(Page.Master));
+                                       Widget.Master = Platform.GetRenderer(Page.Master).Container;
+                                       Widget.MasterTitle = Page.Master?.Title ?? string.Empty;
+                                       Page.Master.PropertyChanged += HandleMasterPropertyChanged;
+                                       _currentMaster = Page.Master;
+                               }
+                               if (Page.Detail != _currentDetail)
+                               {
+                                       if (Platform.GetRenderer(Page.Detail) == null)
+                                               Platform.SetRenderer(Page.Detail, Platform.CreateRenderer(Page.Detail));
+                                       Widget.Detail = Platform.GetRenderer(Page.Detail).Container;
+                                       _currentDetail = Page.Detail;
+                               }
                                UpdateBarTextColor();
                                UpdateBarBackgroundColor();
-
-                               Page.Master.PropertyChanged += HandleMasterPropertyChanged;
                        });
                }
 
index 380c5c9..2426c90 100644 (file)
@@ -67,6 +67,12 @@ namespace Xamarin.Forms.Platform.GTK.Renderers
                                if (_currentPage != null)
                                {
                                        _currentPage.PropertyChanged -= OnCurrentPagePropertyChanged;
+                                       _currentPage = null;
+                               }
+                               if (_currentStack != null)
+                               {
+                                       _currentStack.ForEach(s => s.Dispose());
+                                       _currentStack = null;
                                }
                        }
 
index 19cf012..fb8bb8f 100644 (file)
@@ -87,6 +87,11 @@ namespace Xamarin.Forms.Platform.GTK.Renderers
                                if (Control.Vadjustment != null)
                                        Control.Vadjustment.ValueChanged -= OnScrollEvent;
                        }
+                       if (_viewPort != null)
+                       {
+                               _viewPort.Destroy();
+                               _viewPort = null;
+                       }
 
                        base.Dispose(disposing);
                }
index febdfd8..727b6ad 100644 (file)
@@ -15,13 +15,12 @@ namespace Xamarin.Forms.Platform.GTK
 
                protected override void Dispose(bool disposing)
                {
-                       base.Dispose(disposing);
-
                        if (Control != null)
                        {
-                               Control.Dispose();
+                               Control.Destroy();
                                Control = null;
                        }
+                       base.Dispose(disposing);
                }
 
                protected override void OnElementChanged(ElementChangedEventArgs<TView> e)
index be39cfb..bb93d02 100644 (file)
@@ -137,10 +137,9 @@ namespace Xamarin.Forms.Platform.GTK
                        UpdateIsVisible();
                }
 
-               public override void Dispose()
+               public override void Destroy()
                {
-                       base.Dispose();
-
+                       base.Destroy();
                        Dispose(true);
                }
 
index 6bd9667..11b238a 100644 (file)
@@ -170,7 +170,7 @@ namespace Xamarin.Forms.Platform.GTK
                                _control.ButtonPressEvent -= OnControlButtonPressEvent;
                        }
 
-                       Container.Dispose();
+                       Container.Destroy();
                        Container = null;
                }