Issue: #24304 - Crash on NullReferenceException with measurement cells in CollectionView
PR: #24610 - [Android] Ensure disconnected ItemsViewHandler doesn't hold onto the items source
Author: filipnavara
Platforms Affected: Android (primary fix), iOS (via MarshalingObservableCollection change)
Files Changed: 3 implementation, 1 test

Key Findings

• Root Cause: MarshalingObservableCollection (used on Android when wrapping IList + INotifyCollectionChanged) subscribed to the source collection's CollectionChanged event directly in constructor but never unsubscribed. When DisconnectHandler() was called, the subscription remained, keeping the zombie UI tree alive.

• Scenario: App creates a page with CarouselView/CollectionView bound to an ObservableCollection. Page is navigated away, DisconnectHandler() is called. But if the ObservableCollection outlives the page, future changes to it trigger the old CollectionChanged callback on a half-disposed UI tree, causing NullReferenceException crashes.

• Fix Approach:

  1. Replace direct CollectionChanged += InternalCollectionChanged in MarshalingObservableCollection with a WeakNotifyCollectionChangedProxy (cross-platform change)
  2. Add Dispose() to MarshalingObservableCollection that calls _proxy.Unsubscribe()
  3. In ObservableItemsSource (Android), add disposeItemsSource flag so MarshalingObservableCollection gets properly disposed when the source is unwrapped
  4. In ItemsSourceFactory (Android), pass disposeItemsSource: true when wrapping in MarshalingObservableCollection
  5. Remove #if !ANDROID guard from DisconnectedCarouselViewDoesNotHookCollectionViewChanged test

• Prior Agent Review: A previous agent (kubaflo) approved this PR. Gate PASSED.

• Test Type: Device Test (src/Controls/tests/DeviceTests/Elements/CarouselView/CarouselViewTests.cs)

• Test Method: DisconnectedCarouselViewDoesNotHookCollectionViewChanged

• Discussion: PR had lively discussion about whether to use IDisposable on MarshalingObservableCollection. The current approach uses an internal Dispose() method (not IDisposable interface). There was concern that MarshalingObservableCollection tests relied on collection changing without subscribing to CollectionChanged event.

Fix Candidates

Test Verification Gate — PR #24610

Status: ✅ GATE PASSED

Summary

✅ Final Verdict: VERIFICATION PASSED

The tests correctly detect the issue:

• ✅ DisconnectedCarouselViewDoesNotHookCollectionViewChanged FAILS without the fix (as expected — bug is present)
• ✅ DisconnectedCarouselViewDoesNotHookCollectionViewChanged PASSES with the fix (as expected — bug is fixed)

Conclusion: The test properly validates the fix and catches the bug when it's present.

Configuration

Fix Files Reverted/Restored

Note: The test file CarouselViewTests.cs was NOT reverted — the #if !ANDROID guard removal was kept in both runs (the test needs to exist on Android in both phases).

Test Results Details

Phase 1: WITHOUT Fix

Expected: Test should FAIL (bug is present)
Actual: ❌ FAILED — as expected

03-25 14:52:52.804 I DOTNET  :    Test name: DisconnectedCarouselViewDoesNotHookCollectionViewChanged
03-25 14:52:52.822 I DOTNET  :    Exception stack traces: at Microsoft.Maui.DeviceTests.CarouselViewTests.DisconnectedCarouselViewDoesNotHookCollectionViewChanged()
03-25 14:52:52.822 I DOTNET  : 	[FAIL] DisconnectedCarouselViewDoesNotHookCollectionViewChanged
03-25 14:52:52.822 I DOTNET  : Actual:   False

Failure reason: Assert.True(data.IsCollectionChangedEventEmpty) failed with Actual: False. Without the fix, the MarshalingObservableCollection directly subscribes to the original collection's CollectionChanged event and does not unsubscribe when the handler is disconnected. The subscription remains active, proving the bug is present.

Phase 2: WITH Fix

Expected: Test should PASS (bug is fixed)
Actual: ✅ PASSED — as expected

03-25 15:08:07.261 I DOTNET  : Test collection for Microsoft.Maui.DeviceTests.CarouselViewTests
03-25 15:08:07.855 I DOTNET  : 	[PASS] DisconnectedCarouselViewDoesNotHookCollectionViewChanged
03-25 15:08:11.869 I DOTNET  : Microsoft.Maui.DeviceTests.CarouselViewTests 4.5884244 ms

Fix explanation: The fix uses WeakNotifyCollectionChangedProxy in MarshalingObservableCollection instead of a direct event subscription, and adds a Dispose() method that calls _proxy.Unsubscribe(). The ObservableItemsSource is updated to call Dispose() on the MarshalingObservableCollection when its own Dispose() is called (with disposeItemsSource: true). ItemsSourceFactory passes disposeItemsSource: true to ensure cleanup happens on disconnect.

Log Files

• verify-tests-fail/build-without-fix.log — Build log (without fix)
• verify-tests-fail/run-without-fix.log — xharness run log (without fix)
• verify-tests-fail/without-fix/adb-logcat-com.microsoft.maui.controls.devicetests-default.log — Device logcat (without fix)
• verify-tests-fail/build-with-fix.log — Build log (with fix)
• verify-tests-fail/run-with-fix.log — xharness run log (with fix, includes CarouselView test results)

Fix Candidates

Cross-Pollination

| 5 | gemini-3-pro-preview | Explicit Watch()/Unwatch() methods in MarshalingObservableCollection with autoSubscribe param; OIS calls Watch/Unwatch in constructor/Dispose | ✅ PASS | 3 files | Explicit lifecycle control with backward compat |

| 6 | gpt-5.3-codex | Force MapItemsSource(null) in Android DisconnectHandler to trigger normal teardown path | ❌ FAIL | 1 file | Fails: PlatformView is null when DisconnectHandler fires |

Round 3 Cross-Pollination

Exhausted: Yes (max 3 rounds reached; remaining ideas are variants of existing approaches or already disproven)

Selected Fix: PR #24610's fix — WeakNotifyCollectionChangedProxy + disposeItemsSource flag

Reason for selecting PR's fix:

1. Defense-in-depth: Even if Dispose() is somehow skipped, the WeakNotifyCollectionChangedProxy means the subscription is weak — it won't keep the zombie UI tree alive on its own. The 5 passing alternatives all rely on Dispose() being called; the PR's fix works even if it isn't.
2. Codebase consistency: WeakNotifyCollectionChangedProxy is already used in ObservableItemsSource itself, making the PR's approach stylistically consistent.
3. Gate verified: Test FAILS without fix, PASSES with fix — empirically confirmed on Android emulator.
4. Attempt 1 is simplest: If reviewers prefer fewer lines, Attempt 1 (2 files, direct -= unsubscribe) is a viable, simpler alternative with the same test pass — though it lacks the weak-reference safety net.

✅ Final Recommendation: APPROVE

Phase Status

Summary

PR #24610 correctly fixes a memory-leak/zombie-UI-tree bug where CarouselView/CollectionView bound to an ObservableCollection never disconnected from ItemsSource.CollectionChanged when DisconnectHandler() was called. If the collection outlived the page, future changes would crash the app with NullReferenceException (issue #24304).

The fix was empirically verified on Android: the new device test DisconnectedCarouselViewDoesNotHookCollectionViewChanged fails without the PR's changes and passes with them. Five independent alternative fixes were explored and all confirmed the approach is sound; the PR's fix was selected as the best.

Root Cause

On Android, ItemsSourceFactory wraps IList + INotifyCollectionChanged in a MarshalingObservableCollection, which subscribed directly (+=) to the source collection's CollectionChanged in its constructor. This strong reference was never removed:

1. Page navigated away → DisconnectHandler() called on tree
2. MarshalingObservableCollection still held a strong CollectionChanged subscription to the user's ObservableCollection
3. The user's collection (long-lived) kept MarshalingObservableCollection → ObservableItemsSource → ItemsViewAdapter → RecyclerView (and all its cells) alive
4. Future mutations to the user's collection triggered CollectionChanged on the now-partially-disposed UI tree → crash

Fix Quality

PR's approach is sound and has defense-in-depth:

1. MarshalingObservableCollection — replaces direct CollectionChanged += InternalCollectionChanged with a WeakNotifyCollectionChangedProxy. This means even if Dispose() is never called (e.g., accidental GC cycle), the weak reference won't hold the zombie tree alive. Adds internal Dispose() that calls _proxy.Unsubscribe().

2. ObservableItemsSource — adds disposeItemsSource constructor parameter (default false) and, when true, calls Dispose() on a MarshalingObservableCollection during its own Dispose().

3. ItemsSourceFactory — passes disposeItemsSource: true when creating ObservableItemsSource with a MarshalingObservableCollection (i.e., the factory created the wrapper and owns it).

4. CarouselViewTests.cs — removes #if !ANDROID guard, enabling the existing test on Android.

Why PR's fix is better than alternatives:

The PR's WeakNotifyCollectionChangedProxy is already established in ObservableItemsSource (used for its own subscription), so this change is stylistically consistent with the codebase.

Observations / Minor Notes

1. MarshalingObservableCollection.Dispose() is internal, not IDisposable. This is intentional — the class is public and making it public IDisposable would change its API surface. The internal method is acceptable since the caller (ObservableItemsSource) is internal too.

2. PR label says platform/ios but the code changes are Android-specific (plus the cross-platform MarshalingObservableCollection). The issue Crash on NullReferenceException with measurement cells in CollectionView #24304 originally reported the crash on iOS, but the root cause also affects Android. The PR title correctly says [Android]. This is not a blocker — just worth noting the label may be misleading.

3. No iOS changes — The iOS items implementation (Items2/iOS/) uses ObservableItemsSource differently. The fix ensures Android is covered; iOS coverage should be tracked separately if needed.

Selected Fix: PR's fix ✅

Verdict: The PR correctly identifies the root cause, implements a robust fix with defense-in-depth via WeakNotifyCollectionChangedProxy, and the test properly validates the fix. Multi-model exploration (6 independent attempts) confirms the approach is sound. Recommend APPROVE.