Fix CI failure for CollectionView Scrolling Feature Tests due to PR #35379

[devanathan-vaithiyanathan]

Issue Details

After PR #35379 , CollectionView Scrolling feature matrix tests started failing in CI on Android.

Description of Change

During recycling, the code could attempt to remove a native view even after its Android handle was already disposed (invalid JNI handle), which can cause instability and intermittent failures under heavy scrolling/recycle paths, checks platformView is not null and its native handle is valid (Handle != IntPtr.Zero) before calling RemoveView.

Fixes

• CollectionView_ScrollingFeatureTests

[kubaflo]

/review -b feature/regression-check -p android

[MauiBot]

🤖 AI Summary

👋 @devanathan-vaithiyanathan — new AI review results are available. Please review the latest session below.

📊 Review Session — 445fe51 · Update ItemContentView.cs · 2026-05-20 20:03 UTC

🚦 Gate — Test Before & After Fix

Gate Result: ⚠️ SKIPPED

No tests were detected in this PR.

Recommendation: Add tests to verify the fix using the write-tests-agent.

---

🧪 UI Tests — CollectionView

Detected UI test categories: CollectionView

🧪 UI Test Execution Results

⏭️ SKIPPED — 0 passed, 0 failed, 1 skipped (platform: android)

Category	Result	Tests	Duration	Notes
CollectionView	⏭️ SKIPPED	—	1.6s	Runner threw an exception

Failures here are informational only — they do not block the gate or affect try-fix candidate scoring.

---

🔍 Pre-Flight — Context & Validation

Pre-Flight Context — PR #35536

PR Summary

• Title: Fix CI failure for CollectionView Scrolling Feature Tests due to PR [Android] Fix CollectionView scrolling performance regression #35379
• Author: devanathan-vaithiyanathan (partner/syncfusion, community)
• Base branch: inflight/current
• Files changed: 1 (src/Controls/src/Core/Handlers/Items/Android/ItemContentView.cs), +1/-1
• Labels: area-controls-collectionview, community, partner/syncfusion

The Fix (PR's current diff)

- if (platformView != null)
+ if (platformView is not null && platformView.Handle != IntPtr.Zero)
 {
     RemoveView(platformView);
 }

Root-cause hypothesis

PR #35379 split CollectionView's Android recycle into two paths:

1. Lightweight scroll-recycle — OnViewRecycled → TemplatedItemViewHolder.Recycle(ItemsView) removes only the logical child; it does NOT call _itemContentView.Recycle().
2. Deep teardown — Adapter.Dispose() → DisconnectTemplatedViewHolders() walks tracked WeakReference<TemplatedItemViewHolder> and calls holder.DisconnectAndRecycle(ItemsView), which calls _itemContentView.Recycle() and then _itemContentView.Recycle() calls RemoveView(platformView).

During teardown, the Android View instances backing the recycler pool may already have had their JNI handle disposed (Activity tear-down, GC of Java peers, etc.). Calling ViewGroup.RemoveView(View) with Handle == IntPtr.Zero throws ObjectDisposedException / invalid JNI handle, causing the CI failure in CollectionView_ScrollingFeatureTests.

Bind() (template-changing path) and DisconnectAndRecycle() are the two callers that route through ItemContentView.Recycle() after #35379.

Gate status

⚠️ SKIPPED — no tests detected in this PR. (PR is a CI failure mitigation; no new test was added.)
Recommendation to author: add a focused device/regression test that exercises rapid CollectionView teardown during scroll, or document why a unit test isn't viable.

Considerations for alternative fixes

1. Idiomatic helper: MAUI already ships JavaObjectExtensions.IsAlive() (src/Core/src/Platform/Android/JavaObjectExtensions.cs) that exactly encodes obj != null && obj.Handle != IntPtr.Zero. It's already used at line 214 of this same file (this.IsAlive()).
2. Semantic guard: RemoveView only makes sense if platformView.Parent == this. If the parent is different/null, skip is more correct than just handle check.
3. Symmetric guard: View?.DisconnectHandlers() runs after the RemoveView — if the platformView is dead, the managed virtual view may still be safe to disconnect, but worth considering.
4. Root-cause: The tracked-holder list in ItemsViewAdapter uses WeakReference<TemplatedItemViewHolder>, but the underlying ItemContentView lives on the holder's ItemView. By the time Dispose(true) runs the activity may be down. Eagerly disconnecting in OnDetachedFromWindow (Adapter-level OnDetachedFromRecyclerView) instead of solely in Dispose would tear down while handles are still alive.

Test gate evidence

• Gate report: skipped (no test in diff).
• The failing tests originally cited: CollectionView_ScrollingFeatureTests (on Android). No reproduction harness exists in the PR.

---

🔧 Fix — Analysis & Comparison

Try-Fix Aggregate — PR #35536

Three meaningfully different fix alternatives were generated by the
maui-expert-reviewer agent, applied to the working tree, and verified by
building Controls.Core for net10.0-android36.0 in Release configuration.
All three compile cleanly. Per-candidate detail in try-fix-{1,2,3}/content.md.

Gate context

• Gate result: ⚠️ SKIPPED — no tests detected in this PR.
• The PR (1-line guard) reportedly fixes failing
  CollectionView_ScrollingFeatureTests introduced by PR [Android] Fix CollectionView scrolling performance regression #35379 on Android,
  but adds no test. Recommendation to author: add a focused device test that
  exercises rapid scroll → page-pop / Shell tab switch to lock in the fix.
• Live device-test reruns are not available in this autonomous environment;
  evaluation here is build-verified + static-analysis of each candidate.

Candidate matrix

#	Approach	Δ files	Compiles (android)	Addresses root cause?	Public API delta?	Verdict
PR	Handle != IntPtr.Zero open-coded	1	✅ (baseline)	❌ guards symptom	none	minimal patch
1	IsAlive() extension + guard this	1	✅	❌ (idiomatic + symmetric guard)	none	lowest-risk improvement
2	ReferenceEquals(Parent, this) semantic guard	1	✅	partial (encodes precondition)	none	best correctness
3	Adapter OnDetachedFromRecyclerView deterministic teardown + IsAlive safety net	2 + PublicAPI	✅ (after PublicAPI entry)	✅ root cause	+1 line	most thorough

Build evidence

All three candidates built successfully with:

dotnet build src/Controls/src/Core/Controls.Core.csproj \
  -f net10.0-android36.0 -c Release \
  -p:IncludeAndroidTargetFrameworks=true --no-restore

Each ended with Build succeeded. 0 Warning(s) 0 Error(s).

Candidate 3 initially failed with RS0016 from the public API analyzer
(correctly catching the new public override on ItemsViewAdapter<,>). After
adding the entry to
src/Controls/src/Core/PublicAPI/net-android/PublicAPI.Unshipped.txt, it built
cleanly.

After verification, all working-tree changes were reverted so the PR branch is
unchanged.

Recommendations to PR author (in priority order)

1. Adopt candidate 1 as a near-zero-risk replacement of the inline check.
   It improves idiomaticity, matches IsAlive() usage already in this file at
   line 214, and adds symmetric protection for this. One-line change, no
   public API impact.
2. Consider candidate 2 if reviewer prefers a semantic / precondition-based
   guard. Catches an additional class of bug (live-but-foreign parent).
3. Plan candidate 3 as a follow-up to remove the root-cause race instead of
   masking it. This is the right long-term fix; it can layer on top of
   candidates 1 or 2.
4. Add a regression test — even if the symptom is hard to reproduce
   deterministically, a Helix/device test that rapidly scrolls and
   tears down a CollectionView page would lock in any of these fixes.

Stop condition

Three distinct approaches generated and build-verified. Further candidates
would be trivial variations (e.g., swap IsAlive order, add try/catch). Loop
terminated per "exhausted meaningfully different approaches".

---

📋 Report — Final Recommendation

Comparative Report — PR #35536

Candidates evaluated

Candidate	Source	Δ files	Build (android Release)	Regression test posture	Public API
pr	Author's 1-line guard (Handle != IntPtr.Zero)	1	✅ baseline	⚠️ no test; gate SKIPPED	none
pr-plus-reviewer	pr + reviewer items #1 + #2 (use IsAlive(), guard this)	1	✅ verified	⚠️ same (no new tests)	none
try-fix-1	IsAlive() extension + guard this	1	✅	⚠️ same	none
try-fix-2	ReferenceEquals(platformView.Parent, this) semantic guard	1	✅	⚠️ same	none
try-fix-3	Adapter OnDetachedFromRecyclerView deterministic teardown + IsAlive() safety net	2 + PublicAPI	✅	⚠️ same	+1 line override

Note on gate. The gate was SKIPPED upstream because the PR adds no tests.
Per the ranking rule "candidates that failed regression tests must be ranked
lower than candidates that passed them": no candidate failed regression tests
(no regression tests existed to run), so the rule does not separate them.

Scoring matrix (1=worst → 5=best)

Criterion (weight)	pr	pr-plus-reviewer	try-fix-1	try-fix-2	try-fix-3
Correctness — handles dead-peer race (×3)	4	5	5	4	5
Correctness — handles foreign-parent (×2)	1	1	1	5	1
Idiomatic / consistency with file (×2)	2	5	5	4	5
Symmetry (guards both peers) (×2)	1	5	5	3*	5
Risk of behavior regression (×2, higher=safer)	5	5	5	3	3
Root-cause-ness (×1)	1	2	2	3	5
Public API / churn footprint (×1, higher=smaller)	5	5	5	5	2
Weighted total / 65	30	44	44	39	40

*try-fix-2 guards via Parent==this; a disposed this peer still throws on
the RemoveView call (instance method JNI bridge), so symmetry is partial.

Tie-breaker: pr-plus-reviewer vs try-fix-1

The two candidates are functionally equivalent — both replace the guard with
this.IsAlive() && platformView.IsAlive() and both build cleanly. They are
the same edit. pr-plus-reviewer carries an additional explanatory comment
documenting why the symmetry matters (tying into reviewer finding #2);
try-fix-1 carries a different comment explaining the idiom switch.

Tie broken in favor of pr-plus-reviewer because:

1. It is anchored to PR-author intent (mechanical application of inline
   reviewer feedback), making for a smaller cognitive delta on review.
2. Its comment explicitly names the asymmetry hazard (Activity-teardown
   collecting both peers), which is the bug class being defended against.
3. It carries the explicit imprimatur of the expert reviewer pass, which is
   exactly the role of the "pr-plus-reviewer" track.

Why each non-winner lost

• pr: works, but inconsistent with IsAlive() already used 150 lines
  below in the same file and asymmetric — this peer can also die.
• try-fix-1: tied on the merits with the winner; lost only on
  provenance tie-breaker. Acceptable substitute.
• try-fix-2: catches an additional bug class (live-but-foreign parent),
  but introduces a behavioral change relative to the PR (silently skips when
  parent differs). For an unblock-CI PR targeting inflight/current, the
  smaller behavioral footprint of the winner is preferred. Worth landing as a
  follow-up.
• try-fix-3: best architectural fix and removes the race at its source,
  but adds public API surface (OnDetachedFromRecyclerView override on
  ItemsViewAdapter<,>) and changes lifecycle ordering on Shell tab switches —
  warrants its own PR with device-test coverage. Recommended as the follow-up
  PR after this one merges.

Test coverage (applies to ALL candidates)

The reviewer's strongest finding is #6: a fix to a regression with zero
regression test guarantees future re-regression. Independent of which
candidate ships, the author should add a unit test in
Controls.Core.UnitTests that drives RealizeContent → simulates
PlatformView.Handle == IntPtr.Zero → calls Recycle() and asserts no throw.
This is portable across all five candidates and is not a differentiator.

Winner

pr-plus-reviewer — equal to try-fix-1 on merits, edges it on
provenance and comment quality, dominates pr on idiomaticity and symmetric
protection, beats try-fix-2/try-fix-3 on behavior conservatism for an
inflight CI-unblock change. Public API delta: zero. Single-file, 1-line
predicate change (+ explanatory comment).

Follow-ups recommended to the author

1. This PR: adopt pr-plus-reviewer diff.
2. Same PR or follow-up: add a regression unit test as described above
   (reviewer finding [Spec] Transitions #6).
3. Follow-up PR: land try-fix-3 to remove the race at its source
   (reviewer finding Aloha System.Maui! #4) and add Shell tab-switch device coverage.
4. Follow-up PR: evaluate try-fix-2's RemoveFromParent() /
   ReferenceEquals(Parent, this) predicate for the foreign-parent class of
   bug (reviewer finding Third #3).

---