Rubicon update

[samschott]

This PR demonstrates changes required for toga to work with beeware/rubicon-objc#543. It also removes a few no longer needed manual retain / release pairs.

[samschott]

This is an interesting one: Since a manual release call no longer disables Rubicon's release-on-delete logic, we can now only call image.release() manually when indeed a additional release is needed.

[mhsmith]

I agree this is one case where manual reference management is still required. beeware/rubicon-objc#26 (comment) is a proposal for how Rubicon could handle this automatically.

[mhsmith]

As mentioned at beeware/rubicon-objc#543 (comment), once the new Rubicon version has been released, this PR should update the pyproject.toml files of toga-cocoa and toga-ios to use it.

Apart from the similar code in images and icons, the only remaining manual call to retain, release or autorelease is now in copyWithZone in widgets.internal.data. I eventually worked out that this method is necessary because the object will be assigned to NSCell.objectValue, which has the copy attribute. It would be useful to mention that in a comment.

[samschott]

I eventually worked out that this method is necessary because the object will be assigned to NSCell.objectValue, which has the copy attribute. It would be useful to mention that in a comment.

That method is indeed a bit hacky and I am wondering whether it should be implemented by returning an actual copy instead. @freakboy3742, have you tried that and do you know if there would be significant performance impact to this?

In the current state, callers will think they receive a new object which they own due to the method name. While it is true that they own the returned object, because of the manual retain call, it is not actually new. It could for example get an already cached ObjCInstance in Rubicon and therefore start leaking memory because Rubicon only releases once on Python GC.

[mhsmith]

That method is indeed a bit hacky and I am wondering whether it should be implemented by returning an actual copy instead.

I thought the same thing, but then I found that the NSCopying documentation says immutable objects may implement this method by by calling retain and returning themselves. So I think it's fine the way it is, but it would be a good idea to add a comment with a link to that docs page.

In the current state, callers will think they receive a new object which they own due to the method name. While it is true that they own the returned object, because of the manual retain call, it is not actually new. It could for example get an already cached ObjCInstance in Rubicon and therefore start leaking memory because Rubicon only releases once on Python GC.

The NSCopying docs say that the caller of copy is responsible for releasing the copy. So it'll be released once by Rubicon, plus once for each call to copyWithZone, which is exactly correct.

[mhsmith]

By the way, I think it's better not to do a force push altering commits which have already been reviewed, because it breaks the "show changes since your last review" feature in GitHub. Updates from the main branch can be brought in with a normal merge.

[freakboy3742]

That method is indeed a bit hacky and I am wondering whether it should be implemented by returning an actual copy instead. @freakboy3742, have you tried that and do you know if there would be significant performance impact to this?

I'll be completely honest - I've never 100% understood what is going on with this copy method. It's not a method that we ever call... and it's not doing anything with any data that we are adding... so I'm not entirely clear why we need to override anything. But, every time I've thought "Oh, we can just remove it", things start exploding. The explosions are probably an indication that I should spend some time to work out why and how it's working, I guess :-)

[samschott]

I thought the same thing, but then I found that the NSCopying documentation

Thanks for digging that up! A reference to that would indeed be great.

The NSCopying docs say that the caller of copy is responsible for releasing the copy. So it'll be released once by Rubicon, plus once for each call to copyWithZone, which is exactly correct.

Yes, unless Rubicon is the one calling copyWithZone (which at the moment it isn't). If a Rubicon user were calling this copyWithZone implementation, they would get back a cached ObjCInstance without knowing it and add an extra retain count for every copyWithZone call. However, Rubicon only ever performs a single autorelease call on GC.

Since this copy behavior is allowed, the implementation from beeware/rubicon-objc#543 can still cause memory leaks in some corner cases which will be very hard to debug.

Edit: Added a failing test to demonstrate this in beeware/rubicon-objc@30e4277.

[mhsmith]

I've never 100% understood what is going on with this copy method. It's not a method that we ever call... and it's not doing anything with any data that we are adding... so I'm not entirely clear why we need to override anything.

I think the sequence of events is:

• tableView_objectValueForTableColumn_row_ creates and returns the TogaData object.
• Cocoa creates a TogaDetailedCell and assigns the object to its NSCell.objectValue property.
• Because the property has a copy attribute, it calls copyWithZone on the object and assigns the return value to the property.
• OurTogaDetailedCell implementation accesses the objectValue property to get the cell's data and render it.
• When the cell is destroyed, the NSCell.dealloc method presumably releases the object reference.

If a Rubicon user were calling this copyWithZone implementation, they would get back a cached ObjCInstance without knowing it and add an extra retain count for every copyWithZone call.

I think this falls under the category of "if you call retain, then you're responsible for making sure it's released". If my analysis above is correct, then in this case we are in fact doing that. But to avoid this causing confusion again in the future, it might be better to just rewrite the method to make an actual copy.

[samschott]

But it's not only our TogaData object which has this behavior. You pointed out yourself that this is generally allowed for immutable objects. It seems indeed to be used for most immutable objects such as NSString or NSDictionary. For example:

from rubicon.objc import *
from rubicon.objc.runtime import autoreleasepool

with autoreleasepool():
    obj0 = NSString.stringWithString("some unique string")
    obj1 = obj0.copy()
    obj2 = obj0.copy()

assert obj0 is obj1
assert obj0 is obj2
assert obj0.retainCount() == 3

The caller should not need to figure out how copy is implemented for each class and manually insert release calls.

[mhsmith]

OK, I see what you mean. Let's discuss this in beeware/rubicon-objc#543.

[freakboy3742]

Something else to check - does this fix the remaining memory issues in #2853?

[freakboy3742]

While this is in draft state, would it be worth modifying the pyproject.toml references to rubicon to the PR branch? That will obviously need to be replaced with >= 0.5.0 references before it's merged, but it would give us a good multi-platform confirmation of the PR's impact.

[samschott]

Done. CI tests on a wider range of Python and platform versions than I can do locally.

[HalfWhitt]

I'm definitely still a noob about Rubicon and ObjC... what's the functional difference between these?

[samschott]

Theoretically none at all.

The Objective-C method is setValue:Animated:, in Rubicon Objective-C this translates to calling setValue_animated_().

Rubicon has some additional "magic" to accept a more Pythonic syntax setValue(value, *, animated) which relies loading and caching all method names of an object and its superclasses. It appears that the planned Rubicon update breaks this logic under some circumstances.

The last commit, i.e., this change, won't be permanent. It is only to confirm that the old syntax still works and narrow down the possible issues.