Fix memory leak for toga.Icon and toga.Image on Cocoa

[samschott]

Fixes #2468 by explicitly releasing any successfully create NSImage on del.

PR Checklist:

• All new features have been tested
• All new features have been documented
• I have read the CONTRIBUTING.md file
• I will abide by the code of conduct

[freakboy3742]

Can you give any explanation for the discrepancy in approaches between the two? Both makes sense; but it's not clear to me why there's a disparity. The Image approach is using the _needs_release language of Rubicon; but it's being used on a Toga object, so unless I'm missing something, it's really doing the same thing as self.native is None (just more explicitly), but then Icon isn't using the same explicit approach. The biggest change is the drop of the native.retain() in icon... which I swear was needed at some point, but you've got a clean pass on CI, which was a reliable way to reproduce the problem.

Am I missing something here?

[samschott]

the biggest change is the drop of the native.retain() in icon... which I swear was needed at some point, but you've got a clean pass on CI, which was a reliable way to reproduce the problem.

Its black magic :) Jokes aside, let me try to explain my choices (and please do question them!).

1. Image: The _needs_release approach is indeed doing practically the same as self.native is None, but the additional variable here is needed because self.native = raw uses an NSImage which was initialized outside of toga_cocoa.Image and therefore should not be released in __del__. There might be better approaches to separating the cases where we successfully initialize our own NSImage vs take in an NSImage from the caller. Btw, why is raw needed at all?

2. Icon: Since we always initialize our own NSImage, we need to always call release in __del__ if the init succeeded. I don't quite understand why the native.retain() call was ever needed. IIUC, as it stands, it just adds an additional refcount and would require two release calls in __del__. Since the lifecycle of the native NSImage is bound to the toga_cocoa.Icon anyways, there should be no risk in multiple interfaces referencing the same implementation and native instance.

[freakboy3742]

1. Image: The _needs_release approach is indeed doing practically the same as self.native is None, but the additional variable here is needed because self.native = raw uses an NSImage

Ah - the consequences of the raw API path is what I was missing. Thanks.

Btw, why is raw needed at all?

It's an internal optimisation. Some APIs (e.g., screenshot and camera APIs) return NSImage objects; without a raw API, we'd need to convert that NSImage to bytes, then create a new NSImage on those bytes. raw lets us bypass the conversion step when we know we already have a value in the internal format.

It's not a documented API (at least, not currently) because we don't want to encourage the use of that mechanism; but it's an easy performance win for internal purposes. IIRC, it's not a huge performance issue on macOS; but on Windows, the conversion process is quite slow, so it makes a big difference.

2. Icon: Since we always initialize our own NSImage, we need to always call release in __del__ if the init succeeded. I don't quite understand why the native.retain() call was ever needed.

The use case that was failing was having 2 toga.Icon instances referencing the same toga_cocoa.Icon instance (e.g., TOGA_ICON and TIBERIUS_ICON both reference the same instance). It came up in testbed testing, where test teardown would destroy one icon, then crash when it tried to dispose of the second one. Adding the retain solved that problem... but looking at the code now, I can't rationalise why that would have been.