Fix for BindingExpression memory leak

[jonathanpeppers]

Description of Change

In our application we discovered a memory leak in the following situation:

• Have a long-lived ViewModel object (maybe lives the lifetime of your app)
• Bind to it in a View
• The View goes away, and get's GC'd
• A BindingExpression object and its children remain in memory as long as the long-lived ViewModel

Bugs Fixed

• Not sure if there is an issue out for this, see my unit test that reproduces the issue

API Changes

None

Behavioral Changes

BindingExpression will not remain in memory when the View is GC'd.

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

[dnfclas]

Hi @jonathanpeppers, I'm your friendly neighborhood .NET Foundation Pull Request Bot (You can call me DNFBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by .NET Foundation and real humans are currently evaluating your PR.

TTYL, DNFBOT;

[StephaneDelcroix]

Instead of this, couldn't you reuse the https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Core/WeakEventManager.cs ?

I'm uneasy about having multiple classes doing very similar stuffs.

[StephaneDelcroix]

Nasty one...

I can reproduce the bug, and the fix solves it. I'd like to get rid, if possible, of the WeakPropertyChangedProxy. @hartez could you look at this with @jonathanpeppers ?

Once this is solved, you'll get my 👍 , until then, you'll have to do with my :<3:

[jonathanpeppers]

@StephaneDelcroix @hartez

I did a quick attempt at changing this, but it seems like WeakEventManager has to be used by the class that is sending the event. Binding and its inner workings are the subscribers to any class implementing INotifyPropertyChanged. A developer could be binding any POCO here, as long as it was INotifyPropertyChanged.

I patterned WeakPropertyChangedProxy after WeakNotifyProxy I found inside ListView, and added an option to unsubscribe: https://github.com/xamarin/Xamarin.Forms/blob/master/Xamarin.Forms.Core/ListProxy.cs#L361

Let me know if I'm missing something here. I did feel like I was adding a class that might already exist.

[hartez]

@StephaneDelcroix @jonathanpeppers Jonathan's right, there's no simple way to use one in place of the other; WeakEventManager is tightly coupled to its event source, and WeakPropertyChangedProxy is tightly coupled to INotifyPropertyChanged (as WeakNotifyProxy in ListView is to INotifyCollectionChanged). They're all really doing different things.

There might be a way to consolidate everything they do into one class, but I suspect we'd have to do a lot of reflection to get the event handlers attached and I don't know that it'd be worth the performance hit.

[jamesl77]

This is a serious bug. Has this been released in any NuGet package?
I can't find it in 2.3.3.152-pre2 nor in 2.3.3.163-pre3....
I'm confused, this is a serious bug which was fixed...

[jamesl77]

This memory leak bug was fixed 1.5 months ago, when do you plan to release it?

[samhouts]

It was released in 2.3.2-pre2. It was just missed in the release notes.

[jamesl77]

@samhouts thanks. Looking at the commits, I see this one actually got into beta-2.3.3-pre1, am I correct?
I'm trying to make sure I understand the commits correctly.

[samhouts]

I misspoke and updated my comment. It was actually released in 2.3.2-pre2 and has been in each subsequent build.

[jamesl77]

On https://github.com/xamarin/Xamarin.Forms/commits/beta-2.3.3-pre1 I see it shows all commits up to September 1. Jason merged this PR on August 16.
Doesn't this mean it got into beta-2.3.3-pre1?

[samhouts]

You'll see it is also here: https://github.com/xamarin/Xamarin.Forms/commits/beta-2.3.2-pre2

[jamesl77]

Oh, the first NuGet prerelease for 2.3.3 is 2.3.3.152-pre2 on Sept 15... There was no 2.3.3 pre1 NuGet release

[jamesl77]

This kind of bugs are so hard to spot...Thank you @jonathanpeppers for this PR.... I wonder what other mem leak bugs are out there...

[jamesl77]

@jonathanpeppers how did you spot this memory leak by the way? Did you use any tools?
Does anyone know? It might be useful for anyone who gets into issues...

[jonathanpeppers]

@jamesl77 we have some heavy screens with a lot of bindings. One in particular we could navigate to, dismiss, then it would get slower and slower each time.

To figure it out, I broke portions of our code into unit tests of this pattern:

WeakReference weakRef;

{
  //Do stuff that might cause a leak

  //Assign weakRef
  weakRef = new WeakReference(yourObject);
}

GC.Collect();
await Task.Delay(10);
GC.Collect();

Assert.IsFalse(bindingRef.IsAlive, "This thingy should not be alive!");

After I fixed all the leaks that were our fault (related to events), I discovered I could recreate the issue in Forms with this test.

After the leak was fixed, I used Heapshot/Xamarin profiler to make sure I really fixed it.

[jamesl77]

Thank you. I'm also using a lot of bindings and other stuff.

I have an issue with Messenger, I made a question on SO: http://stackoverflow.com/questions/40163598/does-messenger-really-requires-calling-unscribe