Solving MvvmLight’s EventToCommand Memory Leak (WP7)

By using MvvmLight‘s EventToCommand you are going to leak serious memory.
Controls and whole Pages you navigate to will never be garbage collected.
Won’t be so “Light” after all if your not carefull.

Some links on the issue here and here.

First let’s try to get a good understanding on why and how this happens and then we will solve it ourselves!

All the code included below for your pleasure 😉

Lets analyze the resulting object reference graph and visualize the leak.

Your ViewModel keeps a reference to a Command.

• ViewModel -> Command

EventToCommand hooks onto CanExecuteChanged of the Command and therefore EventToCommand will stay alive as long as the Command stays alive.

• Command -> EventToCommand

EventToCommand is a TriggerAction which means it keeps a reference to the element it has attached on (AssociatedObject property).

• EventToCommand -> FrameworkElement

UIElements in the visual tree keep a reference to their parent which recursively means they hold on to the Page.

• FrameworkElement -> Page

So we have this reference chain : ViewModel -> Command -> EventToCommand -> FrameworkElement -> Page

So every Page will live in memory as long as all ViewModels referenced by EventToCommands inside it stay alive.Now its commonplace for ViewModels to stay alive for a long time through an IoC or even being statically declared in App.xaml and so there we go.We found it!
Sounds easy and obvious now but it wasn’t so easy to locate.It took plenty of time fiddling around with the Windows Phone Performance Analysis Tool.Excellent tool.Could be better.Huge improvement compared to WinDbg 🙂

Ok so lets solve it.Whats the weakest link in that chain?

You guessed it its the CanExecuteChanged event.
The command does not need to reference EventToCommand.
This is where WeakEventListener comes in.Its not the purpose of this post to explain how WeakEvents and listeners work so go ahead and do some reading on the net to get you acquainted.

We know EventToCommand adds and removes an EventHandler to/from Command.CanExecuteChanged.
What would be perfect is if EventToCommand itself added a WeakEventListener to the Command and solve this thing for us.
UPDATE: Apparently MvvmLight is maybe trying to “solve” this issue in the next release but in a way that
totally doesn’t make sense to me.By using WeakActions inside of RelayCommand.I have opened a discussion on the issue here with detail explanation.

For those interested here’s one way of solving it inside EventToCommand that i found just playing around (without a lot of design and classes) :

//Need this instance field in order to unhook the correct handler
private EventHandler hSafeCanExecuteChanged = null;
private static void OnCommandChanged(EventToCommand element, DependencyPropertyChangedEventArgs e)
{
	//Effectively the idea here is that instead of implicitly "closure-ing" this (meaning EventToCommand)
	//inside of the EventHandler we don't access anything from the surrounding environment and instead
        //pass this as a WeakReference thus allowing us to be GCed.
	if (element != null)
	{
		if (e.OldValue != null)
		{
			((ICommand)e.OldValue).CanExecuteChanged -= element.hSafeCanExecuteChanged;
			element.hSafeCanExecuteChanged = null;
		}
		ICommand newValue = (ICommand)e.NewValue;
		if (newValue != null)
		{
			var weakElement = new WeakReference(element);
			element.hSafeCanExecuteChanged =
				(o, args) =>
				{
					var evtToCommand = weakElement.Target as EventToCommand;
					if (evtToCommand != null)
						evtToCommand.OnCommandCanExecuteChanged(args);
				};
			newValue.CanExecuteChanged += element.hSafeCanExecuteChanged;
		}
		element.EnableDisableElement();
	}
}

UPDATE: Here is the whole EventToCommand with the fix:

public class EventToCommand : TriggerAction<FrameworkElement>
{
	#region Command Dependency Property

	public ICommand Command
	{
		get { return (ICommand)GetValue(CommandProperty); }
		set { SetValue(CommandProperty, value); }
	}

	public static readonly DependencyProperty CommandProperty =
		DependencyProperty.Register("Command",
									typeof(ICommand),
									typeof(EventToCommand),
									new PropertyMetadata(null, OnCommandPropertyChanged));

	private static void OnCommandPropertyChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
	{
		var instance = d as EventToCommand;
		if (instance != null)
			instance.OnCommandChanged((ICommand)e.OldValue, (ICommand)e.NewValue);
	}

	#endregion Command Dependency Property

	#region CommandParameter Dependency Property

	public object CommandParameter
	{
		get { return (object)GetValue(CommandParameterProperty); }
		set { SetValue(CommandParameterProperty, value); }
	}

	public static readonly DependencyProperty CommandParameterProperty =
		DependencyProperty.Register("CommandParameter",
									typeof(object),
									typeof(EventToCommand),
									new PropertyMetadata(null, OnCommandParameterPropertyChanged));

	private static void OnCommandParameterPropertyChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
	{
		var instance = d as EventToCommand;
		if (instance != null && instance.AssociatedObject != null)
			instance.EnableDisableElement();
	}

	#endregion CommandParameter Dependency Property

	#region MustToggleIsEnabled Dependency Property

	public bool MustToggleIsEnabled
	{
		get { return (bool)GetValue(MustToggleIsEnabledProperty); }
		set { SetValue(MustToggleIsEnabledProperty, value); }
	}

	public static readonly DependencyProperty MustToggleIsEnabledProperty =
		DependencyProperty.Register("MustToggleIsEnabled",
									typeof(bool),
									typeof(EventToCommand),
									new PropertyMetadata(OnMustToggleIsEnabledPropertyChanged));

	private static void OnMustToggleIsEnabledPropertyChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
	{
		var instance = d as EventToCommand;
		if (instance != null && instance.AssociatedObject != null)
			instance.EnableDisableElement();
	}

	#endregion MustToggleIsEnabled Dependency Property

	#region Public
	public bool PassEventArgsToCommand { get; set; }

	public object CommandParameterValue
	{
		get
		{
			return (this._commandParameterValue ?? this.CommandParameter);
		}
		set
		{
			this._commandParameterValue = value;
			this.EnableDisableElement();
		}
	}
	public void Invoke()
	{
		this.Invoke(null);
	}
	#endregion

	private object _commandParameterValue;
	private WeakEventListener<EventToCommand, Object, EventArgs> weakCanExecuteChangedListener;

	protected void OnCommandChanged(ICommand oldCommand, ICommand newCommand)
	{
		if (oldCommand != null)
		{
			weakCanExecuteChangedListener.Detach();
			weakCanExecuteChangedListener = null;
		}
		if (newCommand != null)
		{
			weakCanExecuteChangedListener = new WeakEventListener<EventToCommand, object, EventArgs>(this);
			weakCanExecuteChangedListener.OnEventAction =
				(me, sender, args) =>
				{
					me.EnableDisableElement();
				};
			weakCanExecuteChangedListener.OnDetachAction =
				(weakListener) =>
				{
					newCommand.CanExecuteChanged -= weakListener.OnEvent;
				};
			newCommand.CanExecuteChanged += weakCanExecuteChangedListener.OnEvent;
		}
		this.EnableDisableElement();
	}

	protected override void Invoke(object parameter)
	{
		if (!this.AssociatedElementIsDisabled())
		{
			ICommand command = this.GetCommand();
			object commandParameterValue = this.CommandParameterValue;
			if ((commandParameterValue == null) && this.PassEventArgsToCommand)
			{
				commandParameterValue = parameter;
			}
			if ((command != null) && command.CanExecute(commandParameterValue))
			{
				command.Execute(commandParameterValue);
			}
		}

	}

	protected void EnableDisableElement()
	{
		Control associatedObject = this.GetAssociatedObject();
		if (associatedObject != null)
		{
			if (this.ReadLocalValue(MustToggleIsEnabledProperty) != DependencyProperty.UnsetValue
				&& MustToggleIsEnabled)
			{
				ICommand command = this.GetCommand();
				if (command != null)
				{
					associatedObject.IsEnabled = command.CanExecute(this.CommandParameterValue);
				}
			}
		}
	}
	protected override void OnAttached()
	{
		base.OnAttached();
		this.EnableDisableElement();
	}

	#region Private
	private Control GetAssociatedObject()
	{
		return (base.AssociatedObject as Control);
	}
	private ICommand GetCommand()
	{
		return this.Command;
	}
	private bool AssociatedElementIsDisabled()
	{
		Control associatedObject = this.GetAssociatedObject();
		return ((associatedObject != null) && !associatedObject.IsEnabled);
	}
	#endregion

}

Until that happens though lets hack our way around it cause that’s more fun shall we.
We know also we have to extend it but we do not have access unfortunately to any of its internals.We do have access though to the Command DP since it has to be public.What could we do?
How about replacing (behind the scenes) the original Command with our own Wrapper (Adapter) Command?
Lets call it WeakCommand.It will internally hold on to the original command but this time we will listen to it using a WeakEventListener. WeakCommand will of course itself be an ICommand which will just delegate all calls to and from the original.Nice.Seems that might do the trick.
So how do we “replace” the original command that ETC gets data bound to? We need to listen to it for changes and when it changes we will update it by calling
ETC.Command = new WeakCommand(ETC.Command);

NOTE: This will remove the ViewModel-Command Binding! If for some reason you change your Command from within your ViewModel and expect it to rebind this won’t happen.
That’s an uncommon case though.
This might be solvable as well but not sure yet.Am thinking something along the lines of grabbing the original BindingExpression (and therefore the Binding object) from the Command DP and apply that to BindingListener.Will look into that as well.

This will call ETC’s code to unhook from the Original command (what we wanted) and then immediatelly hook onto our own WeakCommand.
How do we listen for Dependency Property changes when there is no event though?
Easy.We’ll make a BindingListener class with one generic DependencyProperty of type Object and bind that to the Command DP of ETC.This way when ETC.Command changes that surrogate is gonna notify us so we know to replace the command.The binding surrogate is not at all new btw.It’s probably as old as Silverlight.
Hmm.Sounds good but will that work? Of course it will.Would i be making this post if it didnt? 😛

Lets start one by one.The BindingListener :

///
<summary> /// Helper class to be able to listen for DependencyProperty Changes (for example Visibility)
/// </summary>
public class BindingListener : DependencyObject
{
	private readonly Action<Object, Object> valueChangedCallback;

	public BindingListener(Action<Object,Object> valueChangedCallback)
	{
		if (valueChangedCallback == null)
			throw new ArgumentNullException("valueChangedCallback");
		this.valueChangedCallback = valueChangedCallback;
	}

	public object Value
	{
		get { return (object)GetValue(ValueProperty); }
		set { SetValue(ValueProperty, value); }
	}
	public static readonly DependencyProperty ValueProperty =
		DependencyProperty.Register("Value", typeof(object), typeof(BindingListener), new PropertyMetadata(null, OnValuePropertyChanged));

	private static void OnValuePropertyChanged(DependencyObject d, DependencyPropertyChangedEventArgs e)
	{
		var source = d as BindingListener;
		if (source != null)
		{
			if (!source.valueChangingBecauseOfUnbind)
			{
				source.valueChangedCallback(e.OldValue, e.NewValue);
			}
		}
	}

	public void Bind(Object source, String sourceProperty)
	{
		var binding = new System.Windows.Data.Binding(sourceProperty);
		binding.Source = source;
		System.Windows.Data.BindingOperations.SetBinding(this, ValueProperty, binding);
	}
	private bool valueChangingBecauseOfUnbind;
	public void UnBind()
	{
		valueChangingBecauseOfUnbind = true;
		Value = null;
		valueChangingBecauseOfUnbind = false;
	}

}

Next up WeakCommand :

///
<summary> /// What it all means (Goal: WE should be able to be garbage collected) :
/// - WE hold a reference to the long living wrapped command
/// - The long living command will hold the Listener instead and NOT us therefore we can die
/// - When we die the long living command stills holds the listener so on the next event
/// he fires the listener will be removed as well.
/// </summary>
public class WeakCommand : ICommand, IDisposable
{
	private WeakEventListener<WeakCommand, Object, EventArgs> weakListener;
	private ICommand wrapped;
	public WeakCommand(ICommand wrapped)
	{
		this.wrapped = wrapped;
		weakListener = new WeakEventListener<WeakCommand, Object, EventArgs>(this);
                //Don't worry.You are not alone.It IS really tricky to understand this 😛
		weakListener.OnEventAction =
			(me, wrappedCommand, args) =>
			{
				//DON'T CLOSURE ANYTHING HERE.USE PARAMETERS ONLY
				if (me.CanExecuteChanged != null)
					me.CanExecuteChanged(wrappedCommand, args);
			};
		weakListener.OnDetachAction =
			(listener) =>
			{
				//If I die then remove the Listener instance as well from wrapped command
				wrapped.CanExecuteChanged -= listener.OnEvent;
			};
		wrapped.CanExecuteChanged += weakListener.OnEvent;
	}

	#region IDisposable
	private bool isDisposed;
	public void Dispose()
	{
		if (!isDisposed)
		{
			weakListener.Detach();
			weakListener = null;
			wrapped = null;
			isDisposed = true;
		}

	}
	#endregion

	#region ICommand
	public bool CanExecute(object parameter)
	{
		ThrowIfDisposed();
		return wrapped.CanExecute(parameter);
	}

	public event EventHandler CanExecuteChanged;

	public void Execute(object parameter)
	{
		ThrowIfDisposed();
		wrapped.Execute(parameter);
	}
	#endregion

	private void ThrowIfDisposed()
	{
		if (isDisposed)
			throw new ObjectDisposedException("WeakCommand");
	}
}

Lets get WeakEventListener in there as well:

// (c) Copyright Microsoft Corporation.
// This source is subject to the Microsoft Public License (Ms-PL).
// Please see http://go.microsoft.com/fwlink/?LinkID=131993 for details.
// All other rights reserved.

using System.Diagnostics.CodeAnalysis;
using System;

namespace WeakEventListener
{
	///
<summary> /// Implements a weak event listener that allows the owner to be garbage
 /// collected if its only remaining link is an event handler.
 /// </summary>
	/// Type of instance listening for the event.
	/// Type of source for the event.
	/// Type of event arguments for the event.
	[SuppressMessage("Microsoft.Performance", "CA1812:AvoidUninstantiatedInternalClasses", Justification = "Used as link target in several projects.")]
	public class WeakEventListener where TInstance : class
	{
		///
<summary> /// WeakReference to the instance listening for the event.
 /// </summary>
		private WeakReference _weakInstance;

		///
<summary> /// Gets or sets the method to call when the event fires.
 /// </summary>
		public Action OnEventAction { get; set; }

		///
<summary> /// Gets or sets the method to call when detaching from the event.
 /// </summary>
		public Action OnDetachAction { get; set; }

		///
<summary> /// Initializes a new instances of the WeakEventListener class.
 /// </summary>
		///Instance subscribing to the event.
		public WeakEventListener(TInstance instance)
		{
			if (null == instance)
			{
				throw new ArgumentNullException("instance");
			}
			_weakInstance = new WeakReference(instance);
		}

		///
<summary> /// Handler for the subscribed event calls OnEventAction to handle it.
 /// </summary>
		///Event source.
		///Event arguments.
		public void OnEvent(TSource source, TEventArgs eventArgs)
		{
			TInstance target = (TInstance)_weakInstance.Target;
			if (null != target)
			{
				// Call registered action
				if (null != OnEventAction)
				{
					OnEventAction(target, source, eventArgs);
				}
			}
			else
			{
				// Detach from event
				Detach();
			}
		}

		///
<summary> /// Detaches from the subscribed event.
 /// </summary>
		public void Detach()
		{
			if (null != OnDetachAction)
			{
				OnDetachAction(this);
				OnDetachAction = null;
			}
		}
	}
}

Finally its time to start messing with EventToCommand.Here it is :

public class EventToCommandEx : GalaSoft.MvvmLight.Command.EventToCommand
{
	///
<summary> /// Helper to be able to listen to Command changes.
 /// </summary>
	private BindingListener commandBindingListener;

	protected override void OnAttached()
	{
		base.OnAttached();
		commandBindingListener = new BindingListener(
			(oldValue, newValue) =>
				{
					var weakOld = oldValue as WeakCommand;
					if(weakOld != null)
					{
						weakOld.Dispose();
					}
					if (newValue is WeakCommand)
						return; //We were called because we changed the Command below.Ignore
					var newCommand = newValue as ICommand;
					if (newCommand != null)
					{
						this.Command = new WeakCommand(newCommand);
					}
				});
		commandBindingListener.Bind(this, "Command");
	}

	protected override void OnDetaching()
	{
		base.OnDetaching();
		commandBindingListener.UnBind();
		commandBindingListener = null;
	}
}

Have fun looking at those pages getting GCed! Full Source Code Here

If you have read so far then i assume you are interested (hacking IS fun :P) so lets take a look at another way of doing it.
This also has limitations.I’ll get to those after the code.

public class EventToCommandEx : GalaSoft.MvvmLight.Command.EventToCommand
{
	protected override void OnAttached()
	{
		base.OnAttached();
		this.AssociatedObject.Loaded += OnAssociatedObjectLoaded;
		this.AssociatedObject.Unloaded += OnAssociatedObjectUnloaded;
	}

	private ICommand commandBackup;
	protected virtual void OnAssociatedObjectUnloaded(object sender, RoutedEventArgs e)
	{

		if (Command != null)
		{
			commandBackup = this.Command;
			this.Command = null; //Note : We kill the Binding with this.
		}
		else
			commandBackup = null;

	}
	protected virtual void OnAssociatedObjectLoaded(object sender, RoutedEventArgs e)
	{

		if (commandBackup != null)
		{
			//Restore the Command
			this.Command = commandBackup;
		}
		commandBackup = null;

	}
	protected override void OnDetaching()
	{
		base.OnDetaching();
		this.Command = null; //unhook from CanExecuteChanged
		commandBackup = null;
		this.AssociatedObject.Loaded -= OnAssociatedObjectLoaded;
		this.AssociatedObject.Unloaded -= OnAssociatedObjectUnloaded;
	}
}

Ok.So what did we try to do.The idea is that since EventToCommand never unhooks from the event
(Dont be fooled by the Detach method.This is called when the Trigger is removed from the TriggerCollection) a good time to unhook it is probably when the element is removed from the visual tree.Either it was defined in a DataTemplate and the collection binding changed so everything gets refreshed or the user left the Page containing it.Pretty safe.Well not exactly.You have to account for the cases where the same element for some reason gets removed and re-added. That is why, after removing the Command to avoid the leak, we keep a reference to it in case it gets re-added in which case we re-apply.Seems to work nicely but you need to keep these in mind :

• The Binding gets removed when you set Command = null (same as BindingOperations.ClearBinding() in WPF). If you don’t change the Command from inside the VM ,which i expect you don’t, then it shouldn’t be a problem.
• You have to know for sure that the element you are attaching to fires Loaded/Unloaded events as expected! I noticed a case with a BindableApplicationBar wrapper that this did not happen.