Merge pull request #2472 from samschott/fix-image-memleak

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

changes/2472.bugfix.rst

@@ -0,0 +1 @@
+Fix memory leaks for toga.Icon and toga.Image in the Cocoa backend.

cocoa/src/toga_cocoa/icons.py

@@ -13,24 +13,24 @@ def __init__(self, interface, path):
         self.path = path
         try:
             # 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.
+            # image file is invalid, the init fails, returns NULL, and releases the
+            # Objective-C object. Since we've created an ObjC instance, 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.
             image = NSImage.alloc().retain()
             self.native = image.initWithContentsOfFile(str(path))
             if self.native is None:
                 raise ValueError(f"Unable to load icon from {path}")
         finally:
+            # Calling `release` here disabled Rubicon's "release on delete" automation.
+            # We therefore add an explicit `release` call in __del__ if the NSImage was
+            # initialized successfully.
             image.release()
 
-        # Multiple icon interface instances can end up referencing the same native
-        # instance, so make sure we retain a reference count at the impl level.
-        self.native.retain()
-
     def __del__(self):
         if self.native:
             self.native.release()

cocoa/src/toga_cocoa/images.py

@@ -23,31 +23,44 @@ class Image:
 
     def __init__(self, interface, path=None, data=None, raw=None):
         self.interface = interface
+        self._needs_release = False
 
         try:
             # 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.
+            # image file is invalid, the init fails, returns NULL, and releases the
+            # Objective-C object. Since we've created an ObjC instance, 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.
             image = NSImage.alloc().retain()
             if path:
                 self.native = image.initWithContentsOfFile(str(path))
                 if self.native is None:
                     raise ValueError(f"Unable to load image from {path}")
+                else:
+                    self._needs_release = True
             elif data:
                 nsdata = NSData.dataWithBytes(data, length=len(data))
                 self.native = image.initWithData(nsdata)
                 if self.native is None:
                     raise ValueError("Unable to load image from data")
+                else:
+                    self._needs_release = True
             else:
                 self.native = raw
         finally:
+            # Calling `release` here disabled Rubicon's "release on delete" automation.
+            # We therefore add an explicit `release` call in __del__ if the NSImage was
+            # initialized successfully.
             image.release()
 
+    def __del__(self):
+        if self._needs_release:
+            self.native.release()
+
     def get_width(self):
         return self.native.size.width