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 crash caused by GDScript self unreferencing during reload #41399

Closed
wants to merge 1 commit into from
Closed

Fix crash caused by GDScript self unreferencing during reload #41399

wants to merge 1 commit into from

Conversation

Xrayez
Copy link
Contributor

@Xrayez Xrayez commented Aug 20, 2020

Fixes #41393, caused by 8088e9e.

Saving stuff in the editor runs through threads as well, which likely explains crashes to happen sporadically.

I'm not sure if manual unreferencing is needed for something else, but I couldn't reproduce the crash with this change anymore.

if (!GDScriptCache::singleton->shallow_gdscript_cache.has(source_path)) {
GDScriptCache::singleton->shallow_gdscript_cache[source_path] = self;
self->unreference();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think that self.unref() was meant to be used in this case, but then again it's not really needed to do this manually:

godot/core/reference.h

Lines 233 to 235 in 0ee0fa4

~Ref() {
unref();
}

Also found some interesting remark for mutexes:

godot/core/reference.h

Lines 216 to 225 in 0ee0fa4

void unref() {
//TODO this should be moved to mutexes, since this engine does not really
// do a lot of referencing on references and stuff
// mutexes will avoid more crashes?
if (reference && reference->unreference()) {
memdelete(reference);
}
reference = nullptr;
}

@vnen
Copy link
Member

vnen commented Aug 27, 2020

The problem is that the cache was holding references so it needs to decrease the counter, otherwise they will leak. I changed how the cache works in #41547 to not keep an extra reference so this is not needed anymore.

@Xrayez
Copy link
Contributor Author

Xrayez commented Aug 27, 2020

Oki, thanks for response, should be superseded by #41547.

@Xrayez Xrayez closed this Aug 27, 2020
@Xrayez Xrayez deleted the fix-crash-save-script branch August 27, 2020 14:25
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.

Intermittent crash when saving edited script resources
4 participants