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

NullReference fix in ShellPageRendererTracker.UpdateTabBarVisibility() #26786

Merged
merged 1 commit into from
Jan 15, 2025

Conversation

Marioo1357
Copy link
Contributor

Description of Change

Check if Page and ViewController are not null in ShellPageRendererTracker.UpdateTabBarVisibility()

Issues Fixed

Fixes #26784

@Marioo1357 Marioo1357 requested a review from a team as a code owner December 23, 2024 15:21
@dotnet-policy-service dotnet-policy-service bot added the community ✨ Community Contribution label Dec 23, 2024
Copy link
Contributor

Hey there @Marioo1357! Thank you so much for your PR! Someone from the team will get assigned to your PR shortly and we'll get it reviewed.

@PureWeen PureWeen added the area-controls-shell Shell Navigation, Routes, Tabs, Flyout label Dec 26, 2024
@PureWeen
Copy link
Member

/azp run

@PureWeen PureWeen added this to the .NET 9 SR4 milestone Dec 26, 2024
Copy link

Azure Pipelines successfully started running 3 pipeline(s).

@mattleibow
Copy link
Member

@PureWeen is this small change just hiding some sinister bug somewhere or is it expected that VC and Page may be null at this point? I see a few null checks in places, so I am not super afraid, but it is bugging me a bit.

Regardless, this change will stop a crash, but the possible nulls are causing me to think things.

@PureWeen
Copy link
Member

It looks like the VC is GC-ing while this is still attached.
I'm curious why that's happening

I think that's not reason to block this PR as this PR should help somewhat.

Though I think this might just push the exception down to UpdateLeftToolbarItems which also references the VC.

It'd be good to have a followup PR where we check for VC going null in a couple more places.

@PureWeen PureWeen merged commit 142ec3b into dotnet:main Jan 15, 2025
102 of 104 checks passed
@Marioo1357
Copy link
Contributor Author

@mattleibow @PureWeen
We also catched one in other place in Shell. I Wonder why whole project is not nullabe friendly?
Isn't better to just remove #disable nullable and fix warrings by a null checks?

@PureWeen
Copy link
Member

@mattleibow @PureWeen We also catched one in other place in Shell. I Wonder why whole project is not nullabe friendly? Isn't better to just remove #disable nullable and fix warrings by a null checks?

Ultimately yes, but just haven't scheduled this effort yet

We'll look at doing a sweep through on NET10

we can't change any of the nullability on public APIs during a service release so we fix these in waves as we have time during major releases.

I did what I could for now here
#27160

which should help

We also catched one in other place in Shell.

Can you create an issue?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-controls-shell Shell Navigation, Routes, Tabs, Flyout community ✨ Community Contribution
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

NullReference in ShellPageRendererTracker.UpdateTabBarVisible()
3 participants