Repository navigation
Fix Button and MenuItem leak when a style sets Command - #22367
Merged
MrJul merged 4 commits intoOct 5, 2026
Merged
Conversation
…Command The existing tests set Command as a local value. A Command that a style setter supplies takes a different path: the style applies the value during attach, and removes the value during detach, before the control overrides run. For Button and MenuItem the new tests fail. Each control subscribes two times on attach, and both subscriptions remain after the control leaves the logical tree, so the command keeps the control alive. The SplitButton test passes. SplitButton already handles this sequence, and the test guards that behaviour.
…taches When a style setter supplies Command, Button and MenuItem subscribe to CanExecuteChanged two times on attach and do not unsubscribe on detach. The command then holds the control, and every control behind it, for the lifetime of the command. Both controls decide whether to move the subscription from ILogical.IsAttachedToLogicalTree. StyledElement changes that state before the styles change and before the attach and detach overrides run: - On attach, the style sets Command while the control already reports attached. The property handler subscribes, then OnAttachedToLogicalTree subscribes again. - On detach, the style value goes away while the control already reports detached. The property handler skips the unsubscribe, then OnDetachedFromLogicalTree finds no Command to unsubscribe from. Track the subscription state in the control instead. Set the flag where OnAttachedToLogicalTree subscribes and clear the flag where OnDetachedFromLogicalTree unsubscribes. The Command property handler then moves the subscription only while the control owns one. SplitButton already uses this approach. Fixes AvaloniaUI#20588 Fixes AvaloniaUI#19619
NathanDrake2406
marked this pull request as ready for review
October 2, 2026 02:06
NuGet packagesThe packages for this pull request have been published (workflow run):
|
MrJul
added
bug
backport-candidate-12.1.x
maxkatz6
reviewed
Oct 4, 2026
The field name states its purpose. The comment only gave the history of the change.
maxkatz6
approved these changes
Oct 4, 2026
grokys
pushed a commit
to Evan260/Avalonia
that referenced
this pull request
Oct 7, 2026
) * test(controls): cover CanExecuteChanged subscription for a style-set Command The existing tests set Command as a local value. A Command that a style setter supplies takes a different path: the style applies the value during attach, and removes the value during detach, before the control overrides run. For Button and MenuItem the new tests fail. Each control subscribes two times on attach, and both subscriptions remain after the control leaves the logical tree, so the command keeps the control alive. The SplitButton test passes. SplitButton already handles this sequence, and the test guards that behaviour. * fix(controls): release a style-set Command when Button or MenuItem detaches When a style setter supplies Command, Button and MenuItem subscribe to CanExecuteChanged two times on attach and do not unsubscribe on detach. The command then holds the control, and every control behind it, for the lifetime of the command. Both controls decide whether to move the subscription from ILogical.IsAttachedToLogicalTree. StyledElement changes that state before the styles change and before the attach and detach overrides run: - On attach, the style sets Command while the control already reports attached. The property handler subscribes, then OnAttachedToLogicalTree subscribes again. - On detach, the style value goes away while the control already reports detached. The property handler skips the unsubscribe, then OnDetachedFromLogicalTree finds no Command to unsubscribe from. Track the subscription state in the control instead. Set the flag where OnAttachedToLogicalTree subscribes and clear the flag where OnDetachedFromLogicalTree unsubscribes. The Command property handler then moves the subscription only while the control owns one. SplitButton already uses this approach. Fixes AvaloniaUI#20588 Fixes AvaloniaUI#19619 * chore: trim code comments * chore: remove the comment on _isSubscribedToCommand The field name states its purpose. The comment only gave the history of the change.
MrJul
pushed a commit
that referenced
this pull request
Oct 9, 2026
* test(controls): cover CanExecuteChanged subscription for a style-set Command The existing tests set Command as a local value. A Command that a style setter supplies takes a different path: the style applies the value during attach, and removes the value during detach, before the control overrides run. For Button and MenuItem the new tests fail. Each control subscribes two times on attach, and both subscriptions remain after the control leaves the logical tree, so the command keeps the control alive. The SplitButton test passes. SplitButton already handles this sequence, and the test guards that behaviour. * fix(controls): release a style-set Command when Button or MenuItem detaches When a style setter supplies Command, Button and MenuItem subscribe to CanExecuteChanged two times on attach and do not unsubscribe on detach. The command then holds the control, and every control behind it, for the lifetime of the command. Both controls decide whether to move the subscription from ILogical.IsAttachedToLogicalTree. StyledElement changes that state before the styles change and before the attach and detach overrides run: - On attach, the style sets Command while the control already reports attached. The property handler subscribes, then OnAttachedToLogicalTree subscribes again. - On detach, the style value goes away while the control already reports detached. The property handler skips the unsubscribe, then OnDetachedFromLogicalTree finds no Command to unsubscribe from. Track the subscription state in the control instead. Set the flag where OnAttachedToLogicalTree subscribes and clear the flag where OnDetachedFromLogicalTree unsubscribes. The Command property handler then moves the subscription only while the control owns one. SplitButton already uses this approach. Fixes #20588 Fixes #19619 * chore: trim code comments * chore: remove the comment on _isSubscribedToCommand The field name states its purpose. The comment only gave the history of the change.
MrJul
added
backported-12.1.x
and removed
backport-candidate-12.1.x
3 of 4 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does the pull request do?
Stops
ButtonandMenuItemfrom leaking when a style setter suppliesCommand. Each control now subscribes toICommand.CanExecuteChangedone time on attach and unsubscribes on detach, for a style-setCommandas well as for a local value or binding.What is the current behavior?
When a style sets
Command,ButtonandMenuItemsubscribe toCanExecuteChangedtwo times on attach and never unsubscribe. The command keeps a strong reference to the control. With a long-lived command, such as a view model command behind a recycledListBoxitem template, every detached control stays in memory.Both controls use
ILogical.IsAttachedToLogicalTreeto decide whether theCommandproperty handler moves the subscription.StyledElementchanges that state before it changes the styles, and before it calls the attach and detach overrides:IsAttachedToLogicalTreeApplyStylingsetsCommandtrueOnAttachedToLogicalTreetrueInvalidateStylesclearsCommandfalseOnDetachedFromLogicalTreefalseCommandisnull, nothing to unsubscribeWhat is the updated/expected behavior with this PR?
Command, control attachedCommand, control detachedCommandThe new tests count the
CanExecuteChangedsubscribers of a command directly, so they do not depend on the garbage collector:Button_Subscribes_Once_To_Style_Set_Command_CanExecuteChanged_When_Added_To_Logical_TreeButton_Unsubscribes_From_Style_Set_Command_CanExecuteChanged_When_Removed_From_Logical_TreeMenuItemSplitButton_Subscribes_Once_To_Style_Set_Command_CanExecuteChanged_While_In_Logical_TreeThe first commit contains only the tests. At that commit the four
ButtonandMenuItemtests fail with a count of 2. TheSplitButtontest passes at that commit, becauseSplitButtonis not affected. It is a guard, not a reproduction.Validation
Run from
tests/Avalonia.Controls.UnitTests/bin/Debug/net10.0withdotnet exec ./Avalonia.Controls.UnitTests.dll:--filter-method "*Style_Set_Command*"--filter-class "*ButtonTests*"--filter-class "*MenuItem*"Avalonia.Controlsalso builds in Release with no warnings. I did not run the full test suite or the leak tests locally.How was the solution implemented (if it's not obvious)?
The control owns the subscription, so the control tracks it. A private flag is set where
OnAttachedToLogicalTreesubscribes and cleared whereOnDetachedFromLogicalTreeunsubscribes. TheCommandproperty handler moves the subscription only while that flag is set.This is the approach
SplitButtonalready uses with its_isAttachedToLogicalTreefield, which is whySplitButtondoes not have the bug.Alternatives that this PR does not take:
StyledElement.OnDetachedFromLogicalTreeCore, as proposed in Memory Leak in MenuItem due to incorrect Command.CanExecuteChanged subscription/unsubscription #19619. That changes the order of style invalidation and detach notification for every control, and it does not remove the double subscription on attach.WeakEvents.CommandCanExecuteChanged, asNativeMenuItemdoes. That hides the unbalanced subscription instead of correcting it, and it changes more behavior than this fix needs.Button.CanExecuteChangedstill usesIsAttachedToLogicalTreeto decide whether to evaluateCanExecute. That check does not affect the subscription and is unchanged.Checklist
Breaking changes
None. The change adds a private field to each control and changes no public API.
Obsoletions / Deprecations
None.
Fixed issues
Fixes #20588
Fixes #19619