Why is CanExecute invoked after the command source is removed from the UI?

Question

I am trying to understand why CanExecute is invoked on a command source that has been removed from the UI. Here is a simplified program to demonstrate:

<Window x:Class="WpfApplication1.MainWindow"
        xmlns="http://schemas.microsoft.com/winfx/2006/xaml/presentation"
        xmlns:x="http://schemas.microsoft.com/winfx/2006/xaml"
        Height="350" Width="525">
    <StackPanel>
        <ListBox ItemsSource="{Binding Items}">
            <ListBox.ItemTemplate>
                <DataTemplate>
                    <StackPanel>
                        <Button Content="{Binding Txt}" 
                                Command="{Binding Act}" />
                    </StackPanel>
                </DataTemplate>
            </ListBox.ItemTemplate>
        </ListBox>
        <Button Content="Remove first item" Click="Button_Click"  />
    </StackPanel>
</Window>

Code-behind:

public partial class MainWindow : Window
{
    public class Foo
    {
        static int _seq = 0;
        int _txt = _seq++;
        RelayCommand _act;
        public bool Removed = false;

        public string Txt { get { return _txt.ToString(); } }

        public ICommand Act
        {
            get
            {
                if (_act == null) {
                    _act = new RelayCommand(
                        param => { },
                        param => {
                            if (Removed)
                                Console.WriteLine("Why is this happening?");
                            return true;
                        });
                }
                return _act;
            }
        }
    }

    public ObservableCollection<Foo> Items { get; set; }

    public MainWindow()
    {
        Items = new ObservableCollection<Foo>();
        Items.Add(new Foo());
        Items.Add(new Foo());
        Items.CollectionChanged += 
            new NotifyCollectionChangedEventHandler(Items_CollectionChanged);
        DataContext = this;
        InitializeComponent();
    }

    void Items_CollectionChanged(object sender, NotifyCollectionChangedEventArgs e)
    {
        if (e.Action == NotifyCollectionChangedAction.Remove)
            foreach (Foo foo in e.OldItems) {
                foo.Removed = true;
                Console.WriteLine("Removed item marked 'Removed'");
            }
    }

    void Button_Click(object sender, RoutedEventArgs e)
    {
        Items.RemoveAt(0);
        Console.WriteLine("Item removed");
    }
}

When I click the "Remove first item" button one time, I get this output:

Removed item marked 'Removed'
Item removed
Why is this happening?
Why is this happening?

"Why is this happening?" keeps being printed each time I click on some empty part of the window.

Why is this happening? And what can or should I do to prevent CanExecute from being invoked on removed command sources?

Note: RelayCommand can be found here.

Answers to Michael Edenfield questions:

Q1: Callstack of when CanExecute is invoked on removed button:

WpfApplication1.exe!WpfApplication1.MainWindow.Foo.get_Act.AnonymousMethod__1(object param) Line 30 WpfApplication1.exe!WpfApplication1.RelayCommand.CanExecute(object parameter) Line 41 + 0x1a bytes PresentationFramework.dll!MS.Internal.Commands.CommandHelpers.CanExecuteCommandSource(System.Windows.Input.ICommandSource commandSource) + 0x8a bytes PresentationFramework.dll!System.Windows.Controls.Primitives.ButtonBase.UpdateCanExecute() + 0x18 bytes PresentationFramework.dll!System.Windows.Controls.Primitives.ButtonBase.OnCanExecuteChanged(object sender, System.EventArgs e) + 0x5 bytes PresentationCore.dll!System.Windows.Input.CommandManager.CallWeakReferenceHandlers(System.Collections.Generic.List handlers) + 0xac bytes PresentationCore.dll!System.Windows.Input.CommandManager.RaiseRequerySuggested(object obj) + 0xf bytes

Q2: Also, does this keep happening if you remove all of the buttons from the list (not just the first?)

Yes.

Answer 1

The issue is that the command source (i.e. the button) does not unsubscribed from CanExecuteChanged of the command it is bound to, so that whenever CommandManager.RequerySuggested fires, CanExecute fires as well, long after the command source is gone.

To solve this I implemented IDisposable on RelayCommand, and added the necessary code so that whenever a model object is removed, and so is removed from the UI, Dispose() is invoked on all its RelayCommand.

This is the modified RelayCommand (the original is here):

public class RelayCommand : ICommand, IDisposable
{
    #region Fields

    List<EventHandler> _canExecuteSubscribers = new List<EventHandler>();
    readonly Action<object> _execute;
    readonly Predicate<object> _canExecute;

    #endregion // Fields

    #region Constructors

    public RelayCommand(Action<object> execute)
        : this(execute, null)
    {
    }

    public RelayCommand(Action<object> execute, Predicate<object> canExecute)
    {
        if (execute == null)
            throw new ArgumentNullException("execute");

        _execute = execute;
        _canExecute = canExecute;
    }

    #endregion // Constructors

    #region ICommand

    [DebuggerStepThrough]
    public bool CanExecute(object parameter)
    {
        return _canExecute == null ? true : _canExecute(parameter);
    }

    public event EventHandler CanExecuteChanged
    {
        add
        {
            CommandManager.RequerySuggested += value;
            _canExecuteSubscribers.Add(value);
        }
        remove
        {
            CommandManager.RequerySuggested -= value;
            _canExecuteSubscribers.Remove(value);
        }
    }

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

    #endregion // ICommand

    #region IDisposable

    public void Dispose()
    {
        _canExecuteSubscribers.ForEach(h => CanExecuteChanged -= h);
        _canExecuteSubscribers.Clear();
    }

    #endregion // IDisposable
}

Wherever I use the above, I track all instantiated RelayCommands so I can invoke Dispose() when the time comes:

Dictionary<string, RelayCommand> _relayCommands 
    = new Dictionary<string, RelayCommand>();

public ICommand SomeCmd
{
    get
    {
        RelayCommand command;
        string commandName = "SomeCmd";
        if (_relayCommands.TryGetValue(commandName, out command))
            return command;
        command = new RelayCommand(
            param => {},
            param => true);
        return _relayCommands[commandName] = command;
    }
}

void Dispose()
{
    foreach (string commandName in _relayCommands.Keys)
        _relayCommands[commandName].Dispose();
    _relayCommands.Clear();
}

Answer 2

There is a known problem with using lambda expression and events that you appear to be triggering. I'm hesitant to call it a "bug" because I don't understand the internal details enough to know if this is intended behavior, but it certainly seems counter-intuitive to me.

The key indication here is this part of your call stack:

PresentationCore.dll!System.Windows.Input.CommandManager.CallWeakReferenceHandlers(
   System.Collections.Generic.List handlers) + 0xac bytes 

"Weak" events are a way to hook up events that does not keep the target object alive; it's being used here because you are passing in a lamba expression as the event handler, so the "object" that contains the method is an internally-generated anonymous object. The problem is that the object being passed into the add handler for your event is not the same instance of an expression as the one being passed in to the remove event, it's just a functionally identical object, so it is not being unsubscribed from your event.

There are several workarounds, as described in the following questions:

Weak event handler model for use with lambdas

UnHooking Events with Lambdas in C#

Can using lambdas as event handlers cause a memory leak?

For your case the easiest is to move your CanExecute and Execute code into actual methods:

if (_act == null) {
  _act = new RelayCommand(this.DoCommand, this.CanDoCommand);
}

private void DoCommand(object parameter)
{
}

private bool CanDoCommand(object parameter)
{
    if (Removed)
      Console.WriteLine("Why is this happening?");
    return true;
}

Alternatively, if you can arrange for your object to construct Action<> and Func<> delegates from lambdas once, store them in variables, and use those when creating your RelayCommand, it will force the same instance to be used. IMO, for your case that's probably more complex than it needs to be.