-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Conversation
Assert.Same(parent, parent.Background.Parent); | ||
Assert.Same(context, parent.Background.BindingContext); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a behavior change that we don't set Background.Parent
anymore.
However, Background.BindingContext
is set, so you should be able to use {Binding}
such as:
<Label>
<Label.Background>
<SolidColorBrush Color="{Binding Foo}" />
</Label.Background>
</Label>
And the Label
's BindingContext
should pass on to the SolidColorBrush
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this was always interesting as a single brush can be used for multiple views. Is there anything that needs access to the brush/stroke's parent? If there is, then this is really interesting as who wins? Last one? First one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am also wondering about that binding context... If I set one brush on 2 views, is it just going to overwrite the BC to the last one? Maybe that is just how it works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it seems like that is how it always worked...
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.
82d4c29
to
a66507c
Compare
Thank you for your pull request. We are auto-formating your source code to follow our code guidelines. |
/// <summary>Specifies that an output may be null even if the corresponding type disallows it.</summary> | ||
[AttributeUsage(AttributeTargets.Field | AttributeTargets.Parameter | AttributeTargets.Property | AttributeTargets.ReturnValue, Inherited = false)] | ||
internal sealed class MaybeNullAttribute : Attribute { } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got this from:
https://source.dot.net/#Microsoft.Build.Framework/NullableAttributes.cs,68093cc4b5713519
So we can use these NRT annotations in a netstandard2.0
library.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We did bad things in essentials:
#if !NETSTANDARD
[return: NotNullIfNotNull("defaultValue")]
#endif
One day this can all go away and we use net6/7 everywhere.
Looks good to me, but maybe a third @PureWeen can hit the merge if there are no weirdos in here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just spotted something, not sure if I am missing it or it is a thing...
As seen in dotnet#13973, some of my recent changes had a flaw: * dotnet#13550 * dotnet#13806 * dotnet#13656 Because nothing held onto the `EventHandler` in some of these cases, at some point a GC will prevent future events from firing. So for example, my original attempt to test this behavior: [Fact] public async Task RectangleGeometrySubscribed() { var geometry = new RectangleGeometry(); var visual = new VisualElement { Clip = geometry }; bool fired = false; visual.PropertyChanged += (sender, e) => { if (e.PropertyName == nameof(VisualElement.Clip)) fired = true; }; // Was missing these three lines!!! // await Task.Yield(); // GC.Collect(); // GC.WaitForPendingFinalizers(); geometry.Rect = new Rect(1, 2, 3, 4); Assert.True(fired, "PropertyChanged did not fire!"); } In each case, I added an additional test showing the problem. I played around with some ideas, but the simplest solution is to save the `EventHandler` in a member field of the subscriber. Will keep thinking of smarter ways to handle this. I also fixed several GC-related tests that were ignored, hoping they might help find issues in this area. My `await Task.Yield()` trick was enough to make them pass.
As seen in #13973, some of my recent changes had a flaw: * #13550 * #13806 * #13656 Because nothing held onto the `EventHandler` in some of these cases, at some point a GC will prevent future events from firing. So for example, my original attempt to test this behavior: [Fact] public async Task RectangleGeometrySubscribed() { var geometry = new RectangleGeometry(); var visual = new VisualElement { Clip = geometry }; bool fired = false; visual.PropertyChanged += (sender, e) => { if (e.PropertyName == nameof(VisualElement.Clip)) fired = true; }; // Was missing these three lines!!! // await Task.Yield(); // GC.Collect(); // GC.WaitForPendingFinalizers(); geometry.Rect = new Rect(1, 2, 3, 4); Assert.True(fired, "PropertyChanged did not fire!"); } In each case, I added an additional test showing the problem. I played around with some ideas, but the simplest solution is to save the `EventHandler` in a member field of the subscriber. Will keep thinking of smarter ways to handle this. I also fixed several GC-related tests that were ignored, hoping they might help find issues in this area. My `await Task.Yield()` trick was enough to make them pass. * Fix tests in Release mode In `Release` mode, a `GC.KeepAlive()` call is needed for the tests to pass. Co-authored-by: GitHub Actions Autoformatter <[email protected]>
* Removed BuildTizenDefaultTemplate and just have it call RadioButton's default template since they were identical. (#13996) * Reinstate WebView cookie functionality for Android & iOS (#13736) * Fix iOS cookies * Fix Android Cookies * Update src/Core/src/Platform/iOS/MauiWKWebView.cs Co-authored-by: Manuel de la Pena <[email protected]> * Auto-format source code * Update MauiWKWebView.cs * Update src/Core/src/Platform/iOS/MauiWKWebView.cs --------- Co-authored-by: Manuel de la Pena <[email protected]> Co-authored-by: GitHub Actions Autoformatter <[email protected]> * Revert 10759. Fix Button sizing using HorizontalOptions. (#14005) * Ensure that Grid is treating star rows/columns as Auto when unconstrained (#13999) * Ensure that Grid is treating star rows/columns as Auto when unconstrained Fixes #13993 * Auto-format source code --------- Co-authored-by: GitHub Actions Autoformatter <[email protected]> * [iOS] Implement ScrollView Orientation (#13657) * [iOS] Remove not used mapper for ContentSize * [iOS] Implement Orientation mapping * [Samples] Add sample page for ScrollView orientation * Try without this * [iOS] Move from extension to helper * Add back removed API * Use SetNeedsLayout to call measure of ContentView * Cleanup * [Android] Fix Frame Renderer to use Wrapper View correctly (#12218) * [Android] Fix Frame to call missing mapper methods * - fix rebase * Auto-format source code * - update tests and wrapper view code * - remove code that's now generalized in ViewHandler * - cleanup frame renderer --------- Co-authored-by: GitHub Actions Autoformatter <[email protected]> * [controls] fix cases a GC causes events to not fire (#13997) As seen in #13973, some of my recent changes had a flaw: * #13550 * #13806 * #13656 Because nothing held onto the `EventHandler` in some of these cases, at some point a GC will prevent future events from firing. So for example, my original attempt to test this behavior: [Fact] public async Task RectangleGeometrySubscribed() { var geometry = new RectangleGeometry(); var visual = new VisualElement { Clip = geometry }; bool fired = false; visual.PropertyChanged += (sender, e) => { if (e.PropertyName == nameof(VisualElement.Clip)) fired = true; }; // Was missing these three lines!!! // await Task.Yield(); // GC.Collect(); // GC.WaitForPendingFinalizers(); geometry.Rect = new Rect(1, 2, 3, 4); Assert.True(fired, "PropertyChanged did not fire!"); } In each case, I added an additional test showing the problem. I played around with some ideas, but the simplest solution is to save the `EventHandler` in a member field of the subscriber. Will keep thinking of smarter ways to handle this. I also fixed several GC-related tests that were ignored, hoping they might help find issues in this area. My `await Task.Yield()` trick was enough to make them pass. * Fix tests in Release mode In `Release` mode, a `GC.KeepAlive()` call is needed for the tests to pass. Co-authored-by: GitHub Actions Autoformatter <[email protected]> * [iOS] Scroll with the keyboard to not block entries and editors (#13499) --------- Co-authored-by: dustin-wojciechowski <[email protected]> Co-authored-by: Gerald Versluis <[email protected]> Co-authored-by: Manuel de la Pena <[email protected]> Co-authored-by: GitHub Actions Autoformatter <[email protected]> Co-authored-by: Javier Suárez <[email protected]> Co-authored-by: E.Z. Hart <[email protected]> Co-authored-by: Rui Marinho <[email protected]> Co-authored-by: Shane Neuville <[email protected]> Co-authored-by: Jonathan Peppers <[email protected]> Co-authored-by: TJ Lambert <[email protected]>
Is it possible that this can get backported to NET 7.0? |
Fixes #12344
Fixes #13557
Context: https://github.com/dotnet-presentations/dotnet-maui-workshop
While testing the
Monkey Finder
sample, I found the following scenario causes an issue:Declare a
{StaticResource}
Brush
at theApplication
level, with a lifetime of the entire application.Set
Background
on an item in aCollectionView
,ListView
, etc.Scroll a lot, navigate away, etc.
The
Brush
will hold onto anyView
's indefinitely.The core problem here being
VisualElement
does:If no user code sets
Background
tonull
, these events remain subscribed.To fix this:
Create a
WeakNotifyCollectionChangedProxy
type for event subscription.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:
This is certainly a behavior change. It is now replaced with:
I had to update one unit test that was asserting
Background.Parent
, which can assertBackground.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.