[GTK] Fix several memory leaks in the GTK backend by ylatuya · Pull Request #4112 · xamarin/Xamarin.Forms

ylatuya · 2018-10-17T09:45:28Z

Description of Change

This serie of patches fixes several memory leaks in the GTK backend.
~~The first one is the MasterDetailPage that is not cleaning up old pages when the Detail or Menu page is changed.~~

Another issue is how disposal of Gtk objects is handled, wich is now incorrectly done using the Dispose function. Gtk objects should 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), so that's why Gtk objects should override the Destroy function. Problably the gtk-sharp bindings should be changed at some point to have a more idiomatic pattern for disposal, but that would break backwards compatibility, so right now the only possible solution is to use Destroy.

I created a demo application with a MaterDetail that calls the GC on each Detail change. As you can see the number of live objects in master keeps growing while in this branch it's mostly steady going up and down.

Before

After

Issues Resolved

API Changes

None

Platforms Affected

Gtk

Behavioral/Visual Changes

None

Before/After Screenshots

Not applicable

Testing Procedure

Create a sample MasterDetail application with 2 pages and some content inside the pages.
Call a full garbage collection after switching pages that also waits for pending finalizers.
Launch the application in the profiler with automatic snapshots on garbage collection.

PR Checklist

Has automated tests
Rebased on top of the target branch at time of PR
Changes adhere to coding standard

rmarinho · 2018-10-17T12:02:32Z

needs rebase ?

ylatuya · 2018-10-17T13:10:01Z

I pushed the wrong branch, everything should be fine now.

ylatuya · 2018-10-17T18:05:13Z

Regarding the MasterDetailPage leak, I am not so sure if the solution is the correct one. Should the MasterDetailPage be responsible of disposing the old Detail or should the application be responsible of doing so? I am think of scenarios in which the application would want to reuse the Page and just switch them using the Detail property.

ylatuya · 2018-10-17T21:27:47Z

I just removed the disposal of the Detail page from the MasterDetailPage since it shouldn't be it's responsibility, but now any page set as Detail in the MasterDetailPage is leaked.

jsuarezruiz

Tested. Checking the list of controls inside the Controls folder, there are several ones (SearchEntry, OpenGLView, etc) where Destroy could be used although we can leave it for another PR.

ylatuya · 2018-10-21T16:23:31Z

I will check that because I thought I fixed all of them. There are two different scenarios, Controls using Gtk elements directly, where you have to use Destroy, and those inheriting from ViewRenderer where the base class handles it and you can have a more idiomatic implementation using Dispose

ylatuya · 2018-10-22T14:59:29Z

@jsuarezruiz Looking that reset of components, SearchEntry is using Destroy with the changes in this PR, GLWidget was already using Destroy, so everything should beok

PureWeen · 2018-10-23T04:20:08Z

 	public class MasterDetailPageRenderer : AbstractPageRenderer<Controls.MasterDetailPage, MasterDetailPage>
 	{
+		Page currentMaster;
+		Page currentDetail;


Prefix these two with _

PureWeen · 2018-10-25T21:21:22Z

@ylatuya can you rebase this to master? There were some errors that got checked into master a while ago and your branch doesn't have the fix so the build is currently failing with

Xamarin.Forms.Controls.Issues\Xamarin.Forms.Controls.Issues.Shared\Issue3979.xaml.cs(32,4): Error CS0103: The name 'TargetSpan' does not exist in the current context
Xamarin.Forms.Controls.Issues\Xamarin.Forms.Controls.Issues.Shared\Issue3979.xaml.cs(32,4): Error CS0103: The name 'TargetSpan' does not exist in the current context
Xamarin.Forms.Controls.Issues\Xamarin.Forms.Controls.Issues.Shared\Issue3979.xaml.cs(32,4): Error CS0103: The name 'TargetSpan' does not exist in the current context
Xamarin.Forms.Controls.Issues\Xamarin.Forms.Controls.Issues.Shared\Issue3979.xaml.cs(32,4): Error CS0103: The name 'TargetSpan' does not exist in the current context
Process 'msbuild.exe' exited with code '1'.

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.

ylatuya · 2018-10-27T08:39:09Z

Rebased and comments processed

PureWeen · 2018-10-30T15:41:28Z

build

ylatuya force-pushed the gtk-mem-leaks branch from 32a269a to 76f56fa Compare October 17, 2018 09:51

rmarinho requested a review from jsuarezruiz October 17, 2018 12:02

ylatuya force-pushed the gtk-mem-leaks branch from 76f56fa to 51bd224 Compare October 17, 2018 13:13

PureWeen assigned hartez and PureWeen Oct 17, 2018

PureWeen requested review from PureWeen and hartez October 17, 2018 13:53

ylatuya changed the title ~~Fix several memory leaks in the GTK backend~~ [GTK] Fix several memory leaks in the GTK backend Oct 17, 2018

samhouts added the p/gtk label Oct 17, 2018

samhouts assigned jsuarezruiz Oct 17, 2018

ylatuya force-pushed the gtk-mem-leaks branch from 51bd224 to 1de20c0 Compare October 17, 2018 21:24

PureWeen unassigned hartez Oct 18, 2018

PureWeen removed the request for review from hartez October 18, 2018 15:04

PureWeen added a/performance t/bug 🐛 labels Oct 18, 2018

jsuarezruiz approved these changes Oct 21, 2018

View reviewed changes

PureWeen suggested changes Oct 23, 2018

View reviewed changes

ylatuya added 4 commits October 27, 2018 10:37

[GTK] Don't recreate master or detail if it didn't change

f1df0d5

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

88dfbc5

[GTK] Fix leak connecting to the PropertyChanged event twice

aa6f175

ylatuya force-pushed the gtk-mem-leaks branch from 1de20c0 to f133a13 Compare October 27, 2018 08:37

PureWeen approved these changes Oct 30, 2018

View reviewed changes

PureWeen merged commit 4919476 into xamarin:master Oct 30, 2018

samhouts added this to the 4.0.0 milestone Dec 4, 2018

samhouts modified the milestones: 4.0.0, 3.5.0 Jan 10, 2019

samhouts added the hacktoberfest 🍻 label Oct 1, 2019

mfkl mentioned this pull request Oct 6, 2019

[Bug] (3.3->3.5 Regression) GTK: when navigating back to previous page, not all contents are rendered #6663

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[GTK] Fix several memory leaks in the GTK backend#4112

[GTK] Fix several memory leaks in the GTK backend#4112
PureWeen merged 4 commits intoxamarin:masterfrom
fluendo:gtk-mem-leaks

ylatuya commented Oct 17, 2018 •

edited

Loading

Uh oh!

rmarinho commented Oct 17, 2018

Uh oh!

ylatuya commented Oct 17, 2018 •

edited

Loading

Uh oh!

ylatuya commented Oct 17, 2018

Uh oh!

ylatuya commented Oct 17, 2018 •

edited

Loading

Uh oh!

jsuarezruiz left a comment

Uh oh!

ylatuya commented Oct 21, 2018 •

edited

Loading

Uh oh!

ylatuya commented Oct 22, 2018

Uh oh!

PureWeen Oct 23, 2018

Uh oh!

ylatuya Oct 27, 2018

Uh oh!

PureWeen commented Oct 25, 2018

Uh oh!

ylatuya commented Oct 27, 2018 •

edited

Loading

Uh oh!

PureWeen commented Oct 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

ylatuya commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Change

Issues Resolved

API Changes

Platforms Affected

Behavioral/Visual Changes

Before/After Screenshots

Testing Procedure

PR Checklist

Uh oh!

rmarinho commented Oct 17, 2018

Uh oh!

ylatuya commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ylatuya commented Oct 17, 2018

Uh oh!

ylatuya commented Oct 17, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jsuarezruiz left a comment

Choose a reason for hiding this comment

Uh oh!

ylatuya commented Oct 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ylatuya commented Oct 22, 2018

Uh oh!

PureWeen Oct 23, 2018

Choose a reason for hiding this comment

Uh oh!

ylatuya Oct 27, 2018

Choose a reason for hiding this comment

Uh oh!

PureWeen commented Oct 25, 2018

Uh oh!

ylatuya commented Oct 27, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PureWeen commented Oct 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ylatuya commented Oct 17, 2018 •

edited

Loading

ylatuya commented Oct 17, 2018 •

edited

Loading

ylatuya commented Oct 17, 2018 •

edited

Loading

ylatuya commented Oct 21, 2018 •

edited

Loading

ylatuya commented Oct 27, 2018 •

edited

Loading