Rapid change of selected tab results in crash on Windows

[sheiksyedm]

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

When users rapidly switch tabs in TabbedPage on Windows, concurrent navigation operations cause a crash:
Stack trace shows crash at line 299 (UpdateCurrentPageContent):

WinRT.Runtime.dll!WinRT.ExceptionHelpers.ThrowExceptionForHR(int hr) Microsoft.W
inUI.dll!Microsoft.UI.Xaml.Controls.ContentPresenter.Content.set(object value) M
icrosoft.Maui.Controls.dll!Microsoft.Maui.Controls.TabbedPage.UpdateCurrentPageC
ontent(WPage page) Line 299

The Problem Flow:
1. User rapidly clicks tabs (Tab 1 → Tab 2 → Tab 3)
2. First tab change calls NavigateToPage → NavigationFrame.NavigateToType()
3. Navigation starts, OnNavigated event will fire when complete
4. Before first navigation completes, second tab change triggers another NavigateToPage
5. Second navigation also calls NavigateToType() → starts another navigation
6. Both OnNavigated events fire and try to set presenter.Content
7. WinUI throws: ContentPresenter.Content cannot be modified during active navigation/layout operation

Additionally, setting SelectedItem in MapCurrentPage triggers SelectionChanged events that cause reentrant calls.

Solution

Implemented two guard flags to prevent race conditions:

1. _isNavigating flag - Prevents overlapping navigation operations
   - Set before calling NavigateToType()
   - Checked at start of NavigateToPage() - returns early if navigation in progress
   - Cleared in OnNavigated() finally block after content update completes

2. _isUpdatingSelection flag - Prevents reentrant selection events
   - Set before updating SelectedItem in MapCurrentPage
   - Checked at start of OnSelectedMenuItemChanged - returns early if updating programmatically
   - Cleared in finally block after update completes

This ensures:
- Only one navigation operation runs at a time
- Subsequent rapid clicks are queued/ignored until current navigation completes
- No reentrant selection change events
- Safe cleanup via try-finally blocks

Fixes #32824

[bronteq]

maybe these variables should be volatile?

volatile bool _isUpdatingSelection;
volatile bool _isNavigating;

[qjustfeelitp]

What is the reasoning behind using flags instead of some synchronization mechanism like a lock? I mean, if there is an operation that can not be run simultaneously, are the boolean flags sufficient? Or would it be better to use a Lock object and lock it? Or some kind of other synchronization like SemaphoreSlim?

[sheiksyedm]

@qjustfeelitp All operations run on the Windows UI thread:

 - OnSelectedMenuItemChanged - WinUI SelectionChanged event (UI thread)
 - NavigateToPage - Called from UI thread
 - OnNavigated - WinUI navigation event (UI thread)
 - MapCurrentPage - Handler update (UI thread)

WinUI guarantees these events are dispatched on the UI thread, so there's no concurrent multi-threaded access. The fix of adding flags prevents reentrant calls (the same thread calling again before completing).

Using synchronization primitives like lock or SemaphoreSlim would be overkill. Locking on the UI thread may cause deadlocks if any async operation awaits while holding the lock.

[sheiksyedm]

/azp run MAUI-UITests-public

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[sheiksyedm]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[Copilot]

Pull request overview

This PR addresses issue #32824 where rapidly switching tabs in a TabbedPage on Windows causes a crash. The solution introduces two guard flags (_isNavigating and _isUpdatingSelection) to prevent concurrent navigation operations and reentrant selection change events.

Key Changes

• Added _isNavigating flag to prevent overlapping navigation operations in NavigateToPage
• Added _isUpdatingSelection flag to prevent reentrant SelectionChanged events in MapCurrentPage
• Wrapped flag resets in try-finally blocks to ensure cleanup even on exceptions

[Copilot]

This fix lacks test coverage for the rapid tab switching scenario that caused the original crash. Given that:

1. The repository has comprehensive test coverage for TabbedPage (as seen in TabbedPageTests.Windows.cs)
2. This is a critical bug fix for a crash scenario

A test should be added to verify that rapid successive tab changes don't cause a crash. The test should simulate clicking multiple tabs in quick succession before navigation completes, similar to:

[Fact(DisplayName = "Rapid Tab Switching Does Not Crash")]
public async Task RapidTabSwitchingDoesNotCrash()
{
    SetupBuilder();
    var tabbedPage = CreateBasicTabbedPage();
    tabbedPage.Children.Add(new ContentPage { Title = "Page 2" });
    tabbedPage.Children.Add(new ContentPage { Title = "Page 3" });

    await CreateHandlerAndAddToWindow<TabbedViewHandler>(tabbedPage, async handler =>
    {
        var navView = GetMauiNavigationView(handler.MauiContext);
        var items = (navView.MenuItemsSource as IEnumerable<NavigationViewItemViewModel>).ToList();
        
        // Rapidly switch tabs without waiting
        navView.SelectedItem = items[1];
        navView.SelectedItem = items[2];
        navView.SelectedItem = items[0];
        
        // Wait for navigation to complete and verify no crash
        await Task.Delay(500);
        Assert.Equal(tabbedPage.Children[0], tabbedPage.CurrentPage);
    });
}

This test should be added to src/Controls/tests/DeviceTests/Elements/TabbedPage/TabbedPageTests.Windows.cs.

[Copilot]

The MapCurrentPage method doesn't break out of the loop after finding and setting the selected item. While functionally this works because only one item should match view.CurrentPage, it unnecessarily iterates through all remaining items after a match is found.

Suggested optimization:

foreach (var item in items)
{
    if (item.Data == view.CurrentPage)
    {
        view._navigationView.SelectedItem = item;
        break;  // Exit loop once we've found and set the item
    }
}

Suggested change

	view._navigationView.SelectedItem = item;
	view._navigationView.SelectedItem = item;
	break;

[Copilot]

[nitpick] The try-finally block in OnNavigated ensures _isNavigating is always reset, which is good. However, if UpdateCurrentPageContent throws an exception, the navigation will be marked as complete (_isNavigating = false) even though the content update failed.

This could lead to an inconsistent state where:

1. Navigation is marked complete
2. But the UI doesn't show the correct content
3. Future navigation attempts proceed, potentially compounding the error

Consider whether exceptions from UpdateCurrentPageContent should be caught, logged, and handled explicitly rather than allowing them to propagate while still clearing the flag. At minimum, the existing behavior should be documented with a comment explaining the trade-off.

[Copilot]

The use of volatile here is misleading. The comment states these flags are for preventing reentrant calls within the UI thread, not for multi-threaded access. However, volatile is a threading construct that ensures visibility across threads.

Since this is single-threaded UI code with async operations, volatile provides no benefit and creates confusion. The flags should simply be bool without the volatile modifier. The comment should also be simplified to remove the confusing threading reference.

Suggested change

	// Volatile ensures visibility of flag changes across async operations
	// These flags prevent reentrant calls within the UI thread (not multi-threaded access)
	volatile bool _isUpdatingSelection;
	volatile bool _isNavigating;
	// Flags to prevent reentrant calls within the UI thread
	bool _isUpdatingSelection;
	bool _isNavigating;

[Copilot]

There's a critical bug in the navigation flow. The CurrentPage is set at line 269 before checking whether navigation is already in progress and before setting _isNavigating = true at line 271.

This means if a second rapid click occurs:

1. First click: Sets CurrentPage = page1, then _isNavigating = true
2. Second click (during first navigation): Line 265 checks _isNavigating and returns early
3. But the second click's CurrentPage assignment at line 269 never happened

This leaves CurrentPage and the UI out of sync. The assignment CurrentPage = page; (line 269) should only occur after the _isNavigating check passes, or the entire method should be restructured to queue the navigation request.

Suggested fix:

void NavigateToPage(Page page)
{
    if (_isNavigating)
        return;

    _isNavigating = true;
    CurrentPage = page;  // Move after flag is set and check passes
    
    FrameNavigationOptions navOptions = new FrameNavigationOptions();
    navOptions.IsNavigationStackEnabled = false;
    NavigationFrame.NavigateToType(typeof(WPage), null, navOptions);
}

Suggested change

	CurrentPage = page;
	navOptions.IsNavigationStackEnabled = false;
	_isNavigating = true;
	navOptions.IsNavigationStackEnabled = false;
	_isNavigating = true;
	CurrentPage = page;

[sheiksyedm]

Alternative fix: #33113