6

Say I derive a WPF control eg TextBox and I override one of the On- methods eg OnInitialized

Suppose I did this: this.Initialized += delegate { };

If the window containing this control closed - would this cause a memory leak if nothing else is done?

If this does cause a memory leak, would implementing a Finalizer be a sufficient, minimum remedy?

blue18hutthutt
  • 3,191
  • 5
  • 34
  • 57

1 Answers1

5

The memory leak I believe you're thinking of is when you attach an event handler to an event on an object (call it "A") and then lose all references to the object ("B") that owns the handler. The garbage collector can still reach object "B" through object "A's" event and so will never collect it. See this question for some more discussion.

In your case, you're attaching a handler from inside the Form. When the Form is closed and you drop all your references to it, there is no way to get to the form from the rest of your code so the GC happily collects it (when it gets around to it). No leak will occur.

Based on your last comment, implementing a finalizer may not do what you think it does. It's just a hook the runtime gives you to perform some cleanup code, and you're better off implementing the IDisposable interface and pattern. One thing I tend to do in my classes that expose events is set the event to null in the Dispose method. I.e.:

class Foo : IDisposable
{
    public event EventHandler SomethingHappened;

    // ... normal IDisposable implementation details left out

    protected virtual void Dispose(bool Disposing)
    {
        if (Disposing)
        {
            SomethingHappened = null;
        }
    }
}

I don't do this in all of my classes, but it's a relatively clean way to remove all handlers from an object when it's going away. You can only do this from inside the class itself.

In any case, if you're going to do work in a Form based on the PreviewMouseLeftButtonDown event, you should instead override the OnPreviewMouseLeftButtonDown method. Attaching event handlers to listen to a class' own events is generally bad form. Do the following instead:

protected override void OnPreviewMouseLeftButtonDown(MouseButtonEventArgs e)
{
    // Actually raise the event to let other classes know it happened
    base.OnPreviewMouseLeftButtonDown(e);

    // your code...
}
Community
  • 1
  • 1
Patrick Quirk
  • 23,334
  • 2
  • 57
  • 88
  • Great thanks for the explanation - I just arbitrarily chose OnPreviewMouseLeftButtonDown for my example (didn't realize it was on my clipboard). What I meant by my last comment with the finalizer was unsubscribe in the Finalizer - but now you brought up an interesting question: is event = null the same as manually unsubscribing each handler? I cannot find a definitive answer to this but it is discussed here: http://stackoverflow.com/questions/8892579/does-assigning-null-remove-all-event-handlers-from-an-object – blue18hutthutt Nov 14 '12 at 03:45
  • I just tested it and it worked for me; the handler did not get called after I set an event to null. However that question raises a good point about what is actually happening when you set it to null. I'll need to research a bit more before I can say whether or not I mislead you and it might be bad practice. – Patrick Quirk Nov 14 '12 at 03:53
  • But thanks, your answer clarified this question perfectly - the memory leak only occurs when the *owner of the handler* is no longer directly referenced by anything other than the event! – blue18hutthutt Nov 14 '12 at 03:53
  • 1
    Jon Skeet does it, so it must be good ;) http://stackoverflow.com/questions/153573/how-can-i-clear-event-subscriptions-in-c – Patrick Quirk Nov 14 '12 at 03:55