Skip to content

Rapid change of selected tab results in crash on Windows #32865

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sheiksyedm wants to merge 2 commits into from
Closed

Rapid change of selected tab results in crash on Windows #32865

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
998a244
Rapid change of selected tab results in crash on Windows
sheiksyedm Nov 26, 2025
e3d6d0a
changed to volatile flags.
sheiksyedm Nov 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
36 changes: 31 additions & 5 deletions src/Controls/src/Core/TabbedPage/TabbedPage.Windows.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,10 @@ public partial class TabbedPage
NavigationRootManager? _navigationRootManager;
WFrame? _navigationFrame;
bool _connectedToHandler;
// 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;
Comment on lines +22 to +25
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
WFrame NavigationFrame => _navigationFrame ?? throw new ArgumentNullException(nameof(NavigationFrame));
IMauiContext MauiContext => this.Handler?.MauiContext ?? throw new InvalidOperationException("MauiContext cannot be null here");

Expand Down Expand Up @@ -246,6 +250,9 @@ void UpdateValuesWaitingForNavigationView()

void OnSelectedMenuItemChanged(NavigationView sender, NavigationViewSelectionChangedEventArgs args)
{
if (_isUpdatingSelection)
return;

if (args.SelectedItem is NavigationViewItemViewModel itemViewModel &&
itemViewModel.Data is Page page)
{
Expand All @@ -255,9 +262,13 @@ void OnSelectedMenuItemChanged(NavigationView sender, NavigationViewSelectionCha

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

FrameNavigationOptions navOptions = new FrameNavigationOptions();
CurrentPage = page;
navOptions.IsNavigationStackEnabled = false;
_isNavigating = true;
Comment on lines 269 to +271
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;

Copilot uses AI. Check for mistakes.
NavigationFrame.NavigateToType(typeof(WPage), null, navOptions);
}
Comment on lines 263 to 273
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.

Expand Down Expand Up @@ -301,8 +312,15 @@ void UpdateCurrentPageContent(WPage page)

void OnNavigated(object sender, UI.Xaml.Navigation.NavigationEventArgs e)
{
if (e.Content is WPage page)
UpdateCurrentPageContent(page);
try
{
if (e.Content is WPage page)
UpdateCurrentPageContent(page);
}
finally
{
_isNavigating = false;
}
}
Comment on lines 313 to 324
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[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 uses AI. Check for mistakes.

internal static void MapBarBackground(ITabbedViewHandler handler, TabbedPage view)
Expand Down Expand Up @@ -389,13 +407,21 @@ internal static void MapCurrentPage(ITabbedViewHandler handler, TabbedPage view)
{
if (view._navigationView?.MenuItemsSource is IList<NavigationViewItemViewModel> items)
{
foreach (var item in items)
try
{
if (item.Data == view.CurrentPage)
view._isUpdatingSelection = true;
foreach (var item in items)
{
view._navigationView.SelectedItem = item;
if (item.Data == view.CurrentPage)
{
view._navigationView.SelectedItem = item;
Copy link

Copilot AI Dec 2, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uses AI. Check for mistakes.
}
}
}
finally
{
view._isUpdatingSelection = false;
}
}
}
}
Expand Down
Loading