KEMBAR78
Fix: Null Reference Exception in ShellContentFragment.Destroy by postalservice14 · Pull Request #29713 · dotnet/maui · GitHub
Skip to content

Conversation

@postalservice14
Copy link
Contributor

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!

Description of Change

This PR fixes a null reference exception in the ShellContentFragment.Destroy method for Android. The issue occurred when attempting to call RemoveAppearanceObserver on a possibly null ShellContext. The fix adds a null-conditional operator to safely access ShellContext before invoking the method.

Issues Fixed

Fixes #29712

@Copilot Copilot AI review requested due to automatic review settings May 28, 2025 13:08
@postalservice14 postalservice14 requested a review from a team as a code owner May 28, 2025 13:08
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes a null reference exception occurring in the ShellContentFragment.Destroy method on Android by adding a null-conditional operator.

  • Updates the observer removal call to handle a potentially null _shellContext.
Comments suppressed due to low confidence (1)

src/Controls/src/Core/Compatibility/Handlers/Shell/Android/ShellContentFragment.cs:176

  • Using the null-conditional operator bypasses the observer removal if _shellContext is null. Please confirm that skipping the removal in such cases is an intentional design decision and does not lead to lingering observers.
((IShellController)_shellContext?.Shell)?.RemoveAppearanceObserver(this);

@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label May 28, 2025
@jsuarezruiz jsuarezruiz added area-controls-shell Shell Navigation, Routes, Tabs, Flyout platform/android labels May 28, 2025
@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

Copy link
Member

@rmarinho rmarinho left a comment

Choose a reason for hiding this comment

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

I think using "as" seems better in this case.

Can we add a test?

AnimationFinished?.Invoke(this, EventArgs.Empty);

((IShellController)_shellContext.Shell).RemoveAppearanceObserver(this);
((IShellController)_shellContext?.Shell)?.RemoveAppearanceObserver(this);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
((IShellController)_shellContext?.Shell)?.RemoveAppearanceObserver(this);
(_shellContext?.Shell as IShellController)?.RemoveAppearanceObserver(this);

Copy link
Contributor Author

@postalservice14 postalservice14 May 28, 2025

Choose a reason for hiding this comment

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

@rmarinho Great suggestion on the as.

Regarding the test: I'd love to but, I can't find any existing tests around this, do you have any suggestions on how to test this scenario?

Thanks!

@farneman
Copy link

farneman commented Jul 2, 2025

@rmarinho, @jsuarezruiz, @StephaneDelcroix or @tj-devel709 What can we do to help get this moving? Our app is seeing increased problems as a result of this root issue so we'd really like to get it solved.

It doesn't appear most of the checks on the PR ever reported in—how can those be restarted?

@nbuchert
Copy link

nbuchert commented Jul 8, 2025

We have experienced this issue as well in our first .NET 9/MAUI app release 10 days ago.
According to Google this is especially related to devices with 3-4 GB RAM.
Is there any expected release date for this fix, as we are having 650+ users that are experiencing this crash?

@MartyIX
Copy link
Contributor

MartyIX commented Jul 8, 2025

What can we do to help get this moving?

I believe that this is what needs to be done: #29713 (review)

@postalservice14
Copy link
Contributor Author

What can we do to help get this moving?

I believe that this is what needs to be done: #29713 (review)

@MartyIX I asked for help on the testing here: #29713 (comment).

I wasn't sure how to test it and couldn't find any existing tests around that behavior to modify. Would appreciate some help in that area if someone has the expertise.

@MartyIX
Copy link
Contributor

MartyIX commented Jul 8, 2025

@jsuarezruiz @rmarinho Would you help with the test please?

@jsuarezruiz jsuarezruiz self-assigned this Jul 18, 2025
@jsuarezruiz
Copy link
Contributor

@jsuarezruiz @rmarinho Would you help with the test please?

Yes, let me assigned to the PR to include tests.

@PureWeen
Copy link
Member

/rebase

@PureWeen PureWeen added this to the .NET 9 SR10 milestone Jul 18, 2025
@PureWeen PureWeen moved this from Todo to Ready To Review in MAUI SDK Ongoing Jul 18, 2025
@PureWeen
Copy link
Member

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz
Copy link
Contributor

jsuarezruiz commented Jul 21, 2025

@jsuarezruiz @rmarinho Would you help with the test please?

Yes, let me assigned to the PR to include tests.

@postalservice14 Could you try adding something like?

[Fact(DisplayName = "ShellContentFragment.Destroy handles null _shellContext gracefully")]
public async Task ShellContentFragmentDestroyHandlesNullShellContext()
{
	SetupBuilder();

	var shell = await CreateShellAsync(shell =>
	{
		shell.Items.Add(new TabBar()
		{
			Items =
			{
				new ShellContent()
				{
					Route = "Item1",
					Content = new ContentPage { Title = "Page 1" }
				},
				new ShellContent()
				{
					Route = "Item2", 
					Content = new ContentPage { Title = "Page 2" }
				},
			}
		});
	});

	await CreateHandlerAndAddToWindow<ShellHandler>(shell, async (handler) =>
	{
		await OnLoadedAsync(shell.CurrentPage);
		await OnNavigatedToAsync(shell.CurrentPage);

		// Navigate to trigger fragment creation
		await shell.GoToAsync("//Item2");
		await OnNavigatedToAsync(shell.CurrentPage);

		// Test normal destruction - should work without issues
		await shell.GoToAsync("//Item1");
		await OnNavigatedToAsync(shell.CurrentPage);

		// Test null context scenario
		var exception = Record.Exception(() => 
		{
			// Create fragment with null context - this should not throw
			Page page = new ContentPage();
			var fragment = new ShellContentFragment((IShellContext)null, page);
          
			// Dispose the fragment which calls Destroy internally
			// This validates the null-conditional operators in Destroy method
			fragment.Dispose();
		});

		// Verify no exception was thrown - validates (_shellContext?.Shell as IShellController)?.RemoveAppearanceObserver(this);
		Assert.Null(exception);
	});
}

Here:

@postalservice14
Copy link
Contributor Author

@dotnet-policy-service agree company="Ramsey Solutions"

mattleibow
mattleibow previously approved these changes Jul 22, 2025
@github-project-automation github-project-automation bot moved this from Ready To Review to Approved in MAUI SDK Ongoing Jul 22, 2025
@dotnet dotnet deleted a comment from azure-pipelines bot Jul 22, 2025
@dotnet dotnet deleted a comment from mattleibow Jul 22, 2025
@azure-pipelines
Copy link

Commenter does not have sufficient privileges for PR 29713 in repo dotnet/maui

@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@jsuarezruiz
Copy link
Contributor

Running the failed Android UITest again, but is not related with the changes.

@github-project-automation github-project-automation bot moved this from Approved to Changes Requested in MAUI SDK Ongoing Jul 24, 2025
@jsuarezruiz
Copy link
Contributor

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@PureWeen PureWeen moved this from Changes Requested to Approved in MAUI SDK Ongoing Aug 1, 2025
@PureWeen PureWeen merged commit c9d0abf into dotnet:main Aug 1, 2025
129 checks passed
@github-project-automation github-project-automation bot moved this from Approved to Done in MAUI SDK Ongoing Aug 1, 2025
@postalservice14 postalservice14 deleted the RPLUS-76242/fix-npe branch August 5, 2025 20:14
SuthiYuvaraj pushed a commit to SuthiYuvaraj/maui that referenced this pull request Aug 12, 2025
…#29713)

* Fix null reference exception when removing appearance observer in ShellContentFragment

* Change to use `as` syntax

* Add test to verify ShellContentFragment.Destroy handles null _shellContext gracefully

* Add test to ensure ShellContentFragment.Destroy handles null _shellContext gracefully

* Add safeguard in ShellContentFragment.Destroy to prevent multiple calls
rmarinho pushed a commit that referenced this pull request Aug 13, 2025
* Fix null reference exception when removing appearance observer in ShellContentFragment

* Change to use `as` syntax

* Add test to verify ShellContentFragment.Destroy handles null _shellContext gracefully

* Add test to ensure ShellContentFragment.Destroy handles null _shellContext gracefully

* Add safeguard in ShellContentFragment.Destroy to prevent multiple calls
@github-actions github-actions bot locked and limited conversation to collaborators Sep 5, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-controls-shell Shell Navigation, Routes, Tabs, Flyout community ✨ Community Contribution platform/android

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

NullReferenceException in ShellContentFragment.Destroy When _shellContext Is Null

8 participants