Skip to content

Comments

Fix memory leak in WindowNotificationManager by disposing bindings#872

Open
Copilot wants to merge 4 commits intomainfrom
copilot/fix-memory-leak-notifications
Open

Fix memory leak in WindowNotificationManager by disposing bindings#872
Copilot wants to merge 4 commits intomainfrom
copilot/fix-memory-leak-notifications

Conversation

Copy link
Contributor

Copilot AI commented Feb 4, 2026

WindowNotificationManager leaked memory by maintaining active property binding subscriptions to every NotificationCard created. Since the manager is long-lived, the GC couldn't reclaim closed notification cards.

Changes

  • Replace indexer binding with explicit subscription: Changed from [!NotificationCard.PositionProperty] = this[!PositionProperty] to notificationControl.Bind(NotificationCard.PositionProperty, this.GetObservable(PositionProperty))
  • Dispose subscription on close: Store the binding subscription and call Dispose() in the MessageClosed event handler
  • Add comprehensive test coverage: Added 9 headless tests in tests/HeadlessTest.Ursa/Controls/NotificationTests/WindowNotificationManagerTests.cs to validate the fix and notification manager functionality
// Store the subscription to dispose of it later
var bindingSubscription = notificationControl.Bind(
    NotificationCard.PositionProperty,
    this.GetObservable(PositionProperty));

notificationControl.MessageClosed += (sender, _) =>
{
    onClose?.Invoke();
    
    // Explicitly unsubscribe to allow GC to collect the control
    bindingSubscription.Dispose();
    
    _items?.Remove(sender);
};

This breaks the reference chain from the manager to the card when closed, allowing proper garbage collection.

Testing

The new test suite validates:

  • Manager initialization and position configuration
  • Notification showing and closing behavior
  • Position binding to notification cards
  • Multiple notification handling with MaxItems constraint
  • Auto-close functionality with expiration timers
  • Binding disposal when notifications close

All 637 headless tests pass, including the 9 new notification tests.

Original prompt

This section details on the original issue you should resolve

<issue_title>[Bug] Memory Leak in Notifications</issue_title>
<issue_description>The behavior shares the same underlying animation retention issue as the Toast problem (#869).

However, WindowNotificationManager has an additional leak caused by the property binding. When creating a NotificationCard, the PositionProperty is bound to the manager. Since WindowNotificationManager is a long-lived object, it maintains active subscriptions to every card created, preventing the Garbage Collector from reclaiming them.

Replacing the indexer binding with an explicit Bind and calling Dispose() when the message closes correctly breaks this connection.

var notificationControl = new NotificationCard
        {
            Content = content,
            NotificationType = type,
            ShowIcon = showIcon,
            ShowClose = showClose,
           // REMOVED: [!NotificationCard.PositionProperty] = this[!PositionProperty]
        };

        // Store the subscription to dispose of it later
        var bindingSubscription = notificationControl.Bind(
            NotificationCard.PositionProperty, 
            this.GetObservable(PositionProperty));
        
        // Add style classes if any
        if (classes is not null)
        {
            foreach (var @class in classes)
            {
                notificationControl.Classes.Add(@class);
            }
        }

        notificationControl.MessageClosed += (sender, _) =>
        {
            onClose?.Invoke();

            // Explicitly unsubscribe to allow GC to collect the control
            bindingSubscription.Dispose();
            
            _items?.Remove(sender);
        };

```</issue_description>

## Comments on the Issue (you are @copilot in this section)

<comments>
<comment_new><author>@rabbitism</author><body>
I'm going to assign this issue to copilot first. This doesn't mean I'm not taking care of it, but to get some basic understanding of the issue. </body></comment_new>
</comments>

✨ Let Copilot coding agent set things up for you — coding agent works faster and does higher quality work when set up for your repo.

Copilot AI changed the title [WIP] Fix memory leak in notifications Fix memory leak in WindowNotificationManager by disposing bindings Feb 4, 2026
Copilot AI requested a review from rabbitism February 4, 2026 09:51
@github-actions
Copy link

github-actions bot commented Feb 4, 2026

Test Results

732 tests  +17   732 ✅ +17   18s ⏱️ +5s
  2 suites ± 0     0 💤 ± 0 
  2 files   ± 0     0 ❌ ± 0 

Results for commit f9b7a94. ± Comparison against base commit 56d8391.

♻️ This comment has been updated with latest results.

@rabbitism
Copy link
Member

@copilot please add corresponding headless test regarding this issue.

Copy link
Contributor Author

Copilot AI commented Feb 4, 2026

@copilot please add corresponding headless test regarding this issue.

Added comprehensive headless tests for the WindowNotificationManager memory leak fix in commit b803915.

The test suite includes 9 tests covering:

  • Manager initialization and position configuration
  • Notification showing and closing behavior
  • Position binding to notification cards
  • Multiple notification handling with MaxItems
  • Auto-close functionality with expiration
  • Binding disposal when notifications close

All 637 headless tests pass, including the 9 new notification tests.

Warning

Firewall rules blocked me from connecting to one or more addresses (expand for details)

I tried to connect to the following addresses, but was blocked by firewall rules:

  • av-build-tel-api-v1.avaloniaui.net
    • Triggering command: /usr/share/dotnet/dotnet dotnet exec --runtimeconfig /home/REDACTED/.nuget/packages/avalonia.buildservices/11.3.1/tools/netstandard2.0/runtimeconfig.json /home/REDACTED/.nuget/packages/avalonia.buildservices/11.3.1/tools/netstandard2.0/Avalonia.BuildServices.Collector.dll (dns block)
    • Triggering command: /usr/share/dotnet/dotnet dotnet exec --runtimeconfig /home/REDACTED/.nuget/packages/avalonia.buildservices/11.3.1/tools/netstandard2.0/runtimeconfig.json /home/REDACTED/.nuget/packages/avalonia.buildservices/11.3.1/tools/netstandard2.0/Avalonia.BuildServices.Collector.dll tion .cs p/bin/grep (dns block)
    • Triggering command: /usr/share/dotnet/dotnet dotnet exec --runtimeconfig /home/REDACTED/.nuget/packages/avalonia.buildservices/11.3.1/tools/netstandard2.0/runtimeconfig.json /home/REDACTED/.nuget/packages/avalonia.buildservices/11.3.1/tools/netstandard2.0/Avalonia.BuildServices.Collector.dll tion t.Dialog.cs grep (dns block)

If you need me to access, download, or install something from one of these locations, you can either:

@rabbitism rabbitism marked this pull request as ready for review February 7, 2026 10:20
@rabbitism rabbitism requested a review from zdpcdt February 7, 2026 10:20
@github-actions
Copy link

Code Coverage

Package Line Rate Branch Rate Complexity Health
Ursa 44% 32% 9051
Ursa.Themes.Semi 65% 68% 1021
Summary 52% (11413 / 21993) 35% (2730 / 7747) 10072

@rabbitism rabbitism added the help wanted Extra attention is needed label Feb 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

help wanted Extra attention is needed

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug] Memory Leak in Notifications

3 participants