Skip to content

[WiP] Fix leak on Command Element #26044

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

Closed
wants to merge 5 commits into from

Conversation

pictos
Copy link
Contributor

@pictos pictos commented Nov 21, 2024

Description of Change

This PRs make sure that controls that implements ICommandElement unsubscribe from Command.CanExecuteChanged event avoid the control to leak if the Command lives longer than it.

Issues Fixed

Fixes #16124

@pictos pictos requested a review from a team as a code owner November 21, 2024 23:47
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Nov 21, 2024
@MartyIX MartyIX added platform/macos macOS / Mac Catalyst platform/windows platform/android platform/ios perf/memory-leak 💦 Memory usage grows / objects live forever (sub: perf) labels Nov 22, 2024
@jsuarezruiz
Copy link
Contributor

jsuarezruiz commented Nov 22, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Contributor

@jsuarezruiz jsuarezruiz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The build is failing because there are some unit tests not passing:

Failed ReenabledAfterCommandRemoved [< 1 ms]
Error Message:
Assert.True() Failure
Expected: True
Actual:   False


Failed RemovedCommandEnablesRefreshView [< 1 ms]
Error Message:
Assert.True() Failure
Expected: True
Actual:   False

Could you review it? Let me know if can help with something.

@jfversluis jfversluis added the area-xaml XAML, CSS, Triggers, Behaviors label Dec 11, 2024
@pictos pictos force-pushed the pj/Icommand-element-leak branch from 8c68bb0 to ecb9dd6 Compare December 14, 2024 05:17
Comment on lines +49 to +50
ve.Unloaded -= OnUnloaded;
ve.Unloaded += OnUnloaded;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the idea behind this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

safe code to avoid the same control to subscribe the same event more than once. But this will be reworked, this approach will not be used

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@pictos pictos changed the title Fix leak on Command Element [WiP] Fix leak on Command Element Dec 14, 2024
@pictos
Copy link
Contributor Author

pictos commented Jun 5, 2025

Closing in favor of #29837 29837

@pictos pictos closed this Jun 5, 2025
@pictos pictos deleted the pj/Icommand-element-leak branch June 5, 2025 20:26
@github-actions github-actions bot locked and limited conversation to collaborators Jul 6, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-xaml XAML, CSS, Triggers, Behaviors community ✨ Community Contribution perf/memory-leak 💦 Memory usage grows / objects live forever (sub: perf) platform/android platform/ios platform/macos macOS / Mac Catalyst platform/windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RefreshView can have a memory leak with a custom ICommand
4 participants