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

Fix editor inspector crashing when the old object is no longer valid #94101

Merged
merged 1 commit into from
Jul 17, 2024

Conversation

aaronfranke
Copy link
Member

@aaronfranke aaronfranke commented Jul 8, 2024

This fixes a crash when the editor inspector tries to edit a new object when the old object is an invalid instance. See the stack trace below. I ran into this situation when closing the editor with a complex scene open. Since this is editor code that only runs once in awhile, it's not performance critical. I tested that this fix is correct by adding an else statement and confirmed that it reached there.

Dumping the backtrace. Please include this when reporting the bug to the project developer.
[1] 1   libsystem_platform.dylib            0x0000000186977584 _sigtramp + 56
[2] EditorInspector::edit(Object*) (in godot.macos.editor.arm64) + 384
[3] EditorInspector::_notification(int) (in godot.macos.editor.arm64) + 324
[4] EditorInspector::EditorInspector()
[5] Object::_predelete() (in godot.macos.editor.arm64) + 132
[6] Node::_notification(int) (in godot.macos.editor.arm64) + 384
[7] Object::_predelete() (in godot.macos.editor.arm64) + 132
[8] Node::_notification(int) (in godot.macos.editor.arm64) + 384
[9] Object::_predelete() (in godot.macos.editor.arm64) + 132
[10] Node::_notification(int) (in godot.macos.editor.arm64) + 384
[11] Object::_predelete() (in godot.macos.editor.arm64) + 132
[12] Node::_notification(int) (in godot.macos.editor.arm64) + 384
[13] Object::_predelete() (in godot.macos.editor.arm64) + 132
[14] Node::_notification(int) (in godot.macos.editor.arm64) + 384
[15] Panel::Panel()
[16] Object::_predelete() (in godot.macos.editor.arm64) + 132
[17] Node::_notification(int) (in godot.macos.editor.arm64) + 384
[18] Object::_predelete() (in godot.macos.editor.arm64) + 132
[19] Node::_notification(int) (in godot.macos.editor.arm64) + 384
[20] Object::_predelete() (in godot.macos.editor.arm64) + 132
[21] SceneTree::finalize() (in godot.macos.editor.arm64) + 64
[22] OS_MacOS::run() (in godot.macos.editor.arm64) + 408
[23] main (in godot.macos.editor.arm64) + 392
[24] 24  dyld                                0x00000001865be0e0 start + 2360
-- END OF BACKTRACE --

@aaronfranke aaronfranke added this to the 4.3 milestone Jul 8, 2024
@akien-mga akien-mga requested a review from KoBeWi July 8, 2024 21:27
@KoBeWi
Copy link
Member

KoBeWi commented Jul 8, 2024

This fixes a crash when the editor inspector tries to edit a new object when the old object is an invalid instance. I ran into this situation when closing the editor with a complex scene open.

Node editing should not happen during editor exit.
Although the new object might've been null in this case. Not sure if unediting editors is necessary.

@aaronfranke
Copy link
Member Author

@KoBeWi This happens when the old object is an invalid instance, regardless of what the new object is.

@KoBeWi
Copy link
Member

KoBeWi commented Jul 8, 2024

But it still shouldn't happen during exit. Stack trace with symbols could be useful.

@kitbdev
Copy link
Contributor

kitbdev commented Jul 8, 2024

It seems some use of EditorInspector doesn't clear its edited object before it gets deleted, but it could be a lot of places.
The object should have had a node_removed signal or should at least be cleared before EditorInspector gets deleted.

Also it's important to have that edit(nullptr) in NOTIFICATION_PREDELETE since there are multiple inspectors and some can be deleted before exiting, so they need to disconnect that signal and clear data.

edit: Finding the root cause may not be easy so this fix looks good to me to prevent the crash.

@KoBeWi
Copy link
Member

KoBeWi commented Jul 12, 2024

Did you test if it actually fixes the crash? get_validated_object() still uses the object pointer:

godot/core/variant/variant.cpp

Lines 2152 to 2154 in 97b8ad1

Object *Variant::get_validated_object() const {
if (type == OBJECT) {
return ObjectDB::get_instance(_get_obj().id);

AFAIK the only way to safely validate an object is using ObjectID. It will still crash with raw pointers.

@aaronfranke
Copy link
Member Author

aaronfranke commented Jul 12, 2024

@KoBeWi Yes, I did. It fixes the crash. From the OP:

I tested that this fix is correct by adding an else statement and confirmed that it reached there.

I added else { print_line("test"); } and confirmed that it prints this instead of crashing the editor.

Copy link
Member

@KoBeWi KoBeWi left a comment

Choose a reason for hiding this comment

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

I can't reproduce the crash, but the change does not break anything. If you say it fixes something then fine.

@akien-mga akien-mga merged commit 69a8aed into godotengine:master Jul 17, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@aaronfranke aaronfranke deleted the fix-editor-insp-crash branch October 10, 2024 04:50
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.

4 participants