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 script used as type gone too early (3.2) #43049

Merged
merged 3 commits into from Oct 30, 2020

Conversation

RandomShaper
Copy link
Member

@RandomShaper RandomShaper commented Oct 24, 2020

Following up on what I explained in #42323, it seems that the problem with Variant's Object * constructor was very needed. The only thing that was preventing me from doing it is that GDScript was leveraging the wrong behavior. However, it seems that @vnen already fixed that.

I've cherry-picked the commit that removes that naughtiness from GDScript and the definitive fix for Variant has become possible. Variant will now manage the lifetime of a Reference nicely even if it's assigned via the Object * constructor. As I explained in the mentioned PR, without that fix, two incompatible reference counting mechanisms were being used simultaneously, which was nothing else than summoning the most terrible forces in a cold and stormy night.

NOTE: 4.0 already gets this right.

Fixes #43042.

UPDATE: Fixes #43166.

@akien-mga
Copy link
Member

Release builds fail with:

core/variant.cpp:2347:14: error: no member named 'rc' in 'Variant::ObjData'
                _get_obj().rc = NULL;
                ~~~~~~~~~~ ^
1 error generated.

@ChainedLupine
Copy link
Contributor

I'm still researching what is going on (still pretty new to Godot internals) but I think this is causing problems if I try to use duplicate with a GDScript class only meant to hold data. I am using the Resource::duplicate() to make copies at points and I am getting an unhandled exception in Object::set.

I've applied this PR on top of a fresh copy of 3.2.4 beta 1 BTW.

@RandomShaper
Copy link
Member Author

@ChainedLupine, a minimal reproduction project would be appreciated.

@ChainedLupine
Copy link
Contributor

@ChainedLupine, a minimal reproduction project would be appreciated.

Here you go!

PR43049.zip

@RandomShaper
Copy link
Member Author

I've cherry-picked a fixup by @vnen that was needed.

@ChainedLupine, I don't have time now to test your project. Could you test this PR again on it?

@ChainedLupine
Copy link
Contributor

@ChainedLupine, I don't have time now to test your project. Could you test this PR again on it?

I just re-tested with @vnen's fixup and it still does it. For the record, testing under Windows 10 with VS2019 compiler (crash happens in either debug/release).

@ChainedLupine
Copy link
Contributor

Just to verify I reverted to 2e073ec and I'm not getting the crash. It's definitely something in this PR which is causing a problem with duplicate().

@akien-mga
Copy link
Member

I can confirm that with the latest 3.2 commit (318ae4d) (+ local cherry-picks unrelated to GDScript) I don't get any issue with the project in #43049 (comment).

Haven't tried with the PR yet but this needs to be addressed before merging.

@ChainedLupine
Copy link
Contributor

I've narrowed it down to the changes in variant.cpp. For some reason, when Object::set_script occurs in the cloned Resource, the script_instance seems to refer to a ScriptInstance with no implementation. So when script_instance::set occurs, there's an unhandled exception.

@RandomShaper
Copy link
Member Author

RandomShaper commented Oct 28, 2020

I've been investigating this. The same happens in 4.0, where Variant::Variant(Object *) "detects" References, as this PR does.

The problem is that if a Reference which has not gotten it's ref. count initialized is assigned to a Variant, it will initialize it and then decrement it at destruction. The result is that the object is released prematurely.

That is happening to the self variable in GDScriptFunction::call(), when called during the duplication of the resource. The moment the script property is copied and such script is called on a still-unreferenced Reference which is the duplicate Resource.

I can think of two ways to fix this:

  1. Modify Resource::duplicate() so that the Ref<Resource>(r) is created from the very beginning.
  2. Modify Variant so it doesn't increment/decrement the ref. count of a Reference if it's not already initialized when it's assigned.
    UPDATE: 3. Modify Variant so it doesn't increment/decrement the ref. count of a Reference if's assigned via the Object * constructor.

I'd like to hear @vnen's opinion again. I'm not sure if option 2) would have unintended side effects. We could just do 1) and consider an error assigning a not yet referenced Reference to Variant.

@RandomShaper
Copy link
Member Author

@ChainedLupine, by the way, thanks for the test project and your insights!

@ChainedLupine
Copy link
Contributor

Aha! That explains why the self reference is hollowed out after it finishes running the initializer during set_script, because it's pointing to just the abstract class and not a real object. Now I understand what is going on.

@RandomShaper Thanks for figuring it out, I am so glad I could help!

vnen and others added 3 commits October 29, 2020 21:04
This is needed because of the new changes to Variant. The reference
counter is increased by adding it to a Variant, which means no GDScript
will be freed (or will be double freed if manually freed somewhere).

(cherry picked from commit 4d960ef)
@RandomShaper
Copy link
Member Author

RandomShaper commented Oct 29, 2020

To recap on the situation:

  • What this PR does seems to be correct.
  • If fixes some issues that were introduced by "the dangling Variant fix".
  • However, since this PR makes Variant more clever (the same way it is in 4.0), it uncovers a bug that is both on 3.2 and 4.0, which I explained in a comment above.
  • The plan could be getting the issue common to 3.2 and 4.0 fixed and then merge this, or if the benefits this PR provide are considered bigger than the uncovered issue, merge it right away and file a bug for such issue, which anyway needs to be addressed before the next release.

Copy link
Member

@akien-mga akien-mga left a comment

Choose a reason for hiding this comment

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

Let's merge to fix the identified regressions, and open a new bug report to track the regression already present in the master branch and which this PR is exposing to 3.2.

@akien-mga akien-mga merged commit 82bc682 into godotengine:3.2 Oct 30, 2020
@akien-mga
Copy link
Member

Thanks!

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.

None yet

4 participants