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

Add tests cases for, and fix, #513 #524

Merged
merged 3 commits into from
Aug 20, 2024
Merged

Conversation

mayoff
Copy link
Contributor

@mayoff mayoff commented Aug 19, 2024

This patch addresses both leaks discussed in #513.

First leak

The changes in d1d6ca4 made several Geometry2DTests fail.

The actual unbalanced retain causing the leak in VariantRepresentable.swift:

extension ContentVariantRepresentable {
    public init? (_ variant: Variant) {
        guard Self.godotType == variant.gtype else { return nil }

        var content = Self.zero
        withUnsafeMutablePointer(to: &content) { ptr in
            variant.toType(Self.godotType, dest: ptr) // <-- THIS IS THE UNBALANCED RETAIN
        }

        self.init(content: content)
    }
}

The variant.toType function calls into Godot to copy out the built-in type stored in variant. If that built-in type has an internal reference count, Godot increments it. Then, the call to self.init(content: content) increments the count again. The retain performed by variant.toType is never balanced.

This patch fixes the bug by generating a new init(alreadyOwnedContent:) initializer for each Godot builtin class type. This needs to be on the builtin wrappers (like GDictionary, GArray, etc.), rather than on Variant which is where d1d6ca4 put it.

This patch also adds a test case to check for the leak by looking at Godot's MEMORY_STATIC performance counter, which is only enabled if Godot was built with DEBUG_ENABLED.

Second leak

This patch adds a test for the second leak described in #513, and fixes the leak.

Without this patch, the leak happens because Variant.asObject has an unneeded call to rc.reference() which increments the RefCounted object's reference count. As far as I can tell, nothing balances this increment.

Variant.asObject calls lookupObject(nativeHandle:), which returns a Swift wrapper for an object whose reference count has already been incremented (if the object is RefCounted). The Swift wrapper balances that increment with a decrement in its deinit.

Rob Mayoff added 2 commits August 19, 2024 16:16
The changes in d1d6ca4 made several `Geometry2DTests` fail.

The actual unbalanced retain causing the leak in
`VariantRepresentable.swift`:

``` swift
extension ContentVariantRepresentable {
    public init? (_ variant: Variant) {
        guard Self.godotType == variant.gtype else { return nil }

        var content = Self.zero
        withUnsafeMutablePointer(to: &content) { ptr in
            variant.toType(Self.godotType, dest: ptr) // <-- THIS IS THE UNBALANCED RETAIN
        }

        self.init(content: content)
    }
}
```

The `variant.toType` function calls into Godot to copy out the built-in
type stored in `variant`. If that built-in type has an internal
reference count, Godot increments it. Then, the call to
`self.init(content: content)` increments the count again. The retain
performed by `variant.toType` is never balanced.

This patch fixes the bug by generating a new
`init(alreadyOwnedContent:)` initializer for each Godot builtin class
type. This needs to be on the builtin wrappers (like `GDictionary`,
`GArray`, etc.), rather than on `Variant` which is where d1d6ca4 put it.

This patch also adds a test case to check for the leak by looking at
Godot's `MEMORY_STATIC` performance counter, which is only enabled if
Godot was built with `DEBUG_ENABLED`.
@mayoff mayoff mentioned this pull request Aug 19, 2024
This patch adds a test for the second leak described in migueldeicaza#513, and fixes
the leak.

Without this patch, the leak happens because `Variant.asObject` has an
unneeded call to `rc.reference()` which increments the `RefCounted`
object's reference count. As far as I can tell, nothing balances this
increment.

`Variant.asObject` calls `lookupObject(nativeHandle:)`, which returns
a Swift wrapper for an object whose reference count has already been
incremented (if the object is `RefCounted`). The Swift wrapper
balances that increment with a decrement in its `deinit`.
@migueldeicaza
Copy link
Owner

You are absolutely right about these observations, I ran into these while cooking the tests, and noticed the 2D tests fail and spent some quality time trying to figure it out, when I saw that I decided to check on the bug again and saw your analysis.

Love your patch, and the additional insight on the differences on those built-in types.

@migueldeicaza migueldeicaza merged commit 3f54e31 into migueldeicaza:main Aug 20, 2024
2 checks passed
@mayoff mayoff deleted the fix-513 branch August 20, 2024 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants