Skip to content

NullReference fix in ShellPageRendererTracker.UpdateTabBarVisibility()#26786

Merged
PureWeen merged 1 commit into
dotnet:mainfrom
Marioo1357:fix-26784
Jan 15, 2025
Merged

NullReference fix in ShellPageRendererTracker.UpdateTabBarVisibility()#26786
PureWeen merged 1 commit into
dotnet:mainfrom
Marioo1357:fix-26784

Conversation

@Marioo1357

Copy link
Copy Markdown
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
@dotnet-policy-service

Copy link
Copy Markdown
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
Copy Markdown
Member

/azp run

@PureWeen PureWeen added this to the .NET 9 SR4 milestone Dec 26, 2024
@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 3 pipeline(s).

@mattleibow

Copy link
Copy Markdown
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
Copy Markdown
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
@Marioo1357

Copy link
Copy Markdown
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
Copy Markdown
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?

@github-actions github-actions Bot locked and limited conversation to collaborators Feb 16, 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

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

NullReference in ShellPageRendererTracker.UpdateTabBarVisible()

3 participants