Two sets of leaks

[migueldeicaza]

I recently revamped the memory management for SwiftGodot to deal with the RefCounted scenario that needed to have its reference initialized, and it also solved a leak that was caused in code like this:

func leak () {
    var img = Image()
}

While the new code is a vast design improvement over the previous setup, it regressed a few scenarios that could have been either band-aids or where the ownership protocol contract has been broken, and I need to review those.

First Leak

let node = Node3D()

// Leaks
for _ in 0...10000 {
    for _ in node.getPropertyList() {

    }
}

// Doesn't leak
for _ in 0...10000 {
    node.getPropertyList()
}

This one is because the indexer is creating a Variant by making a copy of it, but it looks like the indexer call itself already made a copy. The change was done in the past to solve this issue: https://github.com/migueldeicaza/SwiftGodot/issues/390

Second Leak

The case is:

let bytes = PackedByteArray([137, 80, 78, 71, 13, 10, 26, 10, 0, 0, 0, 13, 73, 72, 68, 82, 0, 0, 0, 1, 0, 0, 0, 1, 1, 3, 0, 0, 0, 37, 219, 86, 202, 0, 0, 0, 3, 80, 76, 84, 69, 0, 0, 0, 167, 122, 61, 218, 0, 0, 0, 1, 116, 82, 78, 83, 0, 64, 230, 216, 102, 0, 0, 0, 10, 73, 68, 65, 84, 8, 215, 99, 96, 0, 0, 0, 2, 0, 1, 226, 33, 188, 51, 0, 0, 0, 0, 73, 69, 78, 68, 174, 66, 96, 130])

for _ in 0...500000 {
    let resource = Variant(SwiftGodot.Image()).asObject(SwiftGodot.Image.self)!
    _ = resource.loadPngFromBuffer(bytes)
    // Doesn't leak with line below uncommented
    // resource.unreference()
}

This is caused by the additional call to reference in asObject:

    public func asObject<T:GodotObject> (_ type: T.Type = T.self) -> T? {
        guard gtype == .object else {
            return nil
        }
        var value: UnsafeRawPointer = UnsafeRawPointer(bitPattern: 1)!
        toType(.object, dest: &value)
        if value == UnsafeRawPointer(bitPattern: 0) {
            return nil
        }
        let ret: T? = lookupObject(nativeHandle: value)
        if let rc = ret as? RefCounted {
            // When we pull out a refcounted out of a Variant, take a reference
            rc.reference ()
        }
        return ret
    } 

I could be wrong, and will research more next week when I am back, but I think that the code above is partially right in needing the reference, the issue is that lookupObject does two things, it performs a lookup on an object that is known (this currently should only return an existing value for user-defined types since we are tracking their lifetime) but create a fresh instance that is owned otherwise - so the reference is being added to an object that is already referenced in that case.

The additional rc.reference was introduced in 4d3f632dcdb2d7f0fb492c89e16a586ee43ce0d8

My hunch is that I need a version of lookupObject that tells me if this is a freshly created wrapper, or if this returning an object that we knew about (and thus already has a reference), and based on that call the rc.reference or not.

Tests that I should write:

• Add a user-defined type to a Node collection and to a RefCounted collection (need to find a case for the latter case)
• Retrieve the list of nodes (so I can trigger the mapping of handle to object-we-own)
• Verify the reference count, and determine whether it is needed or not.

[jbromberg]

re: the lookupObject issue, could you just check the ref count to see if it's already referenced elsewhere?

[migueldeicaza]

You never know who owns the references, you just know someone has claimed them.

I am not worried, both of those are very simple fixes, I just need a few hours to look into it, will get it fixed soon.

[migueldeicaza]

For the second leak, I almost have a patch, but I need both the tests, and add an additional scenario: currently when we are surfacing Framework peers (those that have no state of ours) we set the "ownsHandle" to false, and I think that we need to change that.

[mayoff]

I humbly suggest that commit d1d6ca4 is incorrect. PR #524 proposes a different fix, and adds a fix for the second leak, and test cases for both leaks.

[migueldeicaza]

You are absolutely right.

I threw away my half cooked patch, I was just starting to realize we needed different code paths to handle some variants and going back to the drawing board when I saw both this comment and your lovely contribution.

Thank you!