Memory leak in toga.Image on macOS

[samschott]

Describe the bug

When instantiating an toga.Image with a valid path and eventually garbage collecting the Python object, the NSImage itself is never freed.

I believe this is because of the workaround described in inline comments in the cocoa backend here:

toga/cocoa/src/toga_cocoa/images.py

Lines 28 to 35 in f9fae85

	# We *should* be able to do a direct NSImage.alloc.init...(), but if the
	# image file is invalid, the init fails, and returns NULL - but we've
	# created an ObjC instance, so when the object passes out of scope, Rubicon
	# tries to free it, which segfaults. To avoid this, we retain result of the
	# alloc() (overriding the default Rubicon behavior of alloc), then release
	# that reference once we're done. If the image was created successfully, we
	# temporarily have a reference count that is 1 higher than it needs to be;
	# if it fails, we don't end up with a stray release.

IIUC, this works around the problem that if the image cannot be initialized from the provided path or data, the init method calls release on the NSImage instance before returning NIL. This is problematic for rubicon-objc, because it does not know about this and eventually will attempt to release the image itself when the Python object gets GCed. This causes a segfault.

The current workaround fixes the segfault explicitly retaining the image before trying to initialize it, and eventually explicitly releasing it. This is problematic because the release call disables rubicon-objc's "smart" release functionality (i.e., sets _needs_release = False. This means that we remove the additional reference, but also prevent rubicon-objc from automatically decrementing the refcount on Python GC.

https://github.com/beeware/rubicon-objc/blob/3f4d9c2bbb0397f89574339e6872047871789171/src/rubicon/objc/api.py#L943-L955

Steps to reproduce

import gc
from toga_cocoa.libs import NSImage
from rubicon.objc.api import ObjCInstance

# Replicate toga.Image implementation behaviour with a valid path.
image = NSImage.alloc().retain()
image = image.initWithContentsOfFile("/Users/samschottpersonal/Python/image.jpeg")
image.release()

# Check that rubicon-objc will no longer release the image.
assert image._needs_release == False

# Validate that it is not released
ptr_value = image.ptr.value
del image
gc.collect()

image_instance = ObjCInstance(ptr_value)
print(image_instance)

Expected behavior

Find a solution that both gracefully handles failed init of NSImage and does not cause memory leaks.

A hacky approach would be to manually set image._needs_release = True in case of successful init, or vice versa setting image._needs_release = False in case of a failed init.

Environment

• Operating System: macOS 14.4.1 (23E224)
• Python version: 3.11.1
• Software versions:
  • Toga: 0.4.2

[samschott]

Addendum: toga.Icon seems to suffer from the same problem.

[samschott]

I am also wondering whether the fact that such workarounds are required point to issues in the rubicon-objc memory management approach. But this will require more careful consideration.

[freakboy3742]

Agreed that the edge cases in Rubicon's memory management are starting to add up, and may need a more coordinated approach. There's at least one intermittent segfault that is seen in Toga's testbed in CI that is consistent in presentation, but almost impossible to narrow down because it's so infrequent.

beeware/rubicon-objc#256 has at least one suggestion for a way to address it; however, changes like this are in the "likely minor code change, huge runtime impact" category of changes that doesn't have me rushing to address it.... but they're also the sort of changes that are essential for ensuring that iOS and macOS apps are actually stable and non-leaky in the long term.