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

Fixed recently introduced notifications related null reference exceptions #19838

Merged
merged 1 commit into from
Dec 8, 2021

Conversation

Mailaender
Copy link
Member

@Mailaender Mailaender commented Dec 5, 2021

Closes #19837

@pchote
Copy link
Member

pchote commented Dec 5, 2021

I don't see how these changes could avoid any NREs inside PlayPredefined. What am I missing?

@Mailaender
Copy link
Member Author

I believe the crash to be related to self.World.Map.Rules where self is disposed.

@pchote
Copy link
Member

pchote commented Dec 5, 2021

INotifyKilled.Killed is never called on a disposed actor (it is called as the last thing before disposing) - if this is somehow happening, then it needs to be fixed at the source!

@Mailaender
Copy link
Member Author

It is clearly a regression from #19667 that I don't yet understand.

@Mailaender
Copy link
Member Author

I found a fix. Not sure why the stacktraces are so misleading.

@Mailaender Mailaender changed the title Try to fix recently introduced sound related null reference exceptions Fixed recently introduced notifications related null reference exceptions Dec 5, 2021
@RoosterDragon
Copy link
Member

Is the issue here that a null sound ends up in currentNotifications which later hurts us? In that case, would it make more sense to add a check after var sound = soundEngine.Play2D(... (line 410) and do if (sound == null) return false;? That would tell the caller the sound failed to play, and stops us putting nulls into either currentNotifications or currentSounds which makes life easier to think about.

@Mailaender
Copy link
Member Author

Added that as well. I don't know why the notifications are null. The rules seem to be correct.

@Dzierzan
Copy link

Dzierzan commented Dec 7, 2021

I am sorry to be an ass, but can someone review that? We wanted to release another pre-release version of OpenHV and we need this fix.

Copy link
Member

@abcdefg30 abcdefg30 left a comment

Choose a reason for hiding this comment

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

Untested but changes lgtm.

@abcdefg30 abcdefg30 added this to the Next release milestone Dec 8, 2021
@reaperrr reaperrr merged commit 5fcc049 into OpenRA:bleed Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NullReferenceException in OpenRA.Sound.PlayPredefined
6 participants