Skip to content
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.

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

Fix for Application Crash When Updating ToolbarItem Icon and Navigating Between Shell Pages #26126

Merged
merged 14 commits into from
Jan 8, 2025

Conversation

SuthiYuvaraj
Copy link
Contributor

Root Cause

  1. Root Cause for the Exception: The crash occurred because the OnPropertyChanged event for the ToolbarItem.IconImageSource was triggered before the page's handler was created during the second navigation. Initially, on the first navigation, this event fired after the page handler was initialized, allowing the UpdateIconAndStyle method to access the MauiContext through item.FindMauiContext(). However, after updating the ToolbarItem.IconImageSource multiple times, the event sequence changed. On subsequent navigations, the OnPropertyChanged event fired before the page handler was created, leaving the ToolbarItem without a valid parent handler. As a result, attempts to access item.FindMauiContext() failed, leading to a null reference exception and causing the app to crash.
  2. Cause for the Error : IIOImageSource:439: *** ERROR: can't open '/path/to/image/sync' (fileExists == false)
    The sample code used an .svg file for the toolbar icon without the correct extension or appropriate handling. Per .NET MAUI guidelines, SVG files are automatically converted to PNG files and must be referenced with a .png extension in the project. Using the incorrect extension or without extension caused the app to fail with error.

Description of Change

To address this issue, a condition was added to the UpdateIconAndStyle method to ensure the ToolbarItem's parent handler exists before attempting to access item.FindMauiContext(). This additional check prevents the method from executing when the required resources are not yet initialized, avoiding any null reference exceptions. This change ensures that updates to ToolbarItem.IconImageSource are safely handled during navigation, maintaining stability and preventing crashes caused by uninitialized platform dependencies.

Issues Fixed

Fixes #25534

Tested the behaviour in the following platforms

  • Android
  • Windows
  • iOS
  • Mac

Output Video

Before Issue Fix After Issue Fix
withoutfix.mov
withfix.mov

Fix for toolbar issue
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Nov 26, 2024
@SuthiYuvaraj SuthiYuvaraj marked this pull request as ready for review November 26, 2024 12:40
@SuthiYuvaraj SuthiYuvaraj requested a review from a team as a code owner November 26, 2024 12:40
@jsuarezruiz
Copy link
Contributor

jsuarezruiz commented Nov 26, 2024

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@praveenkumarkarunanithi
Copy link
Contributor

/rebase

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@sheiksyedm sheiksyedm added the partner/syncfusion Issues / PR's with Syncfusion collaboration label Dec 6, 2024
@SuthiYuvaraj SuthiYuvaraj changed the title 25534- Fix for Application crash on setting icon on toolbar item 25534- Fix for Application Crash When Updating ToolbarItem Icon and Navigating Between Shell Pages Dec 18, 2024
@SuthiYuvaraj SuthiYuvaraj changed the title 25534- Fix for Application Crash When Updating ToolbarItem Icon and Navigating Between Shell Pages Fix for Application Crash When Updating ToolbarItem Icon and Navigating Between Shell Pages Dec 18, 2024
@@ -98,7 +98,7 @@ void OnPropertyChanged(object sender, PropertyChangedEventArgs e)

void UpdateIconAndStyle(ToolbarItem item)
{
if (item?.IconImageSource == null)
if (item?.IconImageSource == null || item.Parent?.Handler is null)
Copy link
Member

Choose a reason for hiding this comment

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

Does it make sense to set the Image to null if the Parent Handler is null?

Maybe the check should be added to the else? Or the method should just exit out of MauiContext is null?

var mc = item.FindMauiContext()
if (mc is null) return;

Copy link
Contributor

Choose a reason for hiding this comment

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

@PureWeen Thank you for the feedback. I've implemented the changes as suggested, focusing on proper MauiContext handling. The method now checks for a valid context before proceeding with image loading, addressing the concerns about potential null references. Please review and let us know if any further changes are required.

@PureWeen
Copy link
Member

PureWeen commented Jan 7, 2025

/azp run

Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@rmarinho rmarinho enabled auto-merge (squash) January 8, 2025 11:10
@PureWeen
Copy link
Member

PureWeen commented Jan 8, 2025

  • failing tests unrelated

@PureWeen PureWeen disabled auto-merge January 8, 2025 20:54
@PureWeen PureWeen merged commit f64d7a3 into dotnet:main Jan 8, 2025
103 of 105 checks passed
@PureWeen PureWeen added this to the .NET 9 SR3 milestone Jan 8, 2025
@github-actions github-actions bot locked and limited conversation to collaborators Feb 8, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-controls-toolbar ToolBar community ✨ Community Contribution partner/syncfusion Issues / PR's with Syncfusion collaboration platform/iOS 🍎 platform/macOS 🍏 macOS / Mac Catalyst
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Application crash on setting icon on toolbar item
6 participants