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

Memoryleak VisualstateManager in ButtonStyle on Android #21389

Closed
Larhei opened this issue Mar 22, 2024 · 11 comments · Fixed by #21484
Closed

Memoryleak VisualstateManager in ButtonStyle on Android #21389

Larhei opened this issue Mar 22, 2024 · 11 comments · Fixed by #21484
Assignees
Labels
area-xaml XAML, CSS, Triggers, Behaviors delighter-sc fixed-in-8.0.20 fixed-in-9.0.0-preview.4.10690 platform/android 🤖 s/needs-attention Issue has more information and needs another look t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Milestone

Comments

@Larhei
Copy link

Larhei commented Mar 22, 2024

Description

When creating an explicite Button Style in Styles.xaml like this.

<Style x:Key="MyButtonStyle" TargetType="Button"> <Setter Property="VisualStateManager.VisualStateGroups"> <VisualStateGroupList> <VisualStateGroup x:Name="CommonStates"> <VisualState x:Name="Normal" /> <VisualState x:Name="Pressed"> </VisualState> <VisualState x:Name="Disabled"> </VisualState> </VisualStateGroup> </VisualStateGroupList> </Setter> </Style>

And referencing it on any ContentenPage like this
<Button x:Name="PART_MyButton" Style="{StaticResource MyButtonStyle}" Text="Welcome to .NET MAUI!" VerticalOptions="Center" HorizontalOptions="Center" />

The ContentPage never get garbage collected on Android.

Steps to Reproduce

  1. Checkout the code
  2. Start the App on Android
  3. Navigate to Leak by pressing Button "Leak"
  4. Navigate back
  5. Hit GC Button multiple Time to force GC

Link to public reproduction project repository

https://github.com/Larhei/Maui-Issues/tree/main/VisualStateLeak

Version with bug

8.0.10 SR3

Is this a regression from previous behavior?

Not sure, did not test other versions

Last version that worked well

Unknown/Other

Affected platforms

Android

Affected platform versions

No response

Did you find any workaround?

Moving the Style to ContentPage.Resources will stop the leak, but with will force me to copy my styles in multiple locations.

Relevant log output

No response

@Larhei Larhei added the t/bug Something isn't working label Mar 22, 2024
@MitchBomcanhao
Copy link

speaking of workarounds, just wondering: what if instead of using the style directly on each page, you import a smaller resource dictionary which includes the necessary button style into the page? if that works (ie it doesn't still trigger a memory leak), it'd get rid of multiple copies of the style.

@Larhei
Copy link
Author

Larhei commented Mar 22, 2024

@MitchBomcanhao
I also tested this... Not working :-(
Even when doing code behind Code that is clearing resources after navigating away and clearing Visual States Group on the Button instance

@PureWeen
Copy link
Member

@jonathanpeppers @StephaneDelcroix thoughts?

@PureWeen PureWeen added this to the Backlog milestone Mar 22, 2024
@StephaneDelcroix StephaneDelcroix self-assigned this Mar 25, 2024
@StephaneDelcroix
Copy link
Contributor

I'll check if this is Android specific or a general issue. VSM or Style might be holding a stronger-than-necessary ref to the Label...

@jonathanpeppers
Copy link
Member

This app seems like it is working for me on both Windows & Android:

[0:] NotLeakingPage finalized.
[0:] LeakingPage finalized.

@Larhei did you disable XAML hot reload when testing this? (otherwise it can keep things alive)

@jonathanpeppers jonathanpeppers added the s/needs-info Issue needs more info from the author label Mar 26, 2024
@Larhei
Copy link
Author

Larhei commented Mar 26, 2024 via email

@dotnet-policy-service dotnet-policy-service bot added s/needs-attention Issue has more information and needs another look and removed s/needs-info Issue needs more info from the author labels Mar 26, 2024
@jonathanpeppers jonathanpeppers added s/needs-info Issue needs more info from the author and removed s/needs-attention Issue has more information and needs another look labels Mar 26, 2024
@Larhei
Copy link
Author

Larhei commented Mar 26, 2024

@jonathanpeppers

Did a Test in release with dotnet gc-dump. Maybe I am reading this wrong... but I still can see LeakingPage in this dump... And this should not be the case from my understanding. Dump

@dotnet-policy-service dotnet-policy-service bot added s/needs-attention Issue has more information and needs another look and removed s/needs-info Issue needs more info from the author labels Mar 26, 2024
@jonathanpeppers
Copy link
Member

Do you see the log messages?

[0:] NotLeakingPage finalized.
[0:] LeakingPage finalized.

If you push/pop the pages a few times and click GC multiple times? I was just tapping quicly and noticed those messages printed.

The file just shows 1 instance:

image

It should grow to several instances if there is an issue.

@Larhei
Copy link
Author

Larhei commented Mar 27, 2024

Hi @jonathanpeppers, how comes this instance is holding multiple button instances? See latest dump file in the repro? Or how comes no instance of the not leaking page is showing even when navigated to that page? I am confused

@StephaneDelcroix
Copy link
Contributor

StephaneDelcroix commented Mar 27, 2024

<ContentPage xmlns="http://schemas.microsoft.com/dotnet/2021/maui"
                    xmlns:x="http://schemas.microsoft.com/winfx/2009/xaml"
                    x:Class="Microsoft.Maui.Controls.Xaml.UnitTests.Maui21839">
    <Button x:Name="button" Text="Click me" Style="{StaticResource buttonStyle}"/>
</ContentPage>
            Application.Current.Resources.Add("buttonStyle", 
                new Style(typeof(Button)) {
                    Setters = {
                        new Setter { Property = VisualStateManager.VisualStateGroupsProperty, Value = new VisualStateGroupList{
                            new VisualStateGroup {
                                Name = "CommonStates",
                                States = {
                                    new VisualState { Name = "Normal" },
                                    new VisualState { Name = "Pressed" },
                                    new VisualState { Name = "Disabled" }
                                }
                            }
                        } }
                    }
                });
            var pagewr = new WeakReference(new Maui21839(useCompiledXaml));
            await Task.Delay(10);
            GC.Collect();
            Assert.IsNull(pagewr.Target, "Page leaked");

I can confirm that it leaks

StephaneDelcroix added a commit that referenced this issue Mar 27, 2024
@Larhei
Copy link
Author

Larhei commented Mar 28, 2024

@jonathanpeppers and @StephaneDelcroix
Thanks for taking you time and investigating this.

StephaneDelcroix added a commit that referenced this issue Mar 29, 2024
* [C] fix a leak in VSG

- fixes #21389

* simplify
@github-actions github-actions bot locked and limited conversation to collaborators May 1, 2024
@Eilon Eilon added t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.) and removed legacy-area-perf Startup / Runtime performance labels May 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-xaml XAML, CSS, Triggers, Behaviors delighter-sc fixed-in-8.0.20 fixed-in-9.0.0-preview.4.10690 platform/android 🤖 s/needs-attention Issue has more information and needs another look t/bug Something isn't working t/perf The issue affects performance (runtime speed, memory usage, startup time, etc.)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants