Repository navigation
Fix memory leak in BindingDiagnostics.BindingFailed by backing the static event with WeakEventManager - #37645
Conversation
GitHub Actions
Inactive
|
🚀 Dogfood this PR with:
curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 37645Or
iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 37645" |
|
Hey there @@Shalini-Ashokan! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed. |
dotnet-policy-service
Bot
added
the
partner/syncfusion
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Inactive
GitHub Actions
Failure
GitHub Actions
Inactive
|
/azp run maui-pr-uitests , maui-pr-devicetests |
|
Azure Pipelines: Successfully started running 2 pipeline(s). |
|
Azure Pipelines: Successfully started running 1 pipeline(s). There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
This PR addresses a memory leak caused by BindingDiagnostics.BindingFailed being a static event that strongly retains instance subscribers that forget to unsubscribe, by backing the event with WeakEventManager so subscribers can be garbage-collected.
Changes:
- Replaced
BindingDiagnostics.BindingFailed’s static delegate-backed event with aWeakEventManager-backed event. - Updated
SendBindingFailure(...)to raise the event throughWeakEventManager. - Added unit tests intended to prevent regressions for the leak scenario described in #37245.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/Controls/src/Core/Xaml/Diagnostics/BindingDiagnostics.cs | Swaps the static event implementation to use WeakEventManager and routes event raising through it. |
| src/Controls/tests/Core.UnitTests/BindingUnitTests.cs | Adds memory regression tests for BindingDiagnostics.BindingFailed subscriber collection behavior. |
Suppressed comments (1)
src/Controls/src/Core/Xaml/Diagnostics/BindingDiagnostics.cs:44
- WeakEventManager is not thread-safe; raising the event should be synchronized with add/remove to avoid races when BindingFailed is raised concurrently with subscription changes.
_weakEventManager.HandleEvent(null, new BindingErrorEventArgs(VisualDiagnostics.GetSourceInfo(binding), binding, source, bo, bp, errorCode, message, messageArgs), nameof(BindingFailed));
| public static event EventHandler<BindingBaseErrorEventArgs> BindingFailed | ||
| { | ||
| add => _weakEventManager.AddEventHandler(value); | ||
| remove => _weakEventManager.RemoveEventHandler(value); | ||
| } |
| } | ||
| Application.Current?.FindMauiContext()?.CreateLogger<BindingDiagnostics>()?.LogWarning(message, messageArgs); | ||
| BindingFailed?.Invoke(null, new BindingBaseErrorEventArgs(VisualDiagnostics.GetSourceInfo(binding), binding, errorCode, message, messageArgs)); | ||
| _weakEventManager.HandleEvent(null, new BindingBaseErrorEventArgs(VisualDiagnostics.GetSourceInfo(binding), binding, errorCode, message, messageArgs), nameof(BindingFailed)); |
| // Regression tests for https://github.com/dotnet/maui/issues/37245. | ||
| // BindingDiagnostics.BindingFailed is a plain static event; instance | ||
| // subscribers that never unsubscribe are pinned by the multicast | ||
| // delegate for the process lifetime. |
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
MauiBot
removed
the
s/agent-review-in-progress
This comment has been minimized.
This comment has been minimized.
github-actions
Bot
added
the
s/agent-review-in-progress
MauiBot
left a comment
MauiBot
left a comment
There was a problem hiding this comment.
AI Review Summary
@Shalini-Ashokan — new AI review results are available based on commit
2bb3673.
🗂️ Review Sessions — click to expand
🚦 Gate — Test Before & After Fix
Gate Result: ✅ PASSED
Platform: ANDROID · Base: main · Merge base: f5789010
| Test | Without Fix (expect FAIL) | With Fix (expect PASS) |
|---|---|---|
🧪 BindingUnitTests BindingUnitTests |
✅ FAIL — 115s | ✅ PASS — 125s |
🔴 Without fix — 🧪 BindingUnitTests: FAIL ✅ · 115s
Error-relevant lines (filtered from the build log):
at Microsoft.Maui.Controls.Core.UnitTests.BindingUnitTests.BindingDiagnosticsBindingFailed_Leaky_SubscribedButNotUnsubscribed_IsCollected() in /_/src/Controls/tests/Core.UnitTests/BindingUnitTests.cs:line 2567
🟢 With fix — 🧪 BindingUnitTests: PASS ✅ · 125s
(no coded error found; showing last 1200 chars)
sed ValueUpdatedWithOldContextDoesNotUpdateWithTwoWayBinding(isDefault: False) [4 ms]
Passed StringFormat [9 ms]
Passed StringFormatOnUpdate [1 ms]
Passed SetBindingContextAndInnerBindingBeforeContextBinding [2 ms]
Passed TypedBindingsShouldNotHang [2 ms]
[xUnit.net 00:00:04.04] SpeedTestApply [SKIP]
[xUnit.net 00:00:04.04] SpeedTestApply
Passed ValueConverterParameter [4 ms]
Passed OneWay bindings should not double apply on source updates. [2 ms]
Passed OneWayToSourceContextSetToNull [1 ms]
Passed PropertyChangeBindingsOccurThroughMainThread [3 ms]
Passed You should get an exception when trying to change a binding after it's been applied [1 ms]
Passed The most derived version of a property should always be called. [1 ms]
Passed ValueUpdatedWithSimplePathOnOneWayBinding(isDefault: True) [1 ms]
Passed ValueUpdatedWithSimplePathOnOneWayBinding(isDefault: False) [1 ms]
Skipped SpeedTestApply [1 ms]
[xUnit.net 00:00:04.10] Finished: Microsoft.Maui.Controls.Core.UnitTests
Passed BindingDoesNotStayAliveForDeadTarget [51 ms]
Passed CloneMode [2 ms]
Test Run Successful.
Total tests: 308
Passed: 306
Skipped: 2
Total time: 4.7895 Seconds
📁 Fix files reverted (1 files)
src/Controls/src/Core/Xaml/Diagnostics/BindingDiagnostics.cs
📋 Pre-Flight — Context & Validation
PR #37645 Pre-Flight
Context
- Title: Fix memory leak in
BindingDiagnostics.BindingFailedby backing the static event withWeakEventManager - Issue: #37245, a process-wide static event strongly retains instance subscribers that omit
-=. - Base / reviewed commit:
mainatf578901012885b2845763e1587f7c230b25b0e8e; materialized review commitd85691791e3d782f5c41487eb2b2a172a29d1411. - Platform: Android. The defect and unit test are managed-code and platform-independent.
Direct Diff Assessment
The PR changes two files:
src/Controls/src/Core/Xaml/Diagnostics/BindingDiagnostics.cs- Replaces the ordinary static event backing field with one static
WeakEventManager. - Routes add/remove through
AddEventHandlerandRemoveEventHandler. - Routes both failure-emission overloads through
HandleEvent.
- Replaces the ordinary static event backing field with one static
src/Controls/tests/Core.UnitTests/BindingUnitTests.cs- Adds never-subscribed, subscribed-then-unsubscribed, and subscribed-without-unsubscribe collection tests.
The production fix file, and therefore the try-fix modification allow-list after baseline establishment, is:
src/Controls/src/Core/Xaml/Diagnostics/BindingDiagnostics.cs
The existing approach is a shared WeakEventManager with direct, unsynchronized add/remove/raise calls. Existing review comments note that WeakEventManager mutates a dictionary/list without synchronization, unlike normal event-field add/remove semantics, so concurrent subscription changes and raises are a known concern to avoid or address in alternatives.
Test Contract
The completed gate already proved the regression:
- Without the production fix:
BindingUnitTestsfails atBindingDiagnosticsBindingFailed_Leaky_SubscribedButNotUnsubscribed_IsCollected. - With the PR fix:
BindingUnitTestspasses (306 passed, 2 skipped).
Use only the detected primary unit-test target:
dotnet test src/Controls/tests/Core.UnitTests/Controls.Core.UnitTests.csproj --filter "BindingUnitTests"No additional mandatory regression tests were supplied. Do not rerun gate verification and do not run a full test suite.
Candidate Requirements
Each candidate must establish the broken baseline with EstablishBrokenBaseline.ps1, implement one mechanism-level alternative to the PR fix, perform the inline expert self-review, run the primary test once with at most one focused correction/retest, capture all required artifacts, and restore exclusively with EstablishBrokenBaseline.ps1 -Restore.
🔬 Code Review — Deep Analysis
Expert PR Evaluation
Verdict
REQUEST CHANGES (high confidence). The submitted fix proves that the leaking instance subscriber can be collected, but routing the shipped public event through WeakEventManager introduces unaddressed delivery, concurrency, and performance regressions.
Independent Assessment
The PR replaces the field-like static BindingDiagnostics.BindingFailed event with custom accessors backed by one process-wide WeakEventManager, and routes both failure-emission overloads through reflection-based weak dispatch. The three new tests correctly establish the reported retention bug: the gate failed without the production change and passed with it.
Findings
- Major — existing lambda subscriptions can silently disappear.
WeakEventManagerstores only a weak reference to the delegate target. A closure created by a startup-time lambda can be collected after its declaring method returns even while the application expects the event subscription to remain active. This breaks ordinary CLR event lifetime semantics for a common subscription form. - Major — delivery behavior is not tested. The added tests cover collection only. They do not prove that a live instance handler is invoked, that removal still works, or that the string-keyed reflection dispatch reaches
BindingFailed. - Major discussion concern — normal event concurrency guarantees are lost. The previous compiler-generated event used atomic delegate combination and snapshot invocation.
WeakEventManagermutates a sharedDictionary<string, List<Subscription>>without synchronization during add, remove, and dead-entry pruning. Binding failures and subscriptions can occur on different threads, so concurrent operations can corrupt state, lose subscriptions, or throw. - Moderate — the no-subscriber fast path is lost. The previous null-conditional invocation skipped event-argument construction and
VisualDiagnostics.GetSourceInfowhen no handlers existed. Both are now evaluated for every binding failure, followed by additionalWeakEventManagerlist and reflection allocations. - Minor — dead subscription metadata can accumulate. Collected targets are pruned only by raise or remove, leaving process-lifetime weak-reference and
MethodInfoentries when pages subscribe but no later binding failure occurs. - Minor — new test comments describe the pre-fix code as current. The comments and assertion message should describe the invariant under test rather than prescribe the
WeakEventManagerchange already present in the same PR.
The complete five inline findings are persisted in ../inline-findings.json. The actionable direction is to preserve weak subscriber collection without relying on the unsynchronized shared manager, while retaining atomic event operations, direct typed invocation, and an explicit no-subscriber fast path.
🛠️ Try-Fix — Analysis & Comparison
PR #37645 Alternative Fix Candidates
Candidate 1 — Copy-on-Write Weak Registry (claude-opus-5)
Result: PASS on the first and only run of BindingUnitTests (306 passed, 2 skipped, 0 failed).
The candidate replaces the static delegate with a private lock-guarded, copy-on-write weak subscription array. It weakly stores instance targets, strongly stores static handlers, invokes typed delegates outside the mutation lock, and removes subscriptions by exact method/target identity. Unlike the PR's unsynchronized shared WeakEventManager, concurrent add/remove/raise sees immutable snapshots and reentrant handlers cannot deadlock.
Self-review recorded two non-blocking findings: weak closures may be collected, and raises reconstruct typed delegates. The implementation is substantially larger than the PR fix (+199/-3).
Narrative: ../try-fix-1/content.md
Full diff: attempt-1/fix.diff
Test output: attempt-1/test-output.log
Candidate 2 — Ephemeron-Backed Forwarding Proxies (gpt-5.6-sol)
Result: PASS on the first and only run of BindingUnitTests (306 passed, 2 skipped, 0 failed).
The candidate retains a private ordinary CLR multicast event but registers forwarding proxies for instance handlers. Each proxy weakly locates its target and uses a ConditionalWeakTable ephemeron to retain the original typed delegate only while that target is independently reachable. Static handlers remain ordinary strong entries. This avoids both the PR's shared unsynchronized manager and candidate 1's manually raised copy-on-write registry while preserving native multicast add/remove/raise behavior.
Self-review recorded one minor finding: dead proxy metadata remains until another event operation prunes it, although it cannot retain the subscriber. The change is smaller than candidate 1 but still materially larger than the PR fix (+122/-3).
Narrative: ../try-fix-2/content.md
Full diff: attempt-2/fix.diff
Test output: attempt-2/test-output.log
Bounded Outcome
Exactly two candidates were attempted sequentially. Both passed the detected primary test on their initial implementation and required no correction/retest. Candidate 2 did not repeat candidate 1's storage mechanism.
📝 PR Finalize — Recommended Title & Description
Assessment: ✏️ Recommend updating — the current title lacks the repository's component prefix and the otherwise strong description omits the two changed files and added regression-test coverage.
Recommended title
[Controls] BindingDiagnostics: Prevent BindingFailed subscriber leaks with WeakEventManager
Recommended description
### Issue Details
When a page or view model subscribes to `BindingDiagnostics.BindingFailed` and forgets to unsubscribe, that page never gets collected. Everything it retains, including images, view models, and data, can remain alive until the app closes, causing memory usage to grow over time.
### Root Cause
`BindingFailed` was a process-lifetime static event. A normal C# event strongly references its instance subscribers, so a subscriber that omitted `-=` remained rooted for the lifetime of the application.
### Description of Change
Changed `BindingDiagnostics.BindingFailed` from a field-like static event to custom accessors backed by `WeakEventManager`. Event add/remove operations now use `AddEventHandler` and `RemoveEventHandler`, and both binding-failure emission paths use `HandleEvent`.
`WeakEventManager` weakly retains instance subscriber targets, allowing the garbage collector to collect a page or view model even when it did not unsubscribe.
### Files Changed
- `src/Controls/src/Core/Xaml/Diagnostics/BindingDiagnostics.cs` - backs `BindingFailed` with `WeakEventManager` and routes both failure-emission overloads through it.
- `src/Controls/tests/Core.UnitTests/BindingUnitTests.cs` - adds collection tests for never-subscribed, subscribed-then-unsubscribed, and subscribed-without-unsubscribe cases.
### Validation
Validated the behavior on:
- [x] Android
- [x] Windows
- [x] iOS
- [x] Mac
### Issues Fixed
Fixes #37245
### Fix References
- [`Application.ApplicationChanged`](https://github.com/dotnet/maui/blob/bf62313db2cecf3215f10c353720b341aba0a4ee/src/Controls/src/Core/Application/Application.cs#L263)
- [`Command.CanExecuteChanged`](https://github.com/dotnet/maui/blob/bf62313db2cecf3215f10c353720b341aba0a4ee/src/Controls/src/Core/Command.cs#L128)
- [`ImageSource.SourceChanged`](https://github.com/dotnet/maui/blob/bf62313db2cecf3215f10c353720b341aba0a4ee/src/Controls/src/Core/ImageSource.cs#L175)
### Output Screenshot
| Before | After |
|--|--|
| <video src="https://github.com/user-attachments/assets/86c6d76d-a051-458b-beb3-6c254545f87a"> | <video src="https://github.com/user-attachments/assets/e3b02ce7-71a7-4e90-94e0-dd6587c8e3cf"> |
🏁 Report — Final Recommendation
⚠️ Final Recommendation: REQUEST CHANGES
Winner: pr-plus-reviewer. The submitted PR fixes the reported retention bug, and its trusted gate passes, but the expert review found unresolved public-event delivery, concurrency, performance, and coverage regressions in the shared WeakEventManager approach. The consolidated reviewer candidate keeps the required weak-collection behavior while addressing those actionable findings, and its one required targeted run passed with 307 passed and 2 skipped.
Comparative Analysis
| Rank | Candidate | Regression result | Assessment |
|---|---|---|---|
| 1 | pr-plus-reviewer |
PASS — 307 passed, 2 skipped | Uses an ordinary private CLR event with ephemeron-backed weak forwarding proxies, preserving atomic add/remove, snapshot dispatch, multicast removal semantics, direct typed exceptions, and the zero-listener short-circuit. It also documents weak lambda lifetime, adds live-delivery coverage, and corrects stale test text. |
| 2 | try-fix-2 |
PASS — 306 passed, 2 skipped | Uses the same strongest underlying proxy/ephemeron mechanism and avoids the PR's shared mutable manager. It ranks below the winner because it still constructs event arguments before discovering there are no listeners, does not add delivery coverage or public weak-lifetime documentation, and leaves stale test text untouched. |
| 3 | try-fix-1 |
PASS — 306 passed, 2 skipped | Its lock-guarded copy-on-write registry fixes the concurrency problem and invokes typed handlers outside the lock. It is substantially larger, reconstructs delegates on every raise, retains the no-listener argument-construction regression, and removes multicast entries individually rather than matching the last contiguous invocation-list subsequence as a CLR event does. |
| 4 | pr |
PASS — gate passed with 306 passed, 2 skipped | Smallest change and proven to release an abandoned instance subscriber, but WeakEventManager weakens common lambda lifetime semantics without documentation, mutates shared dictionary/list state without synchronization, replaces typed dispatch with reflection, loses the no-subscriber short-circuit, and has no delivery test. Expert verdict: request changes. |
No candidate failed its regression run, so no passing candidate was ranked beneath a failing one. pr-plus-reviewer wins on correctness and behavioral compatibility rather than test outcome alone.
Remaining Trade-off
Every candidate that makes the existing event weak necessarily changes ordinary CLR event lifetime semantics for unrooted closure targets. The winner makes that contract explicit and requires callers to retain a lambda, local delegate, or target while notifications are needed. Dead proxy metadata may persist until later event activity, but it does not retain the subscriber.
Because the winning changes are not present in the submitted PR HEAD, the recommendation is REQUEST CHANGES.
📱 UI Tests — Button,Label,Layout
Detected UI test categories: Button,Label,Layout
✅ Deep UI tests — 360 passed, 0 failed, 7 skipped across 3 categories on platform-pool agent (replaces in-process counts above).
🧪 UI Test Execution Results (deep, platform pool)
| Category | Tests | Snapshot diffs |
|---|---|---|
Button |
71/73 (2 skipped) ✓ | — |
Label |
97/99 (2 skipped) ✓ | — |
Layout |
192/195 (3 skipped) ✓ | — |
📎 Download drop-deep-uitests artifact (TRX + snapshot diffs) |
🧭 Next Steps — reviewer patch required (pr-plus-reviewer)
The reviewer-enhanced candidate won, so the submitted PR still needs those changes.
Why: pr-plus-reviewer preserves the proven weak-subscriber collection fix while restoring atomic CLR-event behavior, typed dispatch, the no-listener fast path, and live-delivery coverage. Its targeted BindingUnitTests run passed with 307 passed and 2 skipped.
Apply PRAgent/pr-plus-reviewer/reviewer.patch from the CopilotLogs
artifact (or follow the report's Required submitted-PR change), push the update, and run
the review again.
MauiBot
added
s/agent-changes-requested
MauiBot
added
s/agent-fix-win
Note
Are you waiting for the changes in this PR to be merged?
It would be very helpful if you could test the resulting artifacts from this PR and let us know in a comment if this change resolves your issue. Thank you!
Issue Details
When a page or view model subscribes to BindingDiagnostics.BindingFailed (a helpful event that tells you when a data binding fails) and forgets to unsubscribe, that page never gets cleaned up from memory. Every page it holds — plus its images, view models, and data — stays alive until the app closes. Over time, the app uses more and more memory.
Root Cause
BindingFailed is a static event, which means it lives for the whole life of the app. Normal C# events keep a strong reference to whoever subscribed, so as long as the event is alive, its subscribers stay alive too. Since this event never goes away, any subscriber that doesn't call -= is stuck in memory forever.
Description of Change
Switched BindingFailed from a normal event to one backed by WeakEventManager, which keeps only a weak reference to subscribers — so the garbage collector can clean them up even if someone forgets to unsubscribe.
Validated the behavior in the following platforms
Issues Fixed
Fixes #37245
Fix reference
maui/src/Controls/src/Core/Application/Application.cs at bf62313db2cecf3215f10c353720b341aba0a4ee · …
maui/src/Controls/src/Core/Command.cs at bf62313db2cecf3215f10c353720b341aba0a4ee · dotnet/maui
maui/src/Controls/src/Core/ImageSource.cs at bf62313db2cecf3215f10c353720b341aba0a4ee · dotnet/maui
Output ScreenShot
37245-BeforeFix.mov
37245-AfterFix.mov