Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[controls] fix memory leak in VisualElement.Background #13656

Merged
merged 3 commits into from
Mar 8, 2023

Commits on Mar 2, 2023

  1. [controls] fix memory leak in VisualElement.Background

    Fixes: dotnet#12344
    Fixes: dotnet#13557
    Context: https://github.com/dotnet-presentations/dotnet-maui-workshop
    
    While testing the `Monkey Finder` sample, I found the following
    scenario causes an issue:
    
    1. Declare a `{StaticResource}` `Brush` at the `Application` level,
       with a lifetime of the entire application.
    
    2. Set `Background` on an item in a `CollectionView`, `ListView`, etc.
    
    3. Scroll a lot, navigate away, etc.
    
    4. The `Brush` will hold onto any `View`'s indefinitely.
    
    The core problem here being `VisualElement` does:
    
        void NotifyBackgroundChanges()
        {
            if (Background is ImmutableBrush)
                return;
    
            if (Background != null)
            {
                Background.Parent = this;
                Background.PropertyChanged += OnBackgroundChanged;
    
                if (Background is GradientBrush gradientBrush)
                    gradientBrush.InvalidateGradientBrushRequested += InvalidateGradientBrushRequested;
            }
        }
    
    If no user code sets `Background` to `null`, these events remain
    subscribed.
    
    To fix this:
    
    1. Create a `WeakNotifyCollectionChangedProxy` type for event subscription.
    
    2. Don't set `Background.Parent = this`
    
    ~~ General Cleanup ~~
    
    Through doing other fixes related to memory leaks & C# events, we've
    started to gain a collection of `WeakEventProxy`-related types.
    
    I created some core `internal` types to be reused:
    
    * `abstract WeakEventProxy<TSource, TEventHandler>`
    * `WeakNotifyCollectionChangedProxy`
    
    The following classes now make use of these new shared types:
    
    * `BindingExpression`
    * `BindableLayout`
    * `ListProxy`
    * `VisualElement`
    
    This should hopefully reduce mistakes and reuse code in this area.
    
    ~~ Concerns ~~
    
    Since, we are no longer doing:
    
        Background.Parent = this;
    
    This is certainly a behavior change. It is now replaced with:
    
        SetInheritedBindingContext(Background, BindingContext);
    
    I had to update one unit test that was asserting `Background.Parent`,
    which can assert `Background.BindingContext` instead.
    
    As such, this change is definitely sketchy and I wouldn't backport to
    .NET 7 immediately. We might test it out in a .NET 8 preview first.
    jonathanpeppers committed Mar 2, 2023
    Configuration menu
    Copy the full SHA
    a66507c View commit details
    Browse the repository at this point in the history
  2. Auto-format source code

    GitHub Actions Autoformatter committed Mar 2, 2023
    Configuration menu
    Copy the full SHA
    d87eb34 View commit details
    Browse the repository at this point in the history

Commits on Mar 6, 2023

  1. Configuration menu
    Copy the full SHA
    9bc7d0f View commit details
    Browse the repository at this point in the history