migueldeicaza / SwiftGodot

New Godot bindings for Swift
https://migueldeicaza.github.io/SwiftGodotDocs/tutorials/swiftgodot-tutorials/
MIT License
1.14k stars 76 forks source link

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

Closed mayoff closed 2 months ago

mayoff commented 2 months ago

This patch addresses both leaks discussed in #513.

First leak

The changes in https://github.com/migueldeicaza/SwiftGodot/commit/d1d6ca47f0fc5bb16948017e1a6b2c6c8838b3b5 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 https://github.com/migueldeicaza/SwiftGodot/commit/d1d6ca47f0fc5bb16948017e1a6b2c6c8838b3b5 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 https://github.com/migueldeicaza/SwiftGodot/issues/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 commented 2 months ago

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.