[iOS/Critical] Fix ListView memory leaks #524

Merged
merged 6 commits into from Jan 26, 2017

Conversation

Projects
None yet
8 participants
@adrianknight89
Contributor

adrianknight89 commented Nov 12, 2016

Description of Change

iOS ListView is leaking for various reasons. Although the bug description mentions issues with RecycleElement, this applies to either caching strategy. Since this seems to be a serious issue, can this PR be given priority?

See comments for details.

P.S. It was mentioned that the same issue occurs on Android, but I wasn't able to reproduce it on AppCompat (at least not with the sample code in this PR).

EDIT: Also included the fix for another bug concerning context actions not being properly disposed.

Bugs Fixed

PR Checklist

  • Has tests (if omitted, state reason in description)
  • Rebased on top of master at time of PR
  • Changes adhere to coding standard
  • Consolidate commits as makes sense
+ _disposed = true;
+
+ base.Dispose(disposing);
+ }

This comment has been minimized.

@adrianknight89

adrianknight89 Nov 12, 2016

Contributor

One of the issues seems to be that CellTableViewCell is hanging onto _cell. This should release the reference.

@adrianknight89

adrianknight89 Nov 12, 2016

Contributor

One of the issues seems to be that CellTableViewCell is hanging onto _cell. This should release the reference.

+ foreach (UIView child in view.Subviews)
+ viewsToLookAt.Push(child);
+
+ view.Dispose();

This comment has been minimized.

@adrianknight89

adrianknight89 Nov 12, 2016

Contributor

As this loop executes, eventually we hit CellTableViewCell. Calling Dispose on it should properly release the reference to its _cell.

The original code was calling Dispose only on ViewCellRenderer.ViewTableCell instances. At the moment, I'm not sure if disposing every subview is the correct approach, so this is up for discussion. Please note that TableViewRenderer is doing the same thing, so it might need an update as well.

@adrianknight89

adrianknight89 Nov 12, 2016

Contributor

As this loop executes, eventually we hit CellTableViewCell. Calling Dispose on it should properly release the reference to its _cell.

The original code was calling Dispose only on ViewCellRenderer.ViewTableCell instances. At the moment, I'm not sure if disposing every subview is the correct approach, so this is up for discussion. Please note that TableViewRenderer is doing the same thing, so it might need an update as well.

This comment has been minimized.

@adrianknight89

adrianknight89 Nov 13, 2016

Contributor

EDIT: I made changes so that subviews are visited recursively, removed from their superviews, and disposed before their parents/superviews can be disposed.

@adrianknight89

adrianknight89 Nov 13, 2016

Contributor

EDIT: I made changes so that subviews are visited recursively, removed from their superviews, and disposed before their parents/superviews can be disposed.

+ {
+ _dataSource.Dispose();
+ _dataSource = null;
+ }

This comment has been minimized.

@adrianknight89

adrianknight89 Nov 12, 2016

Contributor

The data source needs to be disposed as well. See what it's doing below.

@adrianknight89

adrianknight89 Nov 12, 2016

Contributor

The data source needs to be disposed as well. See what it's doing below.

+ _disposed = true;
+
+ base.Dispose(disposing);
+ }

This comment has been minimized.

@adrianknight89

adrianknight89 Nov 12, 2016

Contributor

Release the reference to _prototype but do not dispose it.

@adrianknight89

adrianknight89 Nov 12, 2016

Contributor

Release the reference to _prototype but do not dispose it.

+ _disposed = true;
+
+ base.Dispose(disposing);
+ }

This comment has been minimized.

@adrianknight89

adrianknight89 Nov 12, 2016

Contributor

I did not see OnItemSelected being unregistered anywhere. References to ListView and FormsUITableViewController must be nullified. I think what is happening is when the constructor is called, we are passing the references by value which creates a copy for each. You could test this particular change by having a list view with no items. If these references are not nullified, you end up with a leak still.

I'm not sure if setting _templateToId to null was required since it's a Dictionary, but the keys are data templates. It seemed safer to null it out as well.

@adrianknight89

adrianknight89 Nov 12, 2016

Contributor

I did not see OnItemSelected being unregistered anywhere. References to ListView and FormsUITableViewController must be nullified. I think what is happening is when the constructor is called, we are passing the references by value which creates a copy for each. You could test this particular change by having a list view with no items. If these references are not nullified, you end up with a leak still.

I'm not sure if setting _templateToId to null was required since it's a Dictionary, but the keys are data templates. It seemed safer to null it out as well.

+
+ _disposed = true;
+
+ base.Dispose(disposing);

This comment has been minimized.

@adrianknight89

adrianknight89 Nov 12, 2016

Contributor

Same thing with _list

@adrianknight89

adrianknight89 Nov 12, 2016

Contributor

Same thing with _list

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez Nov 12, 2016

Member

At first glance this looks pretty good; I'll have to spend some time on Monday looking more closely at the subview disposal stuff you mentioned.

Could you add some instructions to the test so when the case is being manually verified the tester knows what to look for?

Member

hartez commented Nov 12, 2016

At first glance this looks pretty good; I'll have to spend some time on Monday looking more closely at the subview disposal stuff you mentioned.

Could you add some instructions to the test so when the case is being manually verified the tester knows what to look for?

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Nov 12, 2016

Contributor

@hartez Done.

Contributor

adrianknight89 commented Nov 12, 2016

@hartez Done.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Nov 13, 2016

Contributor

@hartez I included the fix for another bug as well. Also added a disposed check to ViewTableCell since it was being multi-disposed.

Contributor

adrianknight89 commented Nov 13, 2016

@hartez I included the fix for another bug as well. Also added a disposed check to ViewTableCell since it was being multi-disposed.

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez Nov 15, 2016

Member

@adrianknight89 Current version is failing to build; looks like there's a conflict between your CustomPage class and another UI test.

Member

hartez commented Nov 15, 2016

@adrianknight89 Current version is failing to build; looks like there's a conflict between your CustomPage class and another UI test.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Nov 15, 2016

Contributor

@hartez Can you try now?

Contributor

adrianknight89 commented Nov 15, 2016

@hartez Can you try now?

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Nov 17, 2016

Contributor

I think some of these references can be converted to WeakReference.

Contributor

adrianknight89 commented Nov 17, 2016

I think some of these references can be converted to WeakReference.

@EmmanVazz

This comment has been minimized.

Show comment
Hide comment
@EmmanVazz

EmmanVazz Nov 25, 2016

I have also been able to reproduce these leaks and this seem like a good fix. I think these leaks also affect CarouselView as I found a similar leak while using it. I would think it requires a similar fix to this.

I have also been able to reproduce these leaks and this seem like a good fix. I think these leaks also affect CarouselView as I found a similar leak while using it. I would think it requires a similar fix to this.

@hartez

Code changes look good, just need to make some adjustments to the tests.

+ }
+ }
+
+ internal class CustomPageY : ContentPage

This comment has been minimized.

@hartez

hartez Nov 30, 2016

Member

Instead of CustomPageY and CustomPageX, could you give these names which are less likely to accidentally conflict with other test cases in the future? Using the issue number in the name should be sufficient; something like _43941CustomPage or CustomPage43941.

@hartez

hartez Nov 30, 2016

Member

Instead of CustomPageY and CustomPageX, could you give these names which are less likely to accidentally conflict with other test cases in the future? Using the issue number in the name should be sufficient; something like _43941CustomPage or CustomPage43941.

+ });
+ }
+ }
+

This comment has been minimized.

@hartez

hartez Nov 30, 2016

Member

You'll need to add the [Preserve(AllMembers = true)] attribute to these supplemental classes in your test cases; otherwise, the linker will remove them and the automated UI tests will fail.

@hartez

hartez Nov 30, 2016

Member

You'll need to add the [Preserve(AllMembers = true)] attribute to these supplemental classes in your test cases; otherwise, the linker will remove them and the automated UI tests will fail.

This comment has been minimized.

@adrianknight89

adrianknight89 Nov 30, 2016

Contributor

So every class needs to have this attribute? What happens if these classes are nested under a parent class in this case Bugzilla32206? Do they still require the attribute?

@adrianknight89

adrianknight89 Nov 30, 2016

Contributor

So every class needs to have this attribute? What happens if these classes are nested under a parent class in this case Bugzilla32206? Do they still require the attribute?

@@ -0,0 +1,72 @@
+using System;

This comment has been minimized.

@hartez

hartez Nov 30, 2016

Member

Both this and the test case for 32206 could be automated. Take a look at the test for 44166 for an example.

@hartez

hartez Nov 30, 2016

Member

Both this and the test case for 32206 could be automated. Take a look at the test for 44166 for an example.

+using NUnit.Framework;
+#endif
+
+namespace Xamarin.Forms.Controls

This comment has been minimized.

@hartez

hartez Nov 30, 2016

Member

Namespace should be Xamarin.Forms.Controls.Issues (trying to get that to be more consistent).

@hartez

hartez Nov 30, 2016

Member

Namespace should be Xamarin.Forms.Controls.Issues (trying to get that to be more consistent).

+using NUnit.Framework;
+#endif
+
+namespace Xamarin.Forms.Controls

This comment has been minimized.

@hartez

hartez Nov 30, 2016

Member

Namespace should be Xamarin.Forms.Controls.Issues.

@hartez

hartez Nov 30, 2016

Member

Namespace should be Xamarin.Forms.Controls.Issues.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Dec 1, 2016

Contributor

@hartez I addressed the issues except for the UI tests. When I try to compile Xamarin.Forms.ControlGallery.iOS in release mode in XS with the latest alpha of Xamarin, I'm getting a linker issue:

/Users/xyz/Projects/adrianknight89/Xamarin.Forms/Xamarin.Forms.ControlGallery.iOS/Xamarin.Forms.ControlGallery.iOS.csproj (Build) ->
/Library/Frameworks/Mono.framework/External/xbuild/Xamarin/iOS/Xamarin.iOS.Common.targets (_CompileToNative target) ->

	MTOUCH: error MT2001: Could not link assemblies. Reason: Value cannot be null.
Contributor

adrianknight89 commented Dec 1, 2016

@hartez I addressed the issues except for the UI tests. When I try to compile Xamarin.Forms.ControlGallery.iOS in release mode in XS with the latest alpha of Xamarin, I'm getting a linker issue:

/Users/xyz/Projects/adrianknight89/Xamarin.Forms/Xamarin.Forms.ControlGallery.iOS/Xamarin.Forms.ControlGallery.iOS.csproj (Build) ->
/Library/Frameworks/Mono.framework/External/xbuild/Xamarin/iOS/Xamarin.iOS.Common.targets (_CompileToNative target) ->

	MTOUCH: error MT2001: Could not link assemblies. Reason: Value cannot be null.
@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez Dec 1, 2016

Member

@adrianknight89 It's a known issue in the alpha; if you switch to the stable channel you should be able to build.

It looks like they've already got a fix for it, so I'm guessing you'll be able to build with the next alpha update as well.

Member

hartez commented Dec 1, 2016

@adrianknight89 It's a known issue in the alpha; if you switch to the stable channel you should be able to build.

It looks like they've already got a fix for it, so I'm guessing you'll be able to build with the next alpha update as well.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Dec 2, 2016

Contributor

@hartez I have added a UI test for 32206. It navigates back and forth on the stack 10 times and hits GC button to collect and finalize garbage data. The test should be successful if the counter is zero. However, it seems static variables I added for tracking state seem to be null because the parent page no longer exists. If you run the test now, it will crash because _label (tried counter before as well) is null. Would you mind telling me why this is the case? Before I proceed to the other test, I'd like to figure this one out first.

Also came across another bug where, if you push and pop the page rapidly, eventually a page will start leaking. This could be a problem with NavigationPage. It's something to keep in mind for now. (Edit: I think this might be the same as 39908)

P.S. I haven't verified this. 44166 is also using counters. If it has the same issue, then the counters will always be zero.

P.P.S. I think that test also has the same issue. I added a static label to _44166MDP and removed the return statement inside the for loop. The label was null.

Contributor

adrianknight89 commented Dec 2, 2016

@hartez I have added a UI test for 32206. It navigates back and forth on the stack 10 times and hits GC button to collect and finalize garbage data. The test should be successful if the counter is zero. However, it seems static variables I added for tracking state seem to be null because the parent page no longer exists. If you run the test now, it will crash because _label (tried counter before as well) is null. Would you mind telling me why this is the case? Before I proceed to the other test, I'd like to figure this one out first.

Also came across another bug where, if you push and pop the page rapidly, eventually a page will start leaking. This could be a problem with NavigationPage. It's something to keep in mind for now. (Edit: I think this might be the same as 39908)

P.S. I haven't verified this. 44166 is also using counters. If it has the same issue, then the counters will always be zero.

P.P.S. I think that test also has the same issue. I added a static label to _44166MDP and removed the return statement inside the for loop. The label was null.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Dec 2, 2016

Member

@adrianknight89 you can also disable the linker if you want to workaround that error for now.

Member

rmarinho commented Dec 2, 2016

@adrianknight89 you can also disable the linker if you want to workaround that error for now.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Dec 2, 2016

Contributor

@rmarinho I worked around the first issue by going back to the stable channel. You mean the static variable issue is also a linker issue?

Contributor

adrianknight89 commented Dec 2, 2016

@rmarinho I worked around the first issue by going back to the stable channel. You mean the static variable issue is also a linker issue?

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Dec 2, 2016

Member

Nop i was talking if you want to build in alpha and go around that linker issue

Member

rmarinho commented Dec 2, 2016

Nop i was talking if you want to build in alpha and go around that linker issue

@hartez

This comment has been minimized.

Show comment
Hide comment
@hartez

hartez Dec 2, 2016

Member

@adrianknight89 I think making the Label static is causing your problems. Change it to

public Label _label;

And instead of accessing it from the landing page like this:

if (LandingPage32206._label.Text != "Counter: 0")
    Assert.Fail("Test page is leaking.");

Just let the UI test framework look for the correct value:

RunningApp.WaitForElement(q => q.Marked("Counter: 0"));
Member

hartez commented Dec 2, 2016

@adrianknight89 I think making the Label static is causing your problems. Change it to

public Label _label;

And instead of accessing it from the landing page like this:

if (LandingPage32206._label.Text != "Counter: 0")
    Assert.Fail("Test page is leaking.");

Just let the UI test framework look for the correct value:

RunningApp.WaitForElement(q => q.Marked("Counter: 0"));
@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Dec 3, 2016

Contributor

@hartez Your suggestion seems to work; however, I still believe 44166 is broken because it's querying static variables.

I added the other UI test as well. Let me know if any other changes are needed.

Contributor

adrianknight89 commented Dec 3, 2016

@hartez Your suggestion seems to work; however, I still believe 44166 is broken because it's querying static variables.

I added the other UI test as well. Let me know if any other changes are needed.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Dec 10, 2016

Contributor

Found out what the issue was with regards to the extra renderer being created. It happens when uneven rows are used to calculate an estimated row height. Added code to properly dispose _prototype.

Contributor

adrianknight89 commented Dec 10, 2016

Found out what the issue was with regards to the extra renderer being created. It happens when uneven rows are used to calculate an estimated row height. Added code to properly dispose _prototype.

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Dec 10, 2016

Contributor

@hartez There seems to be an issue with the way _prototype is disposed when its element is a layout. Disposing the renderer is not enough to dispose child renderers.

HasUnevenRows=true

public CustomViewCell : ViewCell
{
    public CustomViewCell()
    {
         //View = new DatePicker();
         View = new StackLayout
         {
                 Children = {new DatePicker()}
         }
    }
}

CalculateHeightForCell is clearing renderers out of elements like so:

target.ClearValue(Platform.RendererProperty);
foreach (var descendant in target.Descendants())
	descendant.ClearValue(Platform.RendererProperty);

This doesn't seem to dispose renderers. So, at the moment, ListView is leaking when uneven rows are used and cell view is a layout.

Contributor

adrianknight89 commented Dec 10, 2016

@hartez There seems to be an issue with the way _prototype is disposed when its element is a layout. Disposing the renderer is not enough to dispose child renderers.

HasUnevenRows=true

public CustomViewCell : ViewCell
{
    public CustomViewCell()
    {
         //View = new DatePicker();
         View = new StackLayout
         {
                 Children = {new DatePicker()}
         }
    }
}

CalculateHeightForCell is clearing renderers out of elements like so:

target.ClearValue(Platform.RendererProperty);
foreach (var descendant in target.Descendants())
	descendant.ClearValue(Platform.RendererProperty);

This doesn't seem to dispose renderers. So, at the moment, ListView is leaking when uneven rows are used and cell view is a layout.

@EmmanVazz EmmanVazz referenced this pull request in xamarin/Xamarin.Forms.CarouselView Dec 11, 2016

Closed

Memory Leaks? #16

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Dec 11, 2016

Contributor

Disposing child renderers while clearing RendererProperty in CalculateHeightForCell seems to fix the above issue.

Contributor

adrianknight89 commented Dec 11, 2016

Disposing child renderers while clearing RendererProperty in CalculateHeightForCell seems to fix the above issue.

adrianknight89 added some commits Nov 12, 2016

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 7aaab26.

changed class names

changes

UI test for 32206

update ui test

fix whitespace

UITests done
@jonathanpeppers

This comment has been minimized.

Show comment
Hide comment
@jonathanpeppers

jonathanpeppers Jan 3, 2017

Contributor

@adrianknight89 we are using this fix in our fork of Xamarin.Forms. Thanks for it, you have so many PRs.

There was one change I had to make that was causing NRE's in our app: Hitcents@8f2bed6

Contributor

jonathanpeppers commented Jan 3, 2017

@adrianknight89 we are using this fix in our fork of Xamarin.Forms. Thanks for it, you have so many PRs.

There was one change I had to make that was causing NRE's in our app: Hitcents@8f2bed6

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Jan 3, 2017

Contributor

@jonathanpeppers Thanks for your input. Was the NRE caused by my changes? I'm not sure if we should always expect Element to be non-null by the time we're accessing the platform object.

Contributor

adrianknight89 commented Jan 3, 2017

@jonathanpeppers Thanks for your input. Was the NRE caused by my changes? I'm not sure if we should always expect Element to be non-null by the time we're accessing the platform object.

@jonathanpeppers

This comment has been minimized.

Show comment
Hide comment
@jonathanpeppers

jonathanpeppers Jan 3, 2017

Contributor

So we put your changes on top of our fork by itself. We were specifically trying to fix a memory leak with ListView (which is fixed, thanks).

I guess because we have lots of headers, it could cause the NRE. It is also possible we have a custom renderer as a header.

Contributor

jonathanpeppers commented Jan 3, 2017

So we put your changes on top of our fork by itself. We were specifically trying to fix a memory leak with ListView (which is fixed, thanks).

I guess because we have lots of headers, it could cause the NRE. It is also possible we have a custom renderer as a header.

@rmarinho

This comment has been minimized.

Show comment
Hide comment
@rmarinho

rmarinho Jan 5, 2017

Member

can you add that check @adrianknight89 ?

Member

rmarinho commented Jan 5, 2017

can you add that check @adrianknight89 ?

@adrianknight89

This comment has been minimized.

Show comment
Hide comment
@adrianknight89

adrianknight89 Jan 5, 2017

Contributor

Done.

Contributor

adrianknight89 commented Jan 5, 2017

Done.

@hartez

hartez approved these changes Jan 24, 2017

@rmarinho rmarinho merged commit 6b2a69d into xamarin:master Jan 26, 2017

6 checks passed

Android-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run Android 6.0.1 : Tests passed: 350, i…
Details
OSX-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: OSX Debug : Running
Details
Windows-Debug-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: Windows Debug : Tests passed: 3679, ignored: 10
Details
iOS10-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS10 : Tests passed: 34…
Details
iOS8-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified IOS8 : Tests passed: 346…
Details
iOS9-UITests-C8 Finished TeamCity Build Xamarin.Forms :: Debug :: Cycle 8 :: UI Tests :: OSX Test Cloud Package - Run iOS Unified iOS9 : Tests passed: 348…
Details

@jimmgarrido jimmgarrido referenced this pull request Jun 7, 2017

Merged

Fix possible crash on iOS when using ContextActions #973

4 of 4 tasks complete

@samhouts samhouts added D-15.4 and removed cla-already-signed labels Oct 10, 2017

@samhouts samhouts modified the milestone: 3.1.0 Jun 1, 2018

@samhouts samhouts added this to Done in vNext+1 (master) Jun 26, 2018

@samhouts samhouts removed this from Done in vNext+1 (master) Jun 26, 2018

@samhouts samhouts added this to the 2.3.5 milestone Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment