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.

Sign up for GitHub

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

Jump to bottom

Avoid retrieving the object ID of a stack variable if it is nil #80256

Merged
merged 1 commit into from
Aug 9, 2023
Merged

Avoid retrieving the object ID of a stack variable if it is nil #80256

merged 1 commit into from
Aug 9, 2023

Conversation

garychia
Copy link
Contributor

@garychia garychia commented Aug 4, 2023 •
edited by akien-mga
Loading

Fixes #80236
Bugsquad edit: Fixes #80442

This PR ensures EditorDebuggerInspector checks if the value of a stack variable is Nil before retrieving its object ID.

@garychia garychia requested a review from a team as a code owner August 4, 2023 14:47
@AThousandShips AThousandShips added this to the 4.2 milestone Aug 4, 2023
AThousandShips
AThousandShips approved these changes Aug 4, 2023
View reviewed changes
Copy link
Member

@AThousandShips AThousandShips left a comment

Choose a reason for hiding this comment

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

LGTM, check makes sense

@YuriSizov YuriSizov requested a review from Faless August 4, 2023 18:55
@akien-mga
Copy link
Member

akien-mga commented Aug 9, 2023 •
edited
Loading

Would be interesting to know what caused the regression. I suppose maybe #76582?

Edit: That's likely, this line was changed here: https://github.com/godotengine/godot/pull/76582/files#diff-fdf1c28922963d5277d923f99fbffa0a29ec276ce54ab9b8a1288ed01a3c1857R235

Edit 2: I asked reduz to assess that fix, and he found that the bug comes from this diff from @Faless: #76582 (comment) - so I'll poke @Faless for further review.

Faless
Faless approved these changes Aug 9, 2023
View reviewed changes
Copy link
Collaborator

@Faless Faless left a comment

Choose a reason for hiding this comment

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

LGTM, it will now show up correctly <null>.
I think we could iterate on this to show an empty ObjectID to make it explicit that it was supposed to be an object.
But that should go in a separate PR in case.

@akien-mga akien-mga merged commit 75c979e into godotengine:master Aug 9, 2023
14 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga akien-mga mentioned this pull request Aug 9, 2023
Closed
@garychia garychia deleted the stack_var_debug branch August 10, 2023 06:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Faless Faless Faless approved these changes

@AThousandShips AThousandShips AThousandShips approved these changes

Assignees
No one assigned
Labels
bug crash regression topic:core
Projects
None yet
Milestone
4.2
Development

Successfully merging this pull request may close these issues.

Crash on breakpoint if freed node exists Crash when the debugger encounters FileAccess.open return is null.
4 participants
@garychia @akien-mga @Faless @AThousandShips