Friday, April 3, 2015

Memory leak caused by RegisterClassHandler of EventManager in WPF

Recently I spent some time to fix performance and memory usage issues in one big WPF project.  One critical bug reported was that every time users open an editing dialog, the memory will jump up around 5M, even users immediately close the dialog without any meaningful operations.  The memory will continue growing and never gets released.

Investigation

First I spent some time to understand the related code.  The editor view has an embedded child view EditorContent, and the child view too has 2 grandchildren views embedded.  And every view has a ViewModel class paired.  My strategy was to temporarily remove some child classes and find which View or ViewModel brings the problem.


This time I used Telerik's JustTrace to do the memory profiling.  After several rounds of trying, finally I located the problem in the child view EditorContent.  After carefully comparing the 2 snapshots before and after opening the editor dialog, I noticed there were some suspected events and controls along the GC root.  The control is called BarcodeTextBox and derived from the standard TextBox.

At the same time, I searched around the Internet and tried to find some clues. These 2 articles from Jeremy Alles and Devarchive.net have good hints.  When I went back to check the BarcodeTextBox class, I did find EventManager.RegisterClassHandler() in the code and it did exactly what was mentioned in the above 2 articles.


            EventManager.RegisterClassHandler (typeof(BarcodeTextBox), PreviewKeyDownEvent,
                new KeyEventHandler(HandlePreviewKeyDown));


So the fix is easy.  Either move the above code to a static constructor, or declare the event handler HandlePreviewKeyDown() as a static handler.  After this was done, the memory was released just after the dialog was closed.


Afterthought

From MSDN, I haven't found that it's mandatory to use RegisterClassHandler() in a static constructor or declare the handler as static.  But the MSDN example does use in that way.  And from the name itself, the handler should be at class level, not at instance level.

Checking EventManager from ILSpy, we can see
RegisterClassHandler() calls GlobalEventManager to add the handler to a global event handler array.  By the name, we can guess the handler (therefore the owner object) will be held forever if no explicit unregister method is called.  Unfortunately, the static EventManager class even doesn't have any methods to unregister an event handler.  So I guess by design, these handlers registered in this way should be like static functions, and shouldn't exist at the instance level.

From what I investigated, event is really dangerous in C# world.  Every time we use "+=" to add an event handler or register one, we should always try to find a way to unregister.  For example, I found this one.  The scenario is very tricky and could happen very likely.