[Android] Fix MapHandler leaks when removed Pin instances are retained by app code

[SubhikshaSf4851]

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 whether this change resolves your issue. Thank you!

This pull request improves the way map pin event subscriptions are managed in the MapHandler.Android implementation. The main change is to ensure that property change event handlers for map pins are only attached once, and are properly cleaned up, preventing duplicate subscriptions and potential memory leaks.

Description of Change

Pin subscription management improvements:

• Introduced a _subscribedPins HashSet<IMapPin> field to track pins that have event handlers attached, ensuring each pin is subscribed only once.
• Updated the AddPins method to only add a property changed event handler if the pin hasn't already been subscribed, using the _subscribedPins set.
• Modified the DisconnectPins method to iterate over _subscribedPins instead of the pin list, removing event handlers from only those pins that were actually subscribed, and resetting the tracking set afterwards.

Regarding Tests :

No automated device test included. MapView.OnCreate throws IllegalStateException when no valid Google Maps API key is present — even an empty string value fails. This is the same constraint that keeps MapTests.cs behind #if IOS || MACCATALYST. Adding the key to AndroidManifest.xml would expose credentials in the repo, which is not acceptable.

Issues Fixed

Fixes #35571

Tested the behavior in the following platforms

• Windows
• Android
• iOS
• Mac

Before Issue Fix	After Issue Fix
Fix35571Before.mov	Fix35571after.mov

[dotnet-policy-service]

Hey there @@SubhikshaSf4851! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

[github-actions]

🔍 Skill Validation Results

✅ Static Checks Passed

Skills checked: 18 | Agents checked: 4

Full validator output

Found 2 skill(s)
[agentic-labeler] 📊 agentic-labeler: 2,839 BPE tokens [chars/4: 2,788] (standard ~), 8 sections, 0 code blocks
[agentic-labeler]    ⚠  Skill is 2,839 BPE tokens (chars/4 estimate: 2,788) — approaching "comprehensive" range where gains diminish.
[agentic-labeler]    ⚠  No code blocks — agents perform better with concrete snippets and commands.
[find-regression-risk] 📊 find-regression-risk: 967 BPE tokens [chars/4: 905] (detailed ✓), 10 sections, 2 code blocks
[find-regression-risk]    ⚠  No YAML frontmatter — agents use name/description for skill discovery.
✅ All checks passed (2 skill(s))
Found 4 agent(s)
Validated 4 agent(s)

✅ All checks passed (4 agent(s))

⏭️ LLM Evaluation: Skipped

💡 LLM evaluation was not run for this external PR.
A repository contributor can post /evaluate-skills on this PR to trigger full evaluation.

🔍 Full results and investigation steps

[kubaflo]

/review -b feature/enhanced-reviewer

[MauiBot]

Expert Review — 1 findings

See inline comments for details.

[kubaflo]

Could you check the ai's suggestions?

[SubhikshaSf4851]

Could you check the ai's suggestions?

@kubaflo i have checked the Ai summary and updated the suggested changes

[vishnumenon2684]

/azp run maui-pr-uitests, maui-pr-devicetests

[azure-pipelines]

Azure Pipelines successfully started running 2 pipeline(s).

[kubaflo]

/review -b feature/enhanced-reviewer

[MauiBot]

AI Review Summary

@SubhikshaSf4851 — new AI review results are available based on this last commit: 4afe1e0.
Added suggestion To request a fresh review after new comments or commits, comment /review rerun.

Review Sessions — click to expand

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

Full UI test matrix will run (no specific categories detected from PR changes).

---

Pre-Flight — Context & Validation

Issue: #35571 - [inflight regression] Android MapHandler leaks when removed Pin instances are retained by app code
PR: #35686 - [Android] Fix MapHandler leaks when removed Pin instances are retained by app code
Platforms Affected: Android
Files Changed: 1 implementation, 0 test

Key Findings

• Android-only retained removed-pin leak was introduced by subscribing Pin.PropertyChanged without unsubscribing pins that had already been removed from Map.Pins before handler disconnect.
• PR fix records every subscribed pin in a reference-equality HashSet<IMapPin> and disconnects/unsubscribes that tracked set, addressing retained removed pins and value-equal duplicate pins.
• Gate was previously skipped because no tests were detected; the PR description says automated Android Map tests are blocked by Google Maps API key requirements.
• GitHub CLI authentication was unavailable in this environment, so PR/issue context was fetched through unauthenticated GitHub API where possible.

Code Review Summary

Verdict: NEEDS_DISCUSSION
Confidence: high
Errors: 0 | Warnings: 1 | Suggestions: 0

Key code review findings:

• ⚠️ CI is currently failing across required checks according to the code-review sub-agent; failures appear unrelated to this Android MapHandler diff but require CI triage before merge.

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
PR	PR #35686	Track subscribed pins in HashSet<IMapPin>(ReferenceEqualityComparer.Instance) and unsubscribe/disconnect all tracked pins in DisconnectPins()	⚠️ Gate skipped; targeted Android Maps build passed locally	src/Core/maps/src/Handlers/Map/MapHandler.Android.cs	Original PR fix; code review found no code errors

---

Code Review — Deep Analysis

Code Review — PR #35686

Independent Assessment

What this changes: Android MapHandler now tracks pins it subscribed to and unsubscribes/disconnects those tracked pins during cleanup.
Inferred motivation: Fix retained removed-pin leaks and avoid duplicate PropertyChanged subscriptions.

Reconciliation with PR Narrative

Author claims: Fixes Android map pin subscription leaks for retained removed pins.
Agreement/disagreement: Agrees. Existing review feedback about value-equality HashSet was addressed with ReferenceEqualityComparer.Instance.

Findings

No high-confidence code findings. inline-findings.json contains [].

⚠️ Warning — CI is currently failing

Required checks are failing across maui-pr, maui-pr-devicetests, and maui-pr-uitests. The visible failures appear unrelated to this one-file Android MapHandler diff, but the PR is not merge-ready until CI is green or failures are triaged.

Devil's Advocate

I rechecked reconnect/repeated MapPins flow, removed-pin retention, duplicate subscriptions, and handler disconnect symmetry. The tracked-set approach appears sound and narrower than the old VirtualView.Pins cleanup.

Verdict: NEEDS_DISCUSSION

Confidence: high
Summary: Code change looks correct, but required CI is failing, so I cannot mark LGTM. Human/CI triage should confirm failures are unrelated or rerun to green.

---

Fix — Analysis & Comparison

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
1	maui-expert-reviewer	Disposable PinSubscription objects own marker updates and event unsubscription	❌ FAIL	1 file	Failed Android build due unassigned marker outside IMapPinHandler branch
2	maui-expert-reviewer	Reference-distinct active-pin snapshot/diff unsubscribes removed pins immediately on MapPins remap	✅ PASS	1 file	Targeted Android Maps build passed; more proactive cleanup than PR fix
PR	PR #35686	Reference-equality HashSet<IMapPin> tracks all subscribed pins and unsubscribes them in DisconnectPins()	⚠️ Gate skipped; targeted Android Maps build passed locally	1 file	Original PR fix; simpler than candidate 2 but cleanup waits for remap/disconnect path

Cross-Pollination

Model	Round	New Ideas?	Details
maui-expert-reviewer	1	Yes	Proposed disposable active-pin subscription snapshot; failed build because marker assignment was not guaranteed
maui-expert-reviewer	2	Yes	Proposed active-pin snapshot/diff informed by candidate 1 failure; build passed

Exhausted: No
Selected Fix: Candidate #2 — It passed the available Android validation and is meaningfully different from the PR's fix by unsubscribing removed pins immediately when Map.Pins changes. It is not automatically recommended over the PR for merge because it is more complex and no automated leak regression test exists in this PR.

---

Report — Final Recommendation

Comparative Report — PR #35686

Candidates Compared

Rank	Candidate	Test Result	Assessment
1	pr	Gate skipped; targeted Android Maps build passed in prior context	Best candidate. Fixes the retained removed-pin leak with a small, targeted change while preserving the existing Android MapPins teardown/rebuild lifecycle.
1	pr-plus-reviewer	Same as pr	Tied with pr because the expert reviewer found no actionable code feedback; the sandbox-applied reviewer candidate is identical to the raw PR fix.
3	try-fix-2	PASS	Valid alternative and passed the targeted Android Maps build, but it is larger and changes subscription/handler lifecycle by retaining still-active pin handlers across marker rebuilds. That extra behavioral surface is not necessary to fix the leak.
4	try-fix-1	FAIL	Must rank below passing candidates. It failed Android build with nullable/unassigned marker errors after moving marker ownership into PinSubscription.

Analysis

The raw PR fix changes cleanup from iterating the current VirtualView.Pins collection to iterating all pins the handler actually subscribed to. That is the narrow root-cause fix: removed pins are no longer missed during DisconnectPins(), reference-equal tracking prevents duplicate subscriptions, and reference equality avoids incorrectly coalescing distinct value-equal pins.

pr-plus-reviewer did not diverge from pr. The expert reviewer returned LGTM with no actionable inline findings, so there was no sandbox code change to apply. Its only recommendation was test coverage if Android Maps infrastructure can support it.

try-fix-2 is the strongest non-PR candidate. It passed the available targeted Android Maps build and also uses reference-based tracking, but it replaces the PR's all-subscribed set with an active-pin snapshot/diff. That makes removed-pin cleanup more selective, but it also removes the existing MapPins call to DisconnectPins() and keeps handlers for still-active pins while markers are removed and recreated. Because the PR fix already cleans removed pins on the same remap path with less lifecycle churn and less code, the additional complexity is not justified.

try-fix-1 is not viable because it failed regression validation at build time. Per the ranking rule, a failing candidate must be below all passing/skipped-but-build-validated candidates.

Winner

Winner: pr

The submitted PR fix is the best candidate because it is the smallest change that addresses the leak, preserves existing handler lifecycle behavior, and received no actionable expert-review findings. pr-plus-reviewer is equivalent, but pr is selected because no reviewer changes were actually applied.

---

Future Action — review latest findings

No alternative fix was selected for this run. Review the session findings and CI results before merging.