Memory leak using "Tibet" binding with WinStore and WPF mvvmcoss apps

[ljeancler]

Hi,

I recently noticed with a MvvmCross WPF app that some viewmodels never get finalized. And these viewmodels were very basic (only few string properties, no event handler, one command).

I finally found out that the leak was related to the binding between the View and the ViewModel through the "Tibet" syntax : mvx:Bi.nd="Text Username". That's very weird, since the View get finalized but not the ViewModel.

I've replaced the "Tibet" syntax with the WPF standard's counterpart (Text="{Binding Username, Mode=TwoWay, UpdateSourceTrigger=PropertyChanged}") and memory leaks went away...

Is there something I've missed ? Do we have to manually unregister "Tibet" binding ?

Since "Tibet" binding is awesome, I really want to use it.

Best regards,

edit: using MvvmCross 3.0.14

[slodge]

Doesn't look like you've missed anything.

Do you have a sample and instructions demonstrating the leak?

[ljeancler]

Sure, preparing a sample WPF project right now...

[ljeancler]

Hi,

You can download the Vs2012 solution reproducing the memory leak using "Tibet" binding here : http://sdrv.ms/1m2Yqpe

The application is very basic but uses my own navigation service (keep history of living viewmodels, navigation back, forward, jump, passing object to ViewModel using a shared context object).

To reproduce the issue, simply navigate a few times to the second viewmodel : 1) from the first view model using the "Navigate to next ViewModel..." button and then go back using the "GoBack" button and repeat step 1) 2) from the third view using the "GoBack" button

Be sure to navigate to the "Second ViewModel" more than once...

Now go back to the first viewmodel for exemple, and click on the "Show Instances" button. A dialog will appear with maybe a lot of living viewmodels and views.

Now click on the "GC.Collect" button to force garbage collection, then click on the "Show Instances" and you will see : 1 FirstViewModel, 1 FirstView and a lot of SecondViewModel.

If you change the binding in the SecondView.xaml and use the WPF standard, you will notice that SecondViewModel is well collected by GC.

Will look into the sources of MvvmCross myself if I find the time (but not this week). Windows Store seems to be affected too (did'nt test other platforms)

Best regards and best wishes for 2014

Laurent.

[slodge]

Thanks - good sample

I've had a play this morning by adding a SubObject to SecondViewModel and tracing whether the bindings are being unsubscribed:

public class SubObject
    : INotifyPropertyChanged
{
    private PropertyChangedEventHandler _PropertyChanged;
    public event PropertyChangedEventHandler PropertyChanged
    {
        add
        {
            _PropertyChanged += value;
            Mvx.Trace("Subsription Added");
        }
        remove
        {
            _PropertyChanged -= value;
            Mvx.Trace("Subsription Removed");
        }
    }

    protected virtual void OnPropertyChanged(string s)
    {
        OnPropertyChanged(new PropertyChangedEventArgs(s));
    }

    protected virtual void OnPropertyChanged(PropertyChangedEventArgs e)
    {
        var handler = _PropertyChanged;
        if (handler != null) handler(this, e);
    }

    private string _thing = "Hello";
    public string Thing 
    {   
        get { return _thing; }
        set { _thing = value; OnPropertyChanged("Thing"); }
    }
}

This seems to show that the unsubscription (which ultimate calls BindingOperations.ClearBinding) is no longer being correctly called.

I'll need to dig a bit deeper (got to head off to work now), but thanks again for this - helps a lot

Stuart

(Also adding a link to http://msdn.microsoft.com/en-us/library/ms742863%28v=vs.110%29.aspx which is the basic technique we're using to create/add our bindings)

[slodge]

I've pushed some small changes to https://github.com/MvvmCross/MvvmCross/tree/Tibet-Leak-552 with debug wpf binaries at http://sdrv.ms/1fvoUlB (although really I would much rather share using a new github repo than using skydrive)

I need to check this solution some more as:

1. The ClearBinding public methods on WinPhone/WinRT BindingOperations are missing.... we may need to experiment with other forms of clear like:

              attachedObject.SetBinding(DataContextWatcherProperty, null);
              BindingOperations.SetBinding(attachedObject, DataContextWatcherProperty, null);

2. I'm sure when I first created this the Clear was coming from the operating system - I need to go back to my original experiments/test apps and play with this.

Thanks again for assisting with this test harness, and for your patience while we tweak this through...

Stuart

[slodge]

Have started porting your test app to WinRT - will finish tonight... then on to phone...

[ljeancler]

Thank you very much, I will look into your correction and make some tests. Great support !

[ljeancler]

Forgot to say, that I've workaround that problem on WPF by clearing binding myself in the Unloaded event handler of the my base WpfView :

    public BaseView()
    {
        //- See more at: http://www.e-naxos.com/Blog/post/StringFormat-une-simplification-Xaml-trop-peu-utilisee.aspx#sthash.obz2G6DB.dpuf
        this.Language = XmlLanguage.GetLanguage(Thread.CurrentThread.CurrentCulture.Name);

        this.Unloaded += BaseView_Unloaded;
    }

    private void RecurseClearAllBindings(DependencyObject parent)
    {
        if (parent == null)
            return;

        BindingOperations.ClearAllBindings(parent);
        var count = VisualTreeHelper.GetChildrenCount(parent);
        for (var i = 0; i < count; i++)
        {
            RecurseClearAllBindings(VisualTreeHelper.GetChild(parent, i));
        }
    }

    void BaseView_Unloaded(object sender, System.Windows.RoutedEventArgs e)
    {
        this.Unloaded -= BaseView_Unloaded;
        RecurseClearAllBindings(this as DependencyObject);  // solve the bind problem with mvvmcross Tibet
    }

...

[slodge]

I've tested the WindowsStore version of this - BindingOperations.ClearBinding isn't available there, so I had to use ClearValue instead - this seems to be working.

Will try to get on to WindowsPhone tomorrow

[slodge]

Have got windowsphone working now...

This sample works fine (no leaks) as long as I use the WindowsPhone back button for back navigation. However, if I use the navigation commands then there's a leak. I think this is due to the navigation service in the Wpf.Core app - I think this isn't doing back quite right for WindowsPhone's RootFrame right now.

I need to look at this more when I'm more awake - but I'm pretty sure it's not in the Mvx code...

Thanks again for the sample and the bug report

Stuart

[ljeancler]

That's weird for the WindowsPhone plateform... My navigation service is very basic, it only keeps a stack of living viewmodels and navigation is done with the ShowViewModel method of Mvx and a parameter with the navigation type (back, forward, jump)

Moreover, I've made a BasePhoneView interacting with the "real" NavigationService of WinPhone system : when the hardware back button is pressed, I cancel the windows phone navigation and start my own navigation (with my navigation service)

I will investigate that. I maybe have more work to do with Navigation and WindowsPhone (http://stackoverflow.com/questions/6572305/is-there-a-windows-phone-7-equivalent-to-android-nohistory-activity-attribute)

Thanks for your hard work.

Laurent

[slodge]

Thanks Laurent

on all the platforms navigating back is a different action to navigating forwards.

on winphone, this action is especially different as winphone keeps the entire navigation stack in memory. You can remove the last item if you want to - but beyond that the stack is a real stack abs it's better to goback rather than to keep adding more and more to the stack - look at rootframe for more info

sorry for mobile typing!

stuart

[ljeancler]

Yes you're absolutely right. I need to be more carefull about navigation systems/services for every plateforms to make a common portable system...

Thank's again.

Laurent

[ljeancler]

Hi, having redesigned my NavigationService to avoid memory leaks and I found another leak also related to binding. The leak is related to the ICommand implementation and MvxCommand is affected...

Please, could you consider rewriting MvxCommand implementation to avoid binding leak in a future version a MvvmCross ?

To avoid leaks, it's required to implement the weak event pattern... some links (unfortunatly WeakEventManager base class seems unavailable on PCL): http://social.msdn.microsoft.com/Forums/silverlight/en-US/34d85c3f-52ea-4adc-bb32-8297f5549042/command-binding-memory-leak?forum=silverlightbugs http://www.telerik.com/community/forums/silverlight/general-discussions/command-invalidatecanexecute.aspx http://stackoverflow.com/questions/3524597/example-implementation-of-weak-events-using-nets-weakeventmanager http://prism4.googlecode.com/svn/trunk/Prism4/PrismLibrary/Desktop/Prism/Commands/DelegateCommandBase.cs http://badecho.com/2012/05/a-generic-iweakeventlistener-implementation-part-ii/

Note: this is not a MvvmCross issue (ICommand binding leak seems to be a VERY common issue) Note2: Removing/Adding binding in the Unloaded/Loaded event handlers of visual controls solves the ICommand Binding issue...

Regards,

Laurent

[slodge]

Thanks

Mvx uses weak event binding internally for it's inpc bindings. For ICommand I was under the impression on Windows that this wasn't required. Will check your links when back home later.

[ljeancler]

You're (again) right for windows as long as you don't keep any reference to the ViewModel. If the ViewModel lives longer than the View and the View has some controls bound to a ICommand property of the ViewModel then the View will never be collected since a strong reference is set through CanExecuteChanged event (http://stackoverflow.com/questions/12919288/icommand-binding-causing-ui-memory-leak-in-wpf-application, http://stackoverflow.com/questions/19385486/button-leaks-when-using-icommand)

The way MvvmCross has been designed hide the issue (ViewModels and Views are re created when navigating). My application is more specific: I keep a reference to some ViewModels (to avoid recreation, to provide direct update and page composition).

You can also look at MvvmLight's implementation of the ICommand interface. Laurent Bugnion (creator of MvvmLight) has faced that problem too. I tried to replicate its implementation (He chose WeakAction and WeakFunc pattern...) but I failed with PCL (exception on WindowsPhone 7 = Silverlight) although MvvmLight PCL appears to work (I've tried it with Install-Package Portable.MvvmLightLibs -Version 4.1.27.6 the last version supporting WP7)

Using RelayCommand found at http://www.nuget.org/packages/Portable.MvvmLightLibs/4.1.27.6 and no more memory leak on my solution with my NavigationService... (Views are collected)

Laurent.

[slodge]

Do you fancy trying something like:

using System;
using System.Collections.Generic;

public class MvxCommand2Base
{
    private readonly List<WeakReference> _eventHandlers = new List<WeakReference>();

    public event EventHandler CanExecuteChanged
    {
        add
        {
            _eventHandlers.Add(new WeakReference(value));
        }
        remove
        {
            foreach (var thing in _eventHandlers)
            {
                if (thing.IsAlive
                    && ((EventHandler)thing.Target) == value)
                {
                    _eventHandlers.Remove(thing);
                    break;
                }
            }
        }
    }

    private List<EventHandler> SafeCopyEventHandlerList()
    {
        var toReturn = new List<EventHandler>();
        var deadEntries = new List<WeakReference>();

        foreach (var thing in _eventHandlers)
        {
            if (!thing.IsAlive)
            {
                deadEntries.Add(thing);
                continue;
            }
            var eventHandler = (EventHandler) thing.Target;
            if (eventHandler != null)
            {
                toReturn.Add(eventHandler);
            }
        }

        foreach (var weakReference in deadEntries)
        {
            _eventHandlers.Remove(weakReference);
        }

        return toReturn;
    }

    public void RaiseCanExecuteChanged()
    {
        var list = SafeCopyEventHandlerList();
        foreach (var eventHandler in list)
        {
            eventHandler(this, EventArgs.Empty);
        }
    }
}

public class MvxCommand2
    : MvxCommand2Base
    , IMvxCommand
{
    private readonly Func<bool> _canExecute;
    private readonly Action _execute;

    public MvxCommand2(Action execute)
        : this(execute, null)
    {
    }

    public MvxCommand2(Action execute, Func<bool> canExecute)
    {
        _execute = execute;
        _canExecute = canExecute;
    }

    public bool CanExecute(object parameter)
    {
        return _canExecute == null || _canExecute();
    }

    public bool CanExecute()
    {
        return CanExecute(null);
    }

    public void Execute(object parameter)
    {
        if (CanExecute(parameter))
        {
            _execute();
        }
    }

    public void Execute()
    {
        Execute(null);
    }
}

public class MvxCommand2<T>
    : MvxCommand2Base
    , IMvxCommand
{
    private readonly Func<T, bool> _canExecute;
    private readonly Action<T> _execute;

    public MvxCommand2(Action<T> execute)
        : this(execute, null)
    {
    }

    public MvxCommand2(Action<T> execute, Func<T, bool> canExecute)
    {
        _execute = execute;
        _canExecute = canExecute;
    }

    public bool CanExecute(object parameter)
    {
        return _canExecute == null || _canExecute((T) parameter);
    }

    public bool CanExecute()
    {
        return CanExecute(null);
    }

    public void Execute(object parameter)
    {
        if (CanExecute(parameter))
        {
            _execute((T) parameter);
        }
    }

    public void Execute()
    {
        Execute(null);
    }
}

Note that in Mvx we generally allow the view to have strong references on to the ViewModel - but not the other way around.

If this works, then we may slowly reintroduce this into the tip... but we'll have to do it carefully as it will break people's code... weak references to actions get garbage collected all too easily :/

[ljeancler]

Thanks, I gonna try it.

I agree with you about using weak references and about your concern with breaking changes. Maybe you could consider introducing these changes in a brand new class (as in your code sample) like "MvxWeakCommand".

So it will be a developper's choice to use it or not...

Regards,

Laurent

[slodge]

This is released in the latest beta - beta4

I've tested it in a few apps and it seems to work.

Closing this issue now - happy (very happy) to get other reports in new issues

Thanks again

Stuart