Skip to content

Fix incorrect reference counts upon Gd object initialization #112

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

Merged
merged 1 commit into from
Feb 4, 2023

Conversation

Bromeon
Copy link
Member

@Bromeon Bromeon commented Feb 4, 2023

Changes:

  • Gd::from_obj_sys() now always initializes a strong ref (the default); previously needed .ready()
  • Gd::from_obj_sys_weak() for the rare cases where refcount is not incremented
  • Fixes several cases where accidental weak initialization was used
  • Also remove mem::forget() for ptrcall return values for now; consider explicit inc_ref later
  • Unit tests for Variant <-> Object conversions with ref counts

Fixes #108.

…t <-> Object conversion

Changes:
* Gd::from_obj_sys() now always initializes a strong ref (the default); previously needed .ready()
* Gd::from_obj_sys_weak() for the rare cases where refcount is not incremented
* Fixes several cases where accidental weak initialization was used
* Also remove mem::forget() for ptrcall return values for now; consider explicit inc_ref later
* Unit tests for Variant <-> Object conversions with ref counts
@Bromeon Bromeon added bug c: core Core components labels Feb 4, 2023
@Bromeon
Copy link
Member Author

Bromeon commented Feb 4, 2023

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 4, 2023

Build succeeded:

@bors bors bot merged commit a2b57de into master Feb 4, 2023
@bors bors bot deleted the bugfix/variant-refcount branch February 4, 2023 22:23
@Bromeon
Copy link
Member Author

Bromeon commented Feb 4, 2023

A more elaborate problem description:

Gd::from_obj_sys() did not initialize/increment (init_ref) the refcount. This was by design; a separate Gd::ready() method would do that on-demand. One bug in particular was that ready() was called for user objects (Gd::<MyClass>::new()), but not for engine objects (RefCounted::new()). When later converting such objects to variants, Godot assumes that the refcount has previously been initialized -- which is false.

Over time, it turned out that initializing the refcount should be the default behavior, while leaving it uninitialized should be reserved for special cases. As a result, I changed from_obj_sys() to follow the first behavior, and added from_obj_sys_weak() for the second one. This unifies object initialization across the codebase and makes the behavior more explicit.

There might be more edge cases, but at least Object <-> Variant conversions in both directions are now covered by integration tests, verifying refcount values at different stages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug c: core Core components ub Undefined behavior
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Converting RefCounted to Variant causes premature free
1 participant