[Android] Fix for ToolbarItem retaining the icon from the previous page on Android when using NavigationPage.

[BagavathiPerumal]

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 Description:

On Android, when navigating from one page to another (Navigation.PushAsync(...)), the icons from the previous page may persist on the new page. This doesn’t happen all the time (in either debug or release mode); it occurs randomly.

Root Cause:

The issue occurs because of a race condition between asynchronous icon loading and navigation in MAUI's Android toolbar implementation. When navigating from a page with more toolbar items to a page with fewer items (e.g., 3 items → 2 items), the Android MenuItems get reused for performance optimization. However, the icon loading for FontImageSource uses async callbacks that can complete after navigation has already happened.

The async callbacks from the previous page's toolbar items (like "Delete" and "Edit" icons) can fire after the new page has already set up its toolbar items (like "Cancel" and "Validate" icons), causing the old icons to overwrite the new ones. The race condition is timing-dependent and more likely to occur when there are more toolbar items being reused, which explains why the issue manifests primarily when navigating from pages with higher item counts to pages with lower item counts.

Fix Description:

The fix involves implementing a tracking mechanism that associates each MenuItem with its current ToolbarItem to prevent stale async callbacks from overwriting correct icons. A dictionary maps MenuItem IDs to weak references of their currently assigned ToolbarItem. When an async icon loading callback fires, the code validates that the MenuItem is still associated with the ToolbarItem that initiated the icon loading operation. If a different ToolbarItem is now associated with that MenuItem (indicating the MenuItem was reused for a different toolbar item after navigation), the callback is aborted and logged as a detected race condition.

This prevents async callbacks from previous pages from contaminating the icons on the current page. The mapping is cleaned up when MenuItems are disposed to prevent memory leaks. This solution maintains the performance benefits of MenuItem reuse while eliminating the race condition that caused icon persistence across page navigation.

Note: This issue occurs in a random manner, so it's not possible to add a test case for it.

Issues Fixed

Fixes #31727

Tested the behaviour in the following platforms

• Android
• Windows
• iOS
• Mac

Output Screenshot

Before Issue Fix	After Issue Fix
Beforefix-AndroidToolbarItem.mov	Afterfix-AndroidToolbarItem.mov

[jsuarezruiz]

Could include a related test?

[BagavathiPerumal]

Could include a related test?

@jsuarezruiz, This issue occurs in a random manner, so it's not possible to add a test case for it.

[Copilot]

Pull Request Overview

This PR adds race condition prevention for toolbar item icon loading during navigation on Android. The fix prevents stale async icon loading callbacks from updating the wrong toolbar items when rapid navigation changes occur.

Key changes:

• Introduces a static dictionary tracking MenuItem ID to ToolbarItem associations using weak references
• Adds cleanup logic for disposed MenuItems to remove their entries from the tracking map
• Implements validation in UpdateMenuItemIcon to verify that async icon callbacks are still targeting the correct ToolbarItem

[Copilot]

The race condition check has a gap: if the MenuItem ID is reused for a new ToolbarItem before the async callback executes, but the WeakReference has been garbage collected, the check will pass even though it should abort. Add an additional check: if the menuItem.ItemId exists in the map but the WeakReference target is null (collected), the callback should return early rather than proceeding with the update.

Suggested change

	// Additional check: if the menuItem.ItemId exists in the map but the WeakReference target is null (collected), abort
	if (_menuItemToolbarItemMap.TryGetValue(menuItem.ItemId, out var wr) && !wr.TryGetTarget(out _))
	{
	return;
	}

[Copilot]

The static dictionary _menuItemToolbarItemMap is accessed from multiple threads (UI thread for updates, background threads for async icon loading) without synchronization. This creates a potential race condition where dictionary operations could throw or corrupt data. Consider using ConcurrentDictionary<int, WeakReference> instead to ensure thread-safe access.

[Copilot]

Cleanup operations occur in a loop that iterates backwards through previousMenuItems, resulting in multiple individual dictionary removals. If many MenuItems are being disposed (e.g., during navigation), consider collecting the IDs first and then removing them in a batch, or at minimum, verify the operation succeeded to handle cases where the ID might not exist in the map.

Suggested change

	_menuItemToolbarItemMap.Remove(previousMenuItem.ItemId);
	if (!_menuItemToolbarItemMap.Remove(previousMenuItem.ItemId))
	{
	// Optionally log or handle the case where the mapping does not exist
	// e.g., Debug.WriteLine($"Warning: MenuItem mapping for ItemId {previousMenuItem.ItemId} not found during cleanup.");
	}

[github-actions]

🚀 Dogfood this PR with:

⚠️ WARNING: Do not do this without first carefully reviewing the code of this PR to satisfy yourself it is safe.

curl -fsSL https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.sh | bash -s -- 32311

Or

• Run remotely in PowerShell:

iex "& { $(irm https://raw.githubusercontent.com/dotnet/maui/main/eng/scripts/get-maui-pr.ps1) } 32311"

[sheiksyedm]

/rebase

[PureWeen]

/rebase

[sheiksyedm]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[rmarinho]

🤖 AI Summary

📊 Expand Full Review

---

🔍 Pre-Flight — Context & Validation

---

📝 Review Session — fix-31727-Made changes to fix race condition logic. · fccff41

---

Issue: #31727 - Toolbaritem keeps the icon of the previous page on Android, using NavigationPage (not shell)
PR: #32311
Platforms Affected: Android only
Files Changed: 1 implementation file, 0 test files

Prior Agent Review Detected

A prior agent review was found in PR comments (posted 2026-02-16 by rmarinho).

• Prior Gate: ❌ FAILED (no tests)
• Prior Report: ⚠️ REQUEST CHANGES (cited thread safety, race detection gap, and missing tests)
• Prior PR-FINALIZE: ✅ APPROVE (contradicted the Report, found code quality excellent)

This review re-examines those concerns independently.

Summary

On Android, when navigating between pages using NavigationPage, toolbar icons from the previous page randomly persist on the new page. Root cause: race condition between async icon loading (FontImageSource) and navigation. When navigating from a page with more toolbar items to one with fewer, Android MenuItems are reused. Async icon loading callbacks from the previous page can fire after the new page's toolbar is set up, causing old icons to overwrite new ones.

PR's Fix Approach

Adds a static Dictionary<int, WeakReference<ToolbarItem>> (_menuItemToolbarItemMap) to track MenuItem ID → ToolbarItem associations. Before applying an async icon callback, validates the MenuItem is still associated with the ToolbarItem that initiated the loading. Cleans up mappings on MenuItem disposal.

Files Modified: src/Controls/src/Core/Platform/Android/Extensions/ToolbarExtensions.cs (+26/-2)

Key Findings

1. Thread Safety Re-Analysis: LoadImage is called from toolbar update methods which run on the main/UI thread. The LoadImageResult async method uses await task without ConfigureAwait(false), so continuations (including the callback) execute on the captured SynchronizationContext (main thread). The static dictionary is therefore only accessed from the main thread — thread safety concern is NOT valid.

2. Race Detection Gap Re-Analysis: The copilot reviewer claimed the GC'd WeakReference case would "pass incorrectly." However, examining the actual code:

   if (!weakRef.TryGetTarget(out var currentToolbarItem) || !ReferenceEquals(...))
   {
       return;
   }

   If GC'd, TryGetTarget returns false, making !false = true, so it returns early. The code CORRECTLY handles GC'd WeakReferences. This concern is NOT valid.

3. Static Dictionary Scope: The dictionary is shared across all ToolbarExtensions usage. In multi-window Android scenarios, MenuItem IDs from different NavigationPage instances could potentially collide in the map. This is a minor theoretical concern for advanced multi-window use cases.

4. No Tests: Author stated race conditions are not testable. A basic UI test validating navigation between pages with different toolbar item counts could serve as a regression test, even if it can't reliably trigger the race.

Reviewer Discussion

Concern	Source	Assessment
Thread safety (ConcurrentDictionary needed)	copilot-pull-request-reviewer	❌ Not valid - UI thread only
Race detection gap (GC'd WeakRef)	copilot-pull-request-reviewer	❌ Not valid - code handles it correctly
Cleanup efficiency (batch operations)	copilot-pull-request-reviewer	💡 Minor optimization, not required
Missing tests	jsuarezruiz, prior agent	⚠️ Valid concern

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
PR	PR #32311	Track MenuItem→ToolbarItem with WeakReference dictionary, validate before applying async icon	⏳ PENDING (Gate)	ToolbarExtensions.cs (+26)	Original PR

Platform Selection for Gate

Selected Platform: Android
Rationale: Issue is Android-specific; PR modifies Android-only file (Platform/Android/Extensions/ToolbarExtensions.cs); current host is macOS which supports Android testing.

---

🚦 Gate — Test Verification

---

📝 Review Session — fix-31727-Made changes to fix race condition logic. · fccff41

---

Result: ❌ FAILED
Platform: android
Reason: No tests exist for this PR

Gate Check

Searched for tests matching issue #31727 and PR #32311:

• src/Controls/tests/TestCases.HostApp/Issues/Issue31727.* — Not found
• src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue31727.* — Not found
• No test files referencing issue number 31727

Context

This PR addresses a random race condition in Android toolbar icon loading during NavigationPage navigation. The PR author stated: "This issue occurs in a random manner, so it's not possible to add a test case for it."

Why a Test Is Still Possible

Even though the race condition itself is timing-dependent and hard to trigger reliably:

1. Navigation scenario test: A UI test that navigates from PageA (3 toolbar items) to PageB (2 toolbar items) and verifies PageB shows the correct icons serves as a regression test.
2. Structural test: Verifies the WeakReference dictionary cleanup mechanism works correctly.
3. Device test: A toolbar navigation test that verifies toolbar items update correctly when navigating.

Existing related device tests are in src/Controls/tests/DeviceTests/Elements/Toolbar/ToolbarTests.cs, but none cover the specific navigation-with-different-item-count scenario.

Recommendation

The PR author should add at minimum a UI test that:

• Navigates between two pages with different toolbar item counts
• Verifies the destination page shows the correct toolbar items after navigation
• Can serve as a regression test for this race condition

The write-ui-tests skill can help generate appropriate tests.

Status

Gate ❌ FAILED — proceeding to Report phase per autonomous execution rules.

---

🔧 Fix — Analysis & Comparison

---

📝 Review Session — fix-31727-Made changes to fix race condition logic. · fccff41

---

Fix Candidates

#	Source	Approach	Test Result	Files Changed	Notes
PR	PR #32311	Track MenuItem→ToolbarItem mapping with WeakReference dictionary	⏳ PENDING (Gate)	ToolbarExtensions.cs (+26)	Original PR

Exhausted: N/A — Skipped due to Gate ❌ FAILED
Selected Fix: PR's fix — Gate failed, Fix phase skipped. Code analysis confirms PR's implementation is correct.

Reason for Skip: Gate failed because no tests exist for this issue. Without tests, try-fix cannot run its two-pass verification (FAIL without fix → PASS with fix). Per autonomous execution rules, skipping Fix phase and proceeding to Report.

Note on Code Quality: Independent code analysis (see Report) found the PR's implementation to be correct and well-reasoned, despite Gate failure.

---

📋 Report — Final Recommendation

---

📝 Review Session — fix-31727-Made changes to fix race condition logic. · fccff41

---

⚠️ Final Recommendation: REQUEST CHANGES

Summary

PR #32311 fixes a real and verified race condition on Android where toolbar icons from a previous page persist on the new page during NavigationPage navigation. The implementation is technically sound and correctly addresses the root cause. However, no tests exist for this issue, making it impossible to verify the fix works or prevent future regressions.

Root Cause Analysis

When navigating between pages on Android with different toolbar item counts (e.g., 3 → 2 items), Android reuses MenuItem objects for performance. The FontImageSource icon loading uses async callbacks via LoadImage → FireAndForget. These callbacks can complete after the new page's toolbar has been set up, causing old icons to overwrite the correct new ones. The timing dependency (async completion vs. navigation speed) makes this a random race condition.

Gate Result

❌ FAILED — No tests found for issue #31727 or PR #32311.

Searched: TestCases.HostApp/Issues/Issue31727.*, TestCases.Shared.Tests/Tests/Issues/Issue31727.* — both not found.

Fix Quality Assessment

Independent code review of the PR's implementation:

Aspect	Rating	Notes
Root Cause Understanding	✅ Excellent	Clear, accurate explanation of race condition
Solution Approach	✅ Correct	WeakReference tracking is appropriate for this scenario
Thread Safety	✅ Acceptable	LoadImage callbacks run on main thread (no ConfigureAwait(false))
Race Detection Logic	✅ Correct	!weakRef.TryGetTarget(...) || !ReferenceEquals(...) correctly handles both GC'd and reused cases
Memory Management	✅ Good	Cleanup in both disposal paths prevents leaks
Code Quality	✅ Good	Clear comments explain WHY, not just WHAT

Analysis of Prior Review Concerns

The prior agent review (2026-02-16) raised two technical concerns. Re-analysis shows both are not valid:

1. Thread Safety (prior: ConcurrentDictionary required)

"The static dictionary _menuItemToolbarItemMap is accessed from multiple threads without synchronization."

Re-analysis: UpdateMenuItemIcon is called from UpdateMenuItem → UpdateMenuItems, which are UI thread operations. The LoadImage method uses FireAndForget(LoadImageResult), and LoadImageResult awaits the task without ConfigureAwait(false), so continuations execute on the captured main-thread SynchronizationContext. The dictionary is only accessed from the UI thread. No change needed.

2. Race Detection Gap (prior: GC'd WeakRef passes incorrectly)

"If target was garbage collected, the check will pass even though it should abort."

Re-analysis: The actual code is:

if (!weakRef.TryGetTarget(out var currentToolbarItem) || !ReferenceEquals(currentToolbarItem, toolBarItem))
{
    return;
}

When GC'd: TryGetTarget returns false → !false = true → returns early (aborts). The code already correctly handles this case. No change needed.

Remaining Issue: Missing Tests

This is the primary blocker. Even for random race conditions, a test can be written that:

1. Creates a NavigationPage with PageA (3 toolbar items with FontImageSource icons)
2. Pushes PageB (2 toolbar items with different icons)
3. Verifies PageB shows the correct toolbar items (not PageA's icons)
4. Pops back and pushes PageB multiple times to stress-test

While this won't reliably trigger the race condition timing, it:

• Documents the expected behavior
• Provides a regression test for the tracking mechanism
• Verifies correct toolbar items are shown after navigation
• Can be run with TestCases.HostApp + TestCases.Shared.Tests (UI test)

Suggested test approach: Issue31727.cs in TestCases.HostApp/Issues/ + matching test in TestCases.Shared.Tests/Tests/Issues/. The write-ui-tests skill can help generate this.

What Needs to Change

Required:

1. ✅ Add a UI test (IssueXXXXX.cs) that navigates between pages with different toolbar item counts and verifies icons are correct

Optional (nice-to-have):
2. Consider whether the static dictionary scope is safe in multi-window Android scenarios (MenuItem IDs could theoretically collide across different NavigationPage instances)

Platform Coverage

Affected: Android only
PR Scope: Correctly scoped to Platform/Android/Extensions/ToolbarExtensions.cs

Status

⚠️ REQUEST CHANGES — The fix implementation is technically correct and ready to merge from a code quality standpoint, but the missing test coverage is a legitimate concern per project requirements. The PR author should add at least a basic UI regression test using the write-ui-tests skill.

---

📋 Expand PR Finalization Review

---

Title: ✅ Good

Current: [Android] Fix for ToolbarItem retaining the icon from the previous page on Android when using NavigationPage.

Description: ✅ Good

• Ends with a period (.) — commit message headlines should not end with punctuation
• "on Android" is redundant — [Android] prefix already conveys this
• Slightly verbose — could be tighter

✨ Suggested PR Description

[!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!

Root Cause

On Android, ToolbarExtensions.UpdateMenuItems reuses existing MenuItem objects for performance when the new page has fewer toolbar items than the previous page. Icon loading via FontImageSource / LoadImage is async, so a loading callback from the previous page's toolbar can fire after the current page has already reassigned those MenuItem objects to its own ToolbarItem instances — overwriting the correct icons with stale ones. The race is timing-dependent and occurs more frequently when navigating from a page with more items to one with fewer (e.g., 3 → 2).

Description of Change

Adds a lightweight tracking mechanism in ToolbarExtensions to associate each MenuItem (by its ID) with its currently-assigned ToolbarItem via a WeakReference<ToolbarItem>. When an async icon-loading callback fires inside UpdateMenuItemIcon, it checks whether the MenuItem is still associated with the same ToolbarItem that initiated the load. If the association has changed (i.e., the MenuItem was reused for a different item after navigation) or the ToolbarItem was garbage-collected, the callback returns early without updating the icon.

Cleanup of map entries happens in both disposal paths in UpdateMenuItems:

1. When Android silently removes menu items after activity switches
2. When excess items are removed after the new page's items are applied

This preserves the existing MenuItem reuse performance optimization while preventing stale callbacks from contaminating the current page's icons.

Note: The race condition is non-deterministic, so it is not possible to add a deterministic automated test for it.

What NOT to Do (for future agents)

• ❌ Don't remove MenuItem reuse — reusing MenuItem objects is an intentional performance optimization in UpdateMenuItems; any fix must work alongside it, not against it.
• ❌ Don't use strong references in the map — using strong references to ToolbarItem in the dictionary would create memory leaks if map entries are not cleaned up promptly; use WeakReference<ToolbarItem>.
• ❌ Don't check the ID alone — simply checking whether the MenuItem.ItemId is in the map is insufficient; you must also verify the tracked ToolbarItem is reference-equal to the one that initiated the load.

Issues Fixed

Fixes #31727

Platforms Tested

• Android
• Windows
• iOS
• Mac

Code Review: ⚠️ Issues Found

Code Review — PR #32311

File: src/Controls/src/Core/Platform/Android/Extensions/ToolbarExtensions.cs

---

🔴 Critical Issues

Thread Safety: Static Dictionary Is Not Thread-Safe

Location: Line 30 — _menuItemToolbarItemMap declaration, and all read/write sites

Problem:
_menuItemToolbarItemMap is a static readonly Dictionary<int, WeakReference<ToolbarItem>>. It is written on the UI thread (UpdateMenuItem, UpdateMenuItems) but read (and potentially written via Remove) from within the LoadImage callback in UpdateMenuItemIcon. MAUI's LoadImage callbacks are not guaranteed to be dispatched on the UI thread in all implementations (they depend on the image source type and loader). If a callback fires on a background thread concurrently with a UI-thread dictionary modification, this can throw InvalidOperationException or silently corrupt the dictionary state.

Current code:

static readonly Dictionary<int, WeakReference<ToolbarItem>> _menuItemToolbarItemMap = new();

Recommendation:
Use ConcurrentDictionary<int, WeakReference<ToolbarItem>> to make all operations thread-safe without requiring explicit locking:

static readonly ConcurrentDictionary<int, WeakReference<ToolbarItem>> _menuItemToolbarItemMap = new();

All existing call sites ([] assignment, Remove, TryGetValue) have direct equivalents on ConcurrentDictionary with no other changes needed.

---

🟡 Suggestions

1. Missing Guard When Map Entry Is Absent

Location: UpdateMenuItemIcon, starting at the TryGetValue check (~line 385)

Problem:
The current guard only fires when the menuItem.ItemId IS present in the map:

if (_menuItemToolbarItemMap.TryGetValue(menuItem.ItemId, out var weakRef))
{
    if (!weakRef.TryGetTarget(out var currentToolbarItem) || !ReferenceEquals(currentToolbarItem, toolBarItem))
    {
        return;
    }
}
// Falls through to update the icon if ID is NOT in the map

If the entry was already removed (because the MenuItem was disposed and cleaned up) before the callback fires, TryGetValue returns false, the guard is skipped, and the callback proceeds to call menuItem.SetIcon(...) on a MenuItem that may be stale or disposed. The menuItem.IsAlive() check above this block will catch truly disposed items, but there is a window between cleanup and disposal.

Recommendation:
Consider returning early when the ID is not found in the map (i.e., treat an absent entry as a signal that the item was already cleaned up):

if (!_menuItemToolbarItemMap.TryGetValue(menuItem.ItemId, out var weakRef)
    || !weakRef.TryGetTarget(out var currentToolbarItem)
    || !ReferenceEquals(currentToolbarItem, toolBarItem))
{
    return;
}

This is a safe change only if UpdateMenuItemIcon is always called after the map entry is set in UpdateMenuItem. Looking at the code flow — the map entry is set on line ~350, and UpdateMenuItemIcon is called immediately after on line ~354 — so this ordering is guaranteed for the default path.

---

2. Custom updateMenuItemIcon Callback Bypasses Race Condition Protection

Location: UpdateMenuItem — the updateMenuItemIcon branch (~line 354)

Problem:
The race condition guard is only present in the default UpdateMenuItemIcon path. If a caller provides a custom updateMenuItemIcon action, that action may also perform async icon loading without the guard, re-introducing the same race condition for callers that override the default behavior.

Recommendation:
Document this limitation at the updateMenuItemIcon parameter or consider wrapping custom callbacks so they also validate against the map. At minimum, add a code comment:

// NOTE: Custom updateMenuItemIcon callbacks are responsible for their own
// race condition handling. The _menuItemToolbarItemMap guard only applies
// to the default UpdateMenuItemIcon path.
if (updateMenuItemIcon != null)
    updateMenuItemIcon(context, menuitem, item);
else
    UpdateMenuItemIcon(mauiContext, menuitem, item, tintColor);

---

✅ Looks Good

• WeakReference logic is correct: The check !weakRef.TryGetTarget(out var currentToolbarItem) || !ReferenceEquals(currentToolbarItem, toolBarItem) correctly handles both the GC'd case (returns early) and the reused-for-different-item case (returns early). The Copilot reviewer comment suggesting the GC'd case isn't handled is incorrect — it is handled by the !weakRef.TryGetTarget(...) branch of the ||.
• Both cleanup paths are covered: Map entries are removed both when Android silently deletes menu items (the FindItem == null loop) and when excess items are trimmed (the while loop). No leaks introduced.
• menuitem.IsAlive() guard is preserved: The existing alive check remains in place as a first line of defense before the map check.
• WeakReference prevents memory leaks: Choosing WeakReference<ToolbarItem> over a strong reference is correct — it allows ToolbarItem to be GC'd naturally without being kept alive by the map.
• Map entry is set before icon loading begins: In UpdateMenuItem, _menuItemToolbarItemMap[menuitem.ItemId] is assigned before UpdateMenuItemIcon is called, so the guard in the callback will always find the entry for fresh assignments.

---

[kubaflo]

Could you please add a test?

[BagavathiPerumal]

Could you please add a test?

@kubaflo, this issue occurs randomly, so it is not possible to add a reliable test case. I also tried using the UI test agent; however, the scenario could not be reproduced consistently, and the resulting test was not reliable.

[BagavathiPerumal]

🤖 AI Summary

📊 Expand Full Review
🔍 Pre-Flight — Context & Validation
📝 Review Session — fix-31727-Made changes to fix race condition logic. · fccff41
Issue: #31727 - Toolbaritem keeps the icon of the previous page on Android, using NavigationPage (not shell) PR: #32311 Platforms Affected: Android only Files Changed: 1 implementation file, 0 test files

Prior Agent Review Detected

A prior agent review was found in PR comments (posted 2026-02-16 by rmarinho).

• Prior Gate: ❌ FAILED (no tests)
• Prior Report: ⚠️ REQUEST CHANGES (cited thread safety, race detection gap, and missing tests)
• Prior PR-FINALIZE: ✅ APPROVE (contradicted the Report, found code quality excellent)

This review re-examines those concerns independently.

Summary

On Android, when navigating between pages using NavigationPage, toolbar icons from the previous page randomly persist on the new page. Root cause: race condition between async icon loading (FontImageSource) and navigation. When navigating from a page with more toolbar items to one with fewer, Android MenuItems are reused. Async icon loading callbacks from the previous page can fire after the new page's toolbar is set up, causing old icons to overwrite new ones.

PR's Fix Approach

Adds a static Dictionary<int, WeakReference<ToolbarItem>> (_menuItemToolbarItemMap) to track MenuItem ID → ToolbarItem associations. Before applying an async icon callback, validates the MenuItem is still associated with the ToolbarItem that initiated the loading. Cleans up mappings on MenuItem disposal.

Files Modified: src/Controls/src/Core/Platform/Android/Extensions/ToolbarExtensions.cs (+26/-2)

Key Findings

1. Thread Safety Re-Analysis: LoadImage is called from toolbar update methods which run on the main/UI thread. The LoadImageResult async method uses await task without ConfigureAwait(false), so continuations (including the callback) execute on the captured SynchronizationContext (main thread). The static dictionary is therefore only accessed from the main thread — thread safety concern is NOT valid.

2. Race Detection Gap Re-Analysis: The copilot reviewer claimed the GC'd WeakReference case would "pass incorrectly." However, examining the actual code:

   if (!weakRef.TryGetTarget(out var currentToolbarItem) || !ReferenceEquals(...))
   {
       return;
   }

   If GC'd, TryGetTarget returns false, making !false = true, so it returns early. The code CORRECTLY handles GC'd WeakReferences. This concern is NOT valid.

3. Static Dictionary Scope: The dictionary is shared across all ToolbarExtensions usage. In multi-window Android scenarios, MenuItem IDs from different NavigationPage instances could potentially collide in the map. This is a minor theoretical concern for advanced multi-window use cases.

4. No Tests: Author stated race conditions are not testable. A basic UI test validating navigation between pages with different toolbar item counts could serve as a regression test, even if it can't reliably trigger the race.

Reviewer Discussion

Concern Source Assessment
Thread safety (ConcurrentDictionary needed) copilot-pull-request-reviewer ❌ Not valid - UI thread only
Race detection gap (GC'd WeakRef) copilot-pull-request-reviewer ❌ Not valid - code handles it correctly
Cleanup efficiency (batch operations) copilot-pull-request-reviewer 💡 Minor optimization, not required
Missing tests jsuarezruiz, prior agent ⚠️ Valid concern

Fix Candidates

Source Approach Test Result Files Changed Notes

PR PR #32311 Track MenuItem→ToolbarItem with WeakReference dictionary, validate before applying async icon ⏳ PENDING (Gate) ToolbarExtensions.cs (+26) Original PR

Platform Selection for Gate

Selected Platform: Android Rationale: Issue is Android-specific; PR modifies Android-only file (Platform/Android/Extensions/ToolbarExtensions.cs); current host is macOS which supports Android testing.

🚦 Gate — Test Verification
📝 Review Session — fix-31727-Made changes to fix race condition logic. · fccff41
Result: ❌ FAILED Platform: android Reason: No tests exist for this PR

Gate Check

Searched for tests matching issue #31727 and PR #32311:

• src/Controls/tests/TestCases.HostApp/Issues/Issue31727.* — Not found
• src/Controls/tests/TestCases.Shared.Tests/Tests/Issues/Issue31727.* — Not found
• No test files referencing issue number 31727

Context

This PR addresses a random race condition in Android toolbar icon loading during NavigationPage navigation. The PR author stated: "This issue occurs in a random manner, so it's not possible to add a test case for it."

Why a Test Is Still Possible

Even though the race condition itself is timing-dependent and hard to trigger reliably:

1. Navigation scenario test: A UI test that navigates from PageA (3 toolbar items) to PageB (2 toolbar items) and verifies PageB shows the correct icons serves as a regression test.
2. Structural test: Verifies the WeakReference dictionary cleanup mechanism works correctly.
3. Device test: A toolbar navigation test that verifies toolbar items update correctly when navigating.

Existing related device tests are in src/Controls/tests/DeviceTests/Elements/Toolbar/ToolbarTests.cs, but none cover the specific navigation-with-different-item-count scenario.

Recommendation

The PR author should add at minimum a UI test that:

• Navigates between two pages with different toolbar item counts
• Verifies the destination page shows the correct toolbar items after navigation
• Can serve as a regression test for this race condition

The write-ui-tests skill can help generate appropriate tests.

Status

Gate ❌ FAILED — proceeding to Report phase per autonomous execution rules.

🔧 Fix — Analysis & Comparison
📝 Review Session — fix-31727-Made changes to fix race condition logic. · fccff41

Fix Candidates

Source Approach Test Result Files Changed Notes

PR PR #32311 Track MenuItem→ToolbarItem mapping with WeakReference dictionary ⏳ PENDING (Gate) ToolbarExtensions.cs (+26) Original PR
Exhausted: N/A — Skipped due to Gate ❌ FAILED Selected Fix: PR's fix — Gate failed, Fix phase skipped. Code analysis confirms PR's implementation is correct.

Reason for Skip: Gate failed because no tests exist for this issue. Without tests, try-fix cannot run its two-pass verification (FAIL without fix → PASS with fix). Per autonomous execution rules, skipping Fix phase and proceeding to Report.

Note on Code Quality: Independent code analysis (see Report) found the PR's implementation to be correct and well-reasoned, despite Gate failure.

📋 Report — Final Recommendation
📝 Review Session — fix-31727-Made changes to fix race condition logic. · fccff41

⚠️ Final Recommendation: REQUEST CHANGES

Summary

PR #32311 fixes a real and verified race condition on Android where toolbar icons from a previous page persist on the new page during NavigationPage navigation. The implementation is technically sound and correctly addresses the root cause. However, no tests exist for this issue, making it impossible to verify the fix works or prevent future regressions.

Root Cause Analysis

When navigating between pages on Android with different toolbar item counts (e.g., 3 → 2 items), Android reuses MenuItem objects for performance. The FontImageSource icon loading uses async callbacks via LoadImage → FireAndForget. These callbacks can complete after the new page's toolbar has been set up, causing old icons to overwrite the correct new ones. The timing dependency (async completion vs. navigation speed) makes this a random race condition.

Gate Result

❌ FAILED — No tests found for issue #31727 or PR #32311.

Searched: TestCases.HostApp/Issues/Issue31727.*, TestCases.Shared.Tests/Tests/Issues/Issue31727.* — both not found.

Fix Quality Assessment

Independent code review of the PR's implementation:

Aspect Rating Notes
Root Cause Understanding ✅ Excellent Clear, accurate explanation of race condition
Solution Approach ✅ Correct WeakReference tracking is appropriate for this scenario
Thread Safety ✅ Acceptable LoadImage callbacks run on main thread (no ConfigureAwait(false))
Race Detection Logic ✅ Correct !weakRef.TryGetTarget(...) || !ReferenceEquals(...) correctly handles both GC'd and reused cases
Memory Management ✅ Good Cleanup in both disposal paths prevents leaks
Code Quality ✅ Good Clear comments explain WHY, not just WHAT

Analysis of Prior Review Concerns

The prior agent review (2026-02-16) raised two technical concerns. Re-analysis shows both are not valid:

1. Thread Safety (prior: ConcurrentDictionary required)

"The static dictionary _menuItemToolbarItemMap is accessed from multiple threads without synchronization."

Re-analysis: UpdateMenuItemIcon is called from UpdateMenuItem → UpdateMenuItems, which are UI thread operations. The LoadImage method uses FireAndForget(LoadImageResult), and LoadImageResult awaits the task without ConfigureAwait(false), so continuations execute on the captured main-thread SynchronizationContext. The dictionary is only accessed from the UI thread. No change needed.

2. Race Detection Gap (prior: GC'd WeakRef passes incorrectly)

"If target was garbage collected, the check will pass even though it should abort."

Re-analysis: The actual code is:

if (!weakRef.TryGetTarget(out var currentToolbarItem) || !ReferenceEquals(currentToolbarItem, toolBarItem))
{
    return;
}

When GC'd: TryGetTarget returns false → !false = true → returns early (aborts). The code already correctly handles this case. No change needed.

Remaining Issue: Missing Tests

This is the primary blocker. Even for random race conditions, a test can be written that:

1. Creates a NavigationPage with PageA (3 toolbar items with FontImageSource icons)
2. Pushes PageB (2 toolbar items with different icons)
3. Verifies PageB shows the correct toolbar items (not PageA's icons)
4. Pops back and pushes PageB multiple times to stress-test

While this won't reliably trigger the race condition timing, it:

• Documents the expected behavior
• Provides a regression test for the tracking mechanism
• Verifies correct toolbar items are shown after navigation
• Can be run with TestCases.HostApp + TestCases.Shared.Tests (UI test)

Suggested test approach: Issue31727.cs in TestCases.HostApp/Issues/ + matching test in TestCases.Shared.Tests/Tests/Issues/. The write-ui-tests skill can help generate this.

What Needs to Change

Required:

1. ✅ Add a UI test (IssueXXXXX.cs) that navigates between pages with different toolbar item counts and verifies icons are correct

Optional (nice-to-have): 2. Consider whether the static dictionary scope is safe in multi-window Android scenarios (MenuItem IDs could theoretically collide across different NavigationPage instances)

Platform Coverage

Affected: Android only PR Scope: Correctly scoped to Platform/Android/Extensions/ToolbarExtensions.cs

Status

⚠️ REQUEST CHANGES — The fix implementation is technically correct and ready to merge from a code quality standpoint, but the missing test coverage is a legitimate concern per project requirements. The PR author should add at least a basic UI regression test using the write-ui-tests skill.

📋 Expand PR Finalization Review

I’ve updated the changes based on the review comments. Specifically:

• Replaced the static Dictionary with a ConcurrentDictionary to ensure thread-safe access during async image callbacks.
• Updated all dictionary operations to their thread-safe equivalents (TryRemove, guarded TryGetValue).
• Strengthened guards to safely exit when entries are missing, stale, or already cleaned up.
• Added documentation clarifying that custom updateMenuItemIcon callbacks are responsible for their own race-condition handling.