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.

@RandomShaper RandomShaper added this to the 3.2 milestone Oct 24, 2020
@RandomShaper RandomShaper requested a review from vnen Oct 24, 2020
@akien-mga
Copy link
Member

@akien-mga akien-mga commented Oct 24, 2020

Release builds fail with:

core/variant.cpp:2347:14: error: no member named 'rc' in 'Variant::ObjData'
                _get_obj().rc = NULL;
                ~~~~~~~~~~ ^
1 error generated.
@RandomShaper RandomShaper force-pushed the RandomShaper:fix_gone_script_3.2 branch from 0359c66 to f7463ce Oct 24, 2020
@ChainedLupine
Copy link

@ChainedLupine ChainedLupine commented Oct 27, 2020

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

@RandomShaper RandomShaper commented Oct 27, 2020

@ChainedLupine, a minimal reproduction project would be appreciated.

@ChainedLupine
Copy link

@ChainedLupine ChainedLupine commented Oct 27, 2020

@ChainedLupine, a minimal reproduction project would be appreciated.

Here you go!

PR43049.zip

@RandomShaper RandomShaper force-pushed the RandomShaper:fix_gone_script_3.2 branch from f7463ce to 709e7b2 Oct 27, 2020
@RandomShaper
Copy link
Member Author

@RandomShaper RandomShaper commented Oct 27, 2020

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

@ChainedLupine ChainedLupine commented Oct 27, 2020

@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).

@vnen
vnen approved these changes Oct 27, 2020
@ChainedLupine
Copy link

@ChainedLupine ChainedLupine commented Oct 28, 2020

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

@akien-mga akien-mga commented Oct 28, 2020

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

@ChainedLupine ChainedLupine commented Oct 28, 2020

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 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

@RandomShaper RandomShaper commented Oct 28, 2020

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

@ChainedLupine
Copy link

@ChainedLupine ChainedLupine commented Oct 29, 2020

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 Feb 19, 2020
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 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.
@RandomShaper RandomShaper force-pushed the RandomShaper:fix_gone_script_3.2 branch from 709e7b2 to 1e9a774 Oct 29, 2020
Copy link
Member

@akien-mga akien-mga left a comment

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
13 checks passed
13 checks passed
Linux Headless w/ Mono (target=release_debug, tools=yes)
Details
Template (target=release, tools=no)
Details
Editor (target=release_debug, tools=yes)
Details
Template (target=release, tools=no)
Details
Editor (target=release_debug, tools=yes)
Details
Editor w/ Mono (target=release_debug, tools=yes)
Details
Static Checks (clang-format, black format, file format, documentation checks)
Details
Template (target=release, tools=no)
Details
Linux Server w/ Mono (target=release, tools=no)
Details
Template (target=release, tools=no)
Details
Template (target=release, tools=no)
Details
Template w/ Mono (target=release, tools=no)
Details
Editor and exported project with sanitizers (target=debug/release, tools=yes/no, debug_symbols=ye...
Details
@akien-mga
Copy link
Member

@akien-mga akien-mga commented Oct 30, 2020

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants