Move GDScript uninitialization to GDScriptLanguage::finish()

[adamscott]

GDScriptLanguage::finish() was empty. So I moved the uninitialization of GDScriptLanguage originally in GDScriptLanguage::~GDScriptLanguage() inside it, calling too a new GDScriptCache::clear() to clear the cache sooner than the destructor. And finish_languages is called before the servers uninit.

As cyclic references didn't exist, there wasn't any GDScript instance that could live after the removal of the root script, so it wasn't a problem until cyclic references (which could make it so that instances could live beyond the root script).

The problem existed when cyclic cached scripts would hold RID past the uninitialization of the servers.

Fixes #68973
Would be interesting to merge with #69077

[adamscott]

Credited @rburing as a co-author for his valuable help.

[adamscott]

@unfa This PR should fix the crash that happens when closing Liblast.

[akien-mga]

This would now work, but I'm a bit confused because now finalize() will be called both by GDScriptLanguage::finish() and GDScriptLanguage::~GDScriptLanguage(), but it will only do something for the first one that calls it.

Is that order non deterministic, or why do we need to call it in both?

[adamscott]

Mmm. Yeah, calling it once should work. I'll let the finalizing variable there, just to be sure that it's not called twice.

[akien-mga]

I guess there's no need for a new finalize() method now and it can go in finish() directly?

[adamscott]

Yes. I was inspired by csharp_script.cpp, which explains a lot, but yes, I'll transfer everything into finish().

godot/modules/mono/csharp_script.cpp

Lines 128 to 140 in bcc061e

	void CSharpLanguage::finish() {
	finalize();
	}
	void CSharpLanguage::finalize() {
	if (finalized) {
	return;
	}
	finalizing = true;
	// Make sure all script binding gchandles are released before finalizing GDMono
	for (KeyValue<Object *, CSharpScriptBinding> &E : script_bindings) {

[akien-mga]

Maybe @neikeq can give some input then, there might be a good reason to still call finalize() in both finish() and the destructor.

[neikeq]

Maybe @neikeq can give some input then, there might be a good reason to still call finalize() in both finish() and the destructor.

I don't really remember, to be honest, and I'm afraid changing it could cause an issue. I wish I had left an explanatory comment there...

The reason it's a different method (finalize) is because clang-tidy warns when calling virtual methods (finish) from the destructor.

[adamscott]

@kleonc I added you as co-author of the PR.

[kleonc]

@kleonc I added you as co-author of the PR.

Barely did anything but sure, fine for me. Gotta be able to blame someone for these changes. 😄

[akien-mga]

Thanks!

[adamscott]

@akien-mga Thanks for the review! 😊