Bug 39884 - Leaks in simple push / pop of UIViewController
Summary: Leaks in simple push / pop of UIViewController
Status: RESOLVED ANSWERED
Alias: None
Product: iOS
Classification: Xamarin
Component: General ()
Version: XI 9.4 (iOS 9.2)
Hardware: PC Mac OS
: Normal normal
Target Milestone: Future Cycle (TBD)
Assignee: Rolf Bjarne Kvinge [MSFT]
URL:
Depends on:
Blocks:
 
Reported: 2016-03-24 11:52 UTC by Alek Slater
Modified: 2016-09-09 09:40 UTC (History)
5 users (show)

Tags:
Is this bug a regression?: ---
Last known good build:


Attachments
Reproducable Test Project (16.58 KB, application/zip)
2016-03-24 11:52 UTC, Alek Slater
Details
csharp allocs (244.65 KB, image/png)
2016-03-30 17:27 UTC, Alek Slater
Details
Comparisons (1.05 MB, application/zip)
2016-03-30 17:29 UTC, Alek Slater
Details
Cleaned up and improved reproducable sample with suggested fix that doesnt work (19.94 KB, application/zip)
2016-04-06 12:54 UTC, Alek Slater
Details
System Info (1.63 KB, text/plain)
2016-05-13 14:31 UTC, Alek Slater
Details
Device Output (26.42 KB, text/plain)
2016-05-13 14:32 UTC, Alek Slater
Details
Updated Solution (1.08 MB, application/zip)
2016-05-13 14:34 UTC, Alek Slater
Details


Notice (2018-05-24): bugzilla.xamarin.com is now in read-only mode.

Please join us on Visual Studio Developer Community and in the Xamarin and Mono organizations on GitHub to continue tracking issues. Bugzilla will remain available for reference in read-only mode. We will continue to work on open Bugzilla bugs, copy them to the new locations as needed for follow-up, and add the new items under Related Links.

Our sincere thanks to everyone who has contributed on this bug tracker over the years. Thanks also for your understanding as we make these adjustments and improvements for the future.


Please create a new report on Developer Community or GitHub with your current version information, steps to reproduce, and relevant error messages or log files if you are hitting an issue that looks similar to this resolved bug and you do not yet see a matching new report.

Related Links:
Status:
RESOLVED ANSWERED

Description Alek Slater 2016-03-24 11:52:01 UTC
Created attachment 15515 [details]
Reproducable Test Project

Description of Problem:
I'm encountering quite apparent leaks in that eventually make the app slower and slower, and then ultimately crashes with out of memory. This happens without fail in my attached project, when running on a device. Running it with instruments reveals that the allocated memory and persistent bytes keeps increasing every time a new view controller is pushed onto the nav stack. After the view controller is popped off the stack, the memory is never reclaimed, and will eventually show leaks. Keeping the sample running will eventually result in the app crashing.

There's also 4 (albeit very minor) leaks that show up even before the test is started.


Steps to reproduce the problem:
1. Open navleakcsharp.sln
2. Run this on a device (not simulator)
3. Click the "Market Info In Out" button
4a. Leave running until it crashes
4b. Launch with instruments and see the ever increasing usage of memory

Actual Results:
App leaks and will eventually crash.

Expected Results:
App shouldn't leak, app shouldn't crash.


How often does this happen? 
100% of the time.

Additional Information:
Comment 1 Alek Slater 2016-03-30 17:27:37 UTC
Created attachment 15559 [details]
csharp allocs
Comment 2 Alek Slater 2016-03-30 17:29:23 UTC
Created attachment 15560 [details]
Comparisons

I've now written the same kind of test app in 3 different languages, just to compare em (csharp,objc and swift), and the csharp implementation is the only one that leaks this badly.
Comment 3 Matt Jones 2016-04-01 12:40:35 UTC
Dear Xamarin, please look at this soon.
Comment 4 Rolf Bjarne Kvinge [MSFT] 2016-04-06 09:42:26 UTC
This happens because there is a reference cycle in MarketInfoViewController:

1) The managed MarketInfoViewController retains the native MarketInfoViewController instance (so the native MarketInfoViewController instance isn't freed)
2) The native MarketInfoViewController instance retains the UITableView (tableView) instance (so the native UITableView instance isn't freed)
3) The native UITableView instance has a strong GCHandle to the managed UITableView instance (because other native code has retained the native UITableView instance), so the garbage collector doesn't collect the managed UITableView instance.
4) The managed UITableView instance contains a reference to WeakDataSource and WeakDelegate (this is by design).
5) WeakDataSource and WeakDelegate is really the managed MarketInfoViewController from point 1, thus completing the cycle.

This means that any instance of MarketInfoViewController and anything it references are never freed.

The fix is to null out WeakDataSource and WeakDelegate in ViewDidAppear/ViewDidDisappear:

	public override void ViewDidAppear (bool animated)
	{
		tableView.WeakDataSource = this;
		tableView.WeakDelegate = this;

		base.ViewDidAppear (animated);
	}

	public override void ViewDidDisappear (bool animated)
	{
		tableView.WeakDataSource = null;
		tableView.WeakDelegate = null;

		base.ViewDidDisappear (animated);
	}

that resolves the leak.
Comment 5 Alek Slater 2016-04-06 10:28:27 UTC
Is this approach also needed when using UITableViewController subclasses?
Comment 6 Matt Jones 2016-04-06 11:13:15 UTC
Thanks Rolf, this is really useful info. It should be in the samples.
Comment 7 Alek Slater 2016-04-06 11:14:22 UTC
Oh also for the record, if I have a UITableView in a binding, would I have to null these "weak" (which apparently are not) delegate / datasources? there too?
Comment 8 Alek Slater 2016-04-06 12:14:23 UTC
I tried your fix out, and yes its true, it removes the leak, but it also never shows any data in the table. As soon as I insert a call to ReloadData() after resetting the datasource/delegate to the viewcontroller, the data shows ok, but the leak comes back.
Comment 9 Alek Slater 2016-04-06 12:54:18 UTC
Created attachment 15625 [details]
Cleaned up and improved reproducable sample with suggested fix that doesnt work

I've cleaned up the project a little bit, removing bits that arent needed. I've added the fix in there along with the call to reload the data so there's actual data shown in the table. And it still leaks.

I also added a UITableViewController subclass version that can be used by commenting out the line in the top of the MarketInfo.cs file
Comment 10 Sebastien Pouliot 2016-04-11 12:35:41 UTC
A call to the sample's ReloadData* will show leak again, for similar reasons. From a quick look this weekend it seemed to related to labels (CALayer) in cells.

* just calling `tableView.ReloadData()` does not leak
Comment 11 Matt Jones 2016-05-06 14:56:12 UTC
If I'm not mistaken there is no workaround so could you please take another look at this ASAP?
Comment 12 Alek Slater 2016-05-09 11:34:40 UTC
Did some more testing and these are my findings:

1) Running the sample, it will crash after the 175th attempt.
2) Commenting out the UILabels in the UITableViewCell custom class I have, the app was still going after 1712 attempts, so I think there might be an issue with custom UITableViewCell classes.
Comment 13 Alek Slater 2016-05-10 09:30:55 UTC
I've tried everything I can think of, in a binding, without a binding, in UITableViewController subclasses, inside a normal UIViewController.

At the moment, I can see no way to avoid the leaks caused by even a very rudimentary UITableViewCell subclass that only has 2 labels. All I need is some guidance as to what or how to write the code to avoid or at the very least limit the leaks. 

If i stick the whole UITableView code into an objective-c binding, I can limit the leaks, but it only gets me 10 more re-entries before the app crashes again due to the leaks.

1) Do I have to manually dispose all UITableViewCells (is it even possible?), even when they are inside a binding? 

2) How to use UITableView's in Xamarin.iOS without leaks?

3) I really think this is a bug that should be looked at, I can't imagine this is by design.

4) Please help
Comment 14 Rolf Bjarne Kvinge [MSFT] 2016-05-13 13:33:28 UTC
The sample from comment #9 works fine for me in the simulator, runs without any leaks.

Which version of Xamarin.iOS are you testing with? Can you add the complete version information from Xamarin Studio's About menu (click on Show Details to get the full information)?

If you're not profiling a debug build in the simulator, you should also call GC.Collect every iteration, otherwise the GC won't necessarily run and it would look like you have a lot of leaks when it's just the GC waiting (for debug builds in the simulator Xamarin.iOS will automatically call GC.Collect every second).

The best way to diagnose potential leaks is to use instruments. Here's a simple video:

https://www.dropbox.com/s/th7dblfl2pajjz7/InstrumentsLeak1.mov?dl=0

note that the graph flats out after launching, this means there are no leaks.

To get more information you can take snapshots, and compare memory usage between them:

https://www.dropbox.com/s/5q2rpxp364yk9ak/InstrumentsLeak2.mov?dl=0

Here I took a snapshot every time the app turned green, and as you can see the memory usage between each snapshot turns to 0 after a while (there is some residual memory usage from iOS itself, which is why not all comparisons are exactly 0, but instead a few kB. This is normal, and will eventually reach 0 if you run the app long enough).
Comment 15 Alek Slater 2016-05-13 14:31:03 UTC
You won't get any leaks in the simulator Rolf, you need to run it on a device. Best one is a device like an iPhone, I've been using my iPhone+.

I've been profiling using both Instruments and your Xamarin Profiler, and I can confirm in both cases there's a massive amount of leaks going on. You normally have to leave it running until the 170th re-entry before it crashes, but that will depend on how much RAM you have available on your device.

I'm very confused about the GC.Collect stuff, you post all over the place that doing GC.Collects will make no difference, so yer saying in this case we should be doing GC.Collect? At what point do we call GC.Collect?

I've been doing this with the debugger connected anyway, so I suppose GC is collecting like you said every second. It crashes with me connected to the debugger, it crashes with me connected with the profiler, and it crashes when I just run the app. Always after around 160 - 180 re-entries.

I've attached the system info / versions in "system_info.txt" , and some device output in "device_output.txt".

+ I've attached a video with me profiling on my device.
There's a few minor leaks even before I start the thing, but after it starts it seems pretty good, but notice the persistant bytes, it keeps growing, never clearing out.
When it reaches 120 or so, the leaks come out, and when it gets to 163 the entire system is unable to allocate more memory, causing other apps to crash / fail to work.

Normally when I profile it all ends with this output here and the app crashing:
May 13 21:58:22 skeliphone navleakcsharp[1797] <Error>: navleakcsharp(1797,0x39c49000) malloc: *** mach_vm_map(size=1048576) failed (error code=3)
	*** error: can't allocate region securely
	*** set a breakpoint in malloc_error_break to debug

But in the video, I actually saw proof that this causes other apps and processes to fail, which is arguably a bit worse.

I've also attached an updated solution, with the projects Leak and LeakBound, which both leak, although LeakBound leaks a tiny bit less than the Leak project.
(It's the same as the ones you made the video with except I've renamed them, added some console output to better keep track of things)

If this isn't leaking like a bucket full of holes, whats going on?
Comment 16 Alek Slater 2016-05-13 14:31:36 UTC
Created attachment 15993 [details]
System Info
Comment 17 Alek Slater 2016-05-13 14:32:01 UTC
Created attachment 15994 [details]
Device Output
Comment 18 Alek Slater 2016-05-13 14:34:54 UTC
Created attachment 15995 [details]
Updated Solution
Comment 19 Alek Slater 2016-05-13 14:43:36 UTC
Just waiting for dropbox to sync, and I'll put a link to the video here...
Comment 20 Alek Slater 2016-05-13 14:52:43 UTC
Here's my video:
https://www.dropbox.com/s/7gtnglx04p27p8u/Leak1.mov?dl=0
Comment 21 Matt Jones 2016-06-01 08:46:17 UTC
Status: NeedInfo. How much more info do you need?
Comment 22 Rolf Bjarne Kvinge [MSFT] 2016-06-01 09:24:23 UTC
I'll have a look at the updated solution from comment #18 when I can find some time (it might take a little while).
Comment 23 Matt Jones 2016-08-10 13:29:11 UTC
We're really being hung out to dry on this one - these leaks are a big issue for us. Please look at the information we have provided soon.
Comment 24 Matt Jones 2016-09-07 10:09:59 UTC
Any news on this?
Comment 25 Rolf Bjarne Kvinge [MSFT] 2016-09-08 17:58:23 UTC
In your MarketInfoViewController you have a circular reference:

    MarketInfoViewController -> tableView.WeakDelegate / WeakDataSource -> MarketInfoViewController

so if in your MarketInfoViewController.DestroyView method you add this:

    tableView.WeakDelegate = null;
    tableView.WeakDataSource = null;

you break the circular reference and the memory will be reclaimed once the GC runs.

Note that while testing, you may want to run the GC manually for each iteration (otherwise you can end up waiting a long time), so just add this (for instance in your DestroyView method):

    GC.Collect ();

but remove it when not doing memory profiling.
Comment 26 Alek Slater 2016-09-09 07:00:58 UTC
Ok, then what about UITableViewController subclasses, does that work there too? Have you tried that?
Comment 27 Rolf Bjarne Kvinge [MSFT] 2016-09-09 09:40:58 UTC
@Alek, I don't quite understand your question, can you explain a bit more about what you're trying to do?