dotnet / wpf

WPF is a .NET Core UI framework for building Windows desktop applications.
MIT License
7.08k stars 1.17k forks source link

Memory Leak when Effect assigned to UIElement not been frozen #6782

Open marbel82 opened 2 years ago

marbel82 commented 2 years ago

I found that the Window object is is held by the Border, and the Border is held by an EventHandler, and the EventHandler is held by DropShadowEffect. EventHandler has reference to EffectChanged method.

I think I have found the problem. When an UIElement.Effect property is assigned, the setter (Visual.VisualEffectInternal.set) adds strong event handler for EffectChanged method

https://referencesource.microsoft.com/#PresentationCore/Core/CSharp/System/Windows/Media/Visual.cs,2976 From ILSpy

// System.Windows.Media.Visual
using System.Windows.Media.Effects;

internal Effect VisualEffectInternal
{
    get
    {
        if (NodeHasLegacyBitmapEffect)
        {
            return null;
        }
        return EffectField.GetValue(this);
    }
    set
    {
        Effect value2 = EffectField.GetValue(this);
        if (value2 == value)
        {
            return;
        }
        if (value != null && !value.IsFrozen)
        {
            value.Changed += EffectChangedHandler;  // <-----
        }
        if (value2 != null)
        {
            if (!value2.IsFrozen)
            {
                value2.Changed -= EffectChangedHandler;  // <-----
            }
            DisconnectAttachedResource(VisualProxyFlags.IsEffectDirty, value2);
        }
        SetFlags(value != null, VisualFlags.NodeHasEffect);
        EffectField.SetValue(this, value);
        SetFlagsOnAllChannels(value: true, VisualProxyFlags.IsEffectDirty);
        EffectChanged(null, null);
    }
}

Event handler is removed only if an Effect property is changed. (Am I wrong?)

Analyzing this code, you can see that the event handler has not been added when the Effect object is frozen (IsFrozen=true).

I found a page, on which was written how to switch IsFrozen in XAML.

https://docs.microsoft.com/en-us/dotnet/desktop/wpf/advanced/freezable-objects-overview?redirectedfrom=MSDN&view=netframeworkdesktop-4.8#freezing-from-markup

    xmlns:PresentationOptions="http://schemas.microsoft.com/winfx/2006/xaml/presentation/options" 
    PresentationOptions:Freeze="True" 

But, it's not working.

One solution to the memory leak I have found is to define each DropShadowEffect localy in a Border.

Actual behavior: Effect setter (System.Windows.Media.Visual.VisualEffectInternal.set) use strong reference for event handler.

Expected behavior: Effect setter (System.Windows.Media.Visual.VisualEffectInternal.set) should use weak reference for event handler (e.g. WeakEventManager)

Minimal repro: https://github.com/marbel82/UIElementEffectMemoryLeakRepro

Click [Create LeakWindow], close the Window, click GC.Collect, Pause Visual Studio, Memory Usage/Take Snapshot and find LeakWindow

image

lindexi commented 2 years ago

@marbel82 Does it still refer to the Effect after you release LeakWindow?

marbel82 commented 2 years ago

The Effect (DropShadowEffect) is in App.Resources. The border adds an event handler, so an event handler keeps a reference to the border.

I don't keet a reference to the LeakWindow, I just create it, invoke Show() and than close it.

When I added a finalizer to LeakWindow and VolatileWindow you can see that only VolatileWindow finalizer is invoked.

I update my example to run without debugging. This you can see in the Release build configuration:

image