From 9abcd0dd44838559be33ed9a6542557a086c5e9d Mon Sep 17 00:00:00 2001 From: samschott Date: Tue, 19 Nov 2024 10:38:06 +0100 Subject: [PATCH 01/66] Retain on ObjCInstance creation, autorelease on __del__ --- src/rubicon/objc/api.py | 70 +++++++++-------------------------------- 1 file changed, 15 insertions(+), 55 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 96104a29..8624f9bb 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -223,12 +223,7 @@ def __call__(self, receiver, *args, convert_args=True, convert_result=True): # Convert result to python type if it is an instance or class pointer. if self.restype is not None and issubclass(self.restype, objc_id): - result = ObjCInstance(result) - - # Mark for release if we acquire ownership of an object. Do not autorelease here because - # we might retain a Python reference while the Obj-C reference goes out of scope. - if self.name.startswith((b"alloc", b"new", b"copy", b"mutableCopy")): - result._needs_release = True + result = ObjCInstance(result, returned_from_method=self.name) return result @@ -783,9 +778,9 @@ def objc_class(self): return super(ObjCInstance, type(self)).__getattribute__(self, "_objc_class") except AttributeError: # This assumes that objects never change their class after they are - # seen by Rubicon. There are two reasons why this may not be true: + # seen by Rubicon. This can occur because: # - # 1. Objective-C runtime provides a function object_setClass that + # Objective-C runtime provides a function object_setClass that # can change an object's class after creation, and some code # manipulates objects' isa pointers directly (although the latter # is no longer officially supported by Apple). This is not @@ -793,19 +788,6 @@ def objc_class(self): # done during object creation/initialization, so it's basically # safe to assume that an object's class will never change after # it's been wrapped in an ObjCInstance. - # 2. If a memory address is freed by the Objective-C runtime, and - # then re-allocated by an object of a different type, but the - # Python ObjCInstance wrapper persists, Python will assume the - # object is still of the old type. If a new ObjCInstance wrapper - # for the same pointer is re-created, a check is performed to - # ensure the type hasn't changed; this problem only affects - # pre-existing Python wrappers. If this occurs, it probably - # indicates an issue with the retain count on the Python side (as - # the Objective-C runtime shouldn't be able to dispose of an - # object if Python still has a handle to it). If this *does* - # happen, it will manifest as objects appearing to be the wrong - # type, and/or objects having the wrong list of attributes - # available. Refs #249. super(ObjCInstance, type(self)).__setattr__( self, "_objc_class", ObjCClass(libobjc.object_getClass(self)) ) @@ -815,7 +797,9 @@ def objc_class(self): def _associated_attr_key_for_name(name): return SEL(f"rubicon.objc.py_attr.{name}") - def __new__(cls, object_ptr, _name=None, _bases=None, _ns=None): + def __new__( + cls, object_ptr, _name=None, _bases=None, _ns=None, returned_from_method=b"" + ): """The constructor accepts an :class:`~rubicon.objc.runtime.objc_id` or anything that can be cast to one, such as a :class:`~ctypes.c_void_p`, or an existing :class:`ObjCInstance`. @@ -914,6 +898,13 @@ class or a metaclass, an instance of :class:`ObjCClass` or except KeyError: pass + # Explicitly retain the instance on first handover to Python unless we + # received it from a method that gives us ownership already. + if not returned_from_method.startswith( + (b"alloc", b"new", b"copy", b"mutableCopy") + ): + send_message(object_ptr, "retain", restype=objc_id, argtypes=[]) + # If the given pointer points to a class, return an ObjCClass instead (if we're not already creating one). if not issubclass(cls, ObjCClass) and object_isClass(object_ptr): return ObjCClass(object_ptr) @@ -932,7 +923,6 @@ class or a metaclass, an instance of :class:`ObjCClass` or super(ObjCInstance, type(self)).__setattr__( self, "_as_parameter_", object_ptr ) - super(ObjCInstance, type(self)).__setattr__(self, "_needs_release", False) if isinstance(object_ptr, objc_block): super(ObjCInstance, type(self)).__setattr__( self, "block", ObjCBlock(object_ptr) @@ -944,39 +934,9 @@ class or a metaclass, an instance of :class:`ObjCClass` or return self - def release(self): - """Manually decrement the reference count of the corresponding objc - object. - - The objc object is sent a dealloc message when its reference - count reaches 0. Calling this method manually should not be - necessary, unless the object was explicitly ``retain``\\ed - before. Objects returned from ``.alloc().init...(...)`` and - similar calls are released automatically by Rubicon when the - corresponding Python object is deallocated. - """ - self._needs_release = False - send_message(self, "release", restype=objc_id, argtypes=[]) - - def autorelease(self): - """Decrements the receiver’s reference count at the end of the current - autorelease pool block. - - The objc object is sent a dealloc message when its reference - count reaches 0. If called, the object will not be released when - the Python object is deallocated. - """ - self._needs_release = False - result = send_message(self, "autorelease", restype=objc_id, argtypes=[]) - return ObjCInstance(result) - def __del__(self): - """Release the corresponding objc instance if we own it, i.e., if it - was returned by a method starting with :meth:`alloc`, :meth:`new`, - :meth:`copy`, or :meth:`mutableCopy` and it wasn't already explicitly - released by calling :meth:`release` or :meth:`autorelease`.""" - if self._needs_release: - send_message(self, "release", restype=objc_id, argtypes=[]) + """Release the corresponding objc instance.""" + send_message(self, "release", restype=objc_id, argtypes=[]) def __str__(self): """Get a human-readable representation of ``self``. From 6605842dec7b221b84961b8bddb455e3f358b204 Mon Sep 17 00:00:00 2001 From: samschott Date: Tue, 19 Nov 2024 23:25:05 +0100 Subject: [PATCH 02/66] update tests --- tests/test_core.py | 134 +++++++++++++++++++++------------------------ 1 file changed, 62 insertions(+), 72 deletions(-) diff --git a/tests/test_core.py b/tests/test_core.py index 4fdedebd..1f3d3a89 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -6,6 +6,7 @@ import sys import threading import unittest +import uuid import weakref from ctypes import ( ArgumentError, @@ -60,6 +61,10 @@ from . import OSX_VERSION, rubiconharness +class ObjcWeakref(NSObject): + weak_property = objc_property(weak=True) + + class struct_int_sized(Structure): _fields_ = [("x", c_char * 4)] @@ -1859,94 +1864,79 @@ class TestO: self.assertIsNone(wr_python_object()) - def test_objcinstance_release_owned(self): - # Create an object which we own. - obj = NSObject.alloc().init() - - # Check that it is marked for release. - self.assertTrue(obj._needs_release) - - # Explicitly release the object. - obj.release() + def test_objcinstance_created_retained(self): + with autoreleasepool(): + # Create an object which we don't own. + NSString = ObjCClass("NSString") + obj = NSString.stringWithString(str(uuid.uuid4())) - # Check that we no longer need to release it. - self.assertFalse(obj._needs_release) + self.assertEqual(obj.retainCount(), 1) - # Delete it and make sure that we don't segfault on garbage collection. - del obj - gc.collect() - - def test_objcinstance_autorelease_owned(self): + def test_objcinstance_explicit_retained(self): # Create an object which we own. obj = NSObject.alloc().init() - # Check that it is marked for release. - self.assertTrue(obj._needs_release) + self.assertEqual(obj.retainCount(), 1) - # Explicitly release the object. - res = obj.autorelease() - - # Check that autorelease call returned the object itself. - self.assertIs(obj, res) + def test_objcinstance_created_gc_released(self): + # Create an object which we own. + obj = NSObject.alloc().init() - # Check that we no longer need to release it. - self.assertFalse(obj._needs_release) + wr = ObjcWeakref.alloc().init() + wr.weak_property = obj # Delete it and make sure that we don't segfault on garbage collection. del obj gc.collect() - def test_objcinstance_retain_release(self): - NSString = ObjCClass("NSString") - - # Create an object which we don't own. - string = NSString.stringWithString("test") - - # Check that it is not marked for release. - self.assertFalse(string._needs_release) + # Assert that the obj was deallocated. + self.assertIsNone(wr.weak_property) - # Explicitly retain the object. - res = string.retain() - - # Check that autorelease call returned the object itself. - self.assertIs(string, res) - - # Manually release the object. - string.release() - - # Delete it and make sure that we don't segfault on garbage collection. - del string - gc.collect() - - def test_objcinstance_dealloc(self): - class DeallocTester(NSObject): - attr0 = objc_property() - attr1 = objc_property(weak=True) - - @objc_method - def dealloc(self): - self._did_dealloc = True - - obj = DeallocTester.alloc().init() - obj.__dict__["_did_dealloc"] = False - - attr0 = NSObject.alloc().init() - attr1 = NSObject.alloc().init() - - obj.attr0 = attr0 - obj.attr1 = attr1 + def test_objcinstance_explicit_retained_gc_released(self): + with autoreleasepool(): + # Create an object which we don't own. + NSString = ObjCClass("NSString") + obj = NSString.stringWithString(str(uuid.uuid4())) - self.assertEqual(attr0.retainCount(), 2) - self.assertEqual(attr1.retainCount(), 1) + wr = ObjcWeakref.alloc().init() + wr.weak_property = obj - # ObjC object will be deallocated, can only access Python attributes afterwards. - obj.release() + # Delete it and make sure that we don't segfault on garbage collection. + del obj + gc.collect() - self.assertTrue(obj._did_dealloc, "custom dealloc did not run") - self.assertEqual( - attr0.retainCount(), 1, "strong property value was not released" - ) - self.assertEqual(attr1.retainCount(), 1, "weak property value was released") + # Assert that the obj was deallocated. + self.assertIsNone(wr.weak_property) + + # def test_objcinstance_dealloc(self): + # class DeallocTester(NSObject): + # attr0 = objc_property() + # attr1 = objc_property(weak=True) + # + # @objc_method + # def dealloc(self): + # self._did_dealloc = True + # + # obj = DeallocTester.alloc().init() + # obj.__dict__["_did_dealloc"] = False + # + # attr0 = NSObject.alloc().init() + # attr1 = NSObject.alloc().init() + # + # obj.attr0 = attr0 + # obj.attr1 = attr1 + # + # self.assertEqual(attr0.retainCount(), 2) + # self.assertEqual(attr1.retainCount(), 1) + # + # # ObjC object will be deallocated, can only access Python attributes afterwards. + # obj.release() + # + # self.assertTrue(obj._did_dealloc, "custom dealloc did not run") + # self.assertEqual( + # attr0.retainCount(), 1, "strong property value was not released" + # ) + # self.assertEqual(attr1.retainCount(), 1, "weak property value was released") def test_partial_with_override(self): """If one method in a partial is overridden, that doesn't impact lookup of other partial targets""" From 931c35239335b6e86574d9a7fc842e62fc5d19f4 Mon Sep 17 00:00:00 2001 From: samschott Date: Tue, 19 Nov 2024 23:31:38 +0100 Subject: [PATCH 03/66] add change note --- changes/256.bugfix.rst | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 changes/256.bugfix.rst diff --git a/changes/256.bugfix.rst b/changes/256.bugfix.rst new file mode 100644 index 00000000..4620765d --- /dev/null +++ b/changes/256.bugfix.rst @@ -0,0 +1,2 @@ +Retain Objective-C objects when creating Python wrappers and release them when the +Python wrapped is garbage collected. From a618f2a2c229847109d304a4e4881ff422faea16 Mon Sep 17 00:00:00 2001 From: samschott Date: Wed, 20 Nov 2024 21:11:36 +0100 Subject: [PATCH 04/66] use autorelease instead of release in __del__ --- src/rubicon/objc/api.py | 4 ++-- tests/test_core.py | 18 ++++++++++++------ 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 8624f9bb..cbdc7123 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -935,8 +935,8 @@ class or a metaclass, an instance of :class:`ObjCClass` or return self def __del__(self): - """Release the corresponding objc instance.""" - send_message(self, "release", restype=objc_id, argtypes=[]) + """Autorelease the corresponding objc instance.""" + send_message(self, "autorelease", restype=objc_id, argtypes=[]) def __str__(self): """Get a human-readable representation of ``self``. diff --git a/tests/test_core.py b/tests/test_core.py index 1f3d3a89..25607640 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1414,7 +1414,8 @@ class Ivars(NSObject): self.assertEqual(r.size.height, 78) def test_class_properties(self): - """A Python class can have ObjC properties with synthesized getters and setters.""" + """A Python class can have ObjC properties with synthesized getters and setters + of ObjCInstance type .""" NSURL = ObjCClass("NSURL") @@ -1464,6 +1465,9 @@ def getSchemeIfPresent(self): self.assertIsNone(box.data) def test_class_python_properties(self): + """A Python class can have ObjC properties with synthesized getters and setters + of Python type.""" + class PythonObjectProperties(NSObject): object = objc_property(object) @@ -1499,9 +1503,10 @@ class PythonObject: properties.object = o self.assertIs(properties.object, o) - del o - del properties - gc.collect() + with autoreleasepool(): + del o + del properties + gc.collect() self.assertIsNone(wr()) @@ -1886,8 +1891,9 @@ def test_objcinstance_created_gc_released(self): wr.weak_property = obj # Delete it and make sure that we don't segfault on garbage collection. - del obj - gc.collect() + with autoreleasepool(): + del obj + gc.collect() # Assert that the obj was deallocated. self.assertIsNone(wr.weak_property) From b1bf61cf1dd32ebf1e13dab05d319ad4c294ca23 Mon Sep 17 00:00:00 2001 From: samschott Date: Wed, 20 Nov 2024 21:11:44 +0100 Subject: [PATCH 05/66] code formatting --- src/rubicon/objc/api.py | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index cbdc7123..c033a411 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -778,16 +778,13 @@ def objc_class(self): return super(ObjCInstance, type(self)).__getattribute__(self, "_objc_class") except AttributeError: # This assumes that objects never change their class after they are - # seen by Rubicon. This can occur because: - # - # Objective-C runtime provides a function object_setClass that - # can change an object's class after creation, and some code - # manipulates objects' isa pointers directly (although the latter - # is no longer officially supported by Apple). This is not - # commonly done in practice, and even then it is usually only - # done during object creation/initialization, so it's basically - # safe to assume that an object's class will never change after - # it's been wrapped in an ObjCInstance. + # seen by Rubicon. This can occur because the Objective-C runtime provides a + # function object_setClass that can change an object's class after creation, + # and some code manipulates objects' isa pointers directly (although the + # latter is no longer officially supported by Apple). This is not commonly + # done in practice, and even then it is usually only done during object + # creation/initialization, so it's basically safe to assume that an object's + # class will never change after it's been wrapped in an ObjCInstance. super(ObjCInstance, type(self)).__setattr__( self, "_objc_class", ObjCClass(libobjc.object_getClass(self)) ) From 21f2e0b92c1447328752f04acf74cc6ff9044efb Mon Sep 17 00:00:00 2001 From: samschott Date: Wed, 20 Nov 2024 23:42:02 +0100 Subject: [PATCH 06/66] update docs --- docs/how-to/memory-management.rst | 15 ++++++++------- 1 file changed, 8 insertions(+), 7 deletions(-) diff --git a/docs/how-to/memory-management.rst b/docs/how-to/memory-management.rst index 37b897d5..d81e2e9b 100644 --- a/docs/how-to/memory-management.rst +++ b/docs/how-to/memory-management.rst @@ -16,16 +16,17 @@ Rubicon Objective-C operates at runtime, it cannot make use of ARC. Reference counting in Rubicon Objective-C ----------------------------------------- -You won't have to manage reference counts in Python, Rubicon Objective-C will do -that work for you. It does so by tracking when you gain ownership of an object. -This is the case when you create an Objective-C instance using a method whose -name begins with ``alloc``, ``new``, ``copy``, or ``mutableCopy``. Rubicon -Objective-C will then insert a ``release`` call when the Python variable that -corresponds to the Objective-C instance is deallocated. +In most cases, you won't have to manage reference counts in Python, Rubicon +Objective-C will do that work for you. It does so by calling ``retain`` on an +object when Rubicon creates a ``ObjCInstance`` for it on the Python side, and calling +``autorelease`` when the ``ObjCInstance`` is garbage collected in Python. Retaining +the object ensures it is not deallocated while it is still referenced from Python +and releasing it again on ``__del__`` ensures that we do not leak memory. An exception to this is when you manually ``retain`` an object. Rubicon Objective-C will not keep track of such retain calls and you will be -responsible to insert appropriate ``release`` calls yourself. +responsible to insert appropriate ``release`` or ``autorelease`` calls yourself +to prevent leaking memory. You will also need to pay attention to reference counting in case of **weak references**. In Objective-C, creating a **weak reference** means that the From 20ab8f90791acaf1a2b105f3bb14453a3e1b8487 Mon Sep 17 00:00:00 2001 From: Sam Schott Date: Sat, 23 Nov 2024 12:42:18 +0100 Subject: [PATCH 07/66] add comment about autorelease vs release --- src/rubicon/objc/api.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index c033a411..003e9e95 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -932,7 +932,9 @@ class or a metaclass, an instance of :class:`ObjCClass` or return self def __del__(self): - """Autorelease the corresponding objc instance.""" + # Autorelease our reference on garbage collection of the Python wrapper. We use + # release instead of autorelease allow ObjC to take ownership of an object when + # it is passed for example to a strong property. send_message(self, "autorelease", restype=objc_id, argtypes=[]) def __str__(self): From 160c819232a2ad6f95ddd45a502ef1b2d3d7a3f7 Mon Sep 17 00:00:00 2001 From: Sam Schott Date: Sat, 23 Nov 2024 12:43:38 +0100 Subject: [PATCH 08/66] remove now unneeded cache staleness check --- src/rubicon/objc/api.py | 60 +---------------------------------------- 1 file changed, 1 insertion(+), 59 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 003e9e95..f0f6ccd5 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -833,65 +833,7 @@ class or a metaclass, an instance of :class:`ObjCClass` or # If an ObjCInstance already exists for the Objective-C object, # reuse it instead of creating a second ObjCInstance for the # same object. - cached = cls._cached_objects[object_ptr.value] - - # In a high-churn environment, it is possible for an object to - # be deallocated, and the same memory address be re-used on the - # Objective-C side, but the Python wrapper object for the - # original instance has *not* been cleaned up. In that - # situation, an attempt to wrap the *new* Objective-C object - # instance will cause a false positive cache hit; returning a - # Python object that has a class that doesn't match the class of - # the new instance. - # - # To prevent this, when we get a cache hit on an ObjCInstance, - # use the raw Objective-C API on the pointer to get the current - # class of the object referred to by the pointer. If there's a - # discrepancy, purge the cache for the memory address, and - # re-create the object. - # - # We do this both when the type *is* ObjCInstance (the case when - # instantiating a literal ObjCInstance()), and when type is an - # ObjCClass instance (e.g., ObjClass("Example"), which is the - # type of a directly instantiated instance of Example. - # - # We *don't* do this when the type *is* ObjCClass, - # ObjCMetaClass, as there's a race condition on startup - - # retrieving `.objc_class` causes the creation of ObjCClass - # objects, which will cause cache hits trying to re-use existing - # ObjCClass objects. However, ObjCClass instances generally - # won't be recycled or reused, so that should be safe to exclude - # from the cache freshness check. - # - # One edge case with this approach: if the old and new - # Objective-C objects have the same class, they won't be - # identified as a stale object, and they'll re-use the same - # Python wrapper. This effectively means id(obj) isn't a - # reliable instance identifier... but (a) this won't be a common - # case; (b) this flaw exists in pure Python and Objective-C as - # well, because default object identity is tied to memory - # allocation; and (c) the stale wrapper will *work*, because - # it's the correct class. - # - # Refs #249. - if cls == ObjCInstance or isinstance(cls, ObjCInstance): - cached_class_name = cached.objc_class.name - current_class_name = libobjc.class_getName( - libobjc.object_getClass(object_ptr) - ).decode("utf-8") - if ( - current_class_name != cached_class_name - and not current_class_name.endswith(f"_{cached_class_name}") - ): - # There has been a cache hit, but the object is a - # different class, treat this as a cache miss. We don't - # *just* look for an *exact* class name match, because - # some Cocoa/UIKit classes undergo a class name change - # between `alloc()` and `init()` (e.g., `NSWindow` - # becomes `NSKVONotifying_NSWindow`). Refs #257. - raise KeyError(object_ptr.value) - - return cached + return cls._cached_objects[object_ptr.value] except KeyError: pass From ce9d78c2886acda9b62ec3fe42557d2df5d0a609 Mon Sep 17 00:00:00 2001 From: Sam Schott Date: Sat, 23 Nov 2024 12:55:12 +0100 Subject: [PATCH 09/66] remove stale instance cache tests --- tests/test_core.py | 64 ---------------------------------------------- 1 file changed, 64 deletions(-) diff --git a/tests/test_core.py b/tests/test_core.py index 25607640..99ce7b64 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1958,70 +1958,6 @@ def test_partial_with_override(self): obj.method(2) self.assertEqual(obj.baseIntField, 2) - def test_stale_instance_cache(self): - """Instances returned by the ObjCInstance cache are checked for staleness (#249)""" - # Wrap 2 classes with different method lists - Example = ObjCClass("Example") - Thing = ObjCClass("Thing") - - # Create objects of 2 different types. - old = Example.alloc().init() - thing = Thing.alloc().init() - - # Deliberately poison the ObjCInstance Cache, making the memory address - # for thing point at the "old" example. This matches what happens when a - # memory address is re-allocated by the Objective-C runtime - ObjCInstance._cached_objects[thing.ptr.value] = old - - # Obtain a fresh address-based wrapper for the same Thing instance. - new_thing = Thing(thing.ptr.value) - self.assertEqual(new_thing.objc_class, Thing) - self.assertIsInstance(new_thing, ObjCInstance) - - try: - # Try to access an method known to exist on Thing - new_thing.computeSize(NSSize(37, 42)) - except AttributeError: - # If a stale wrapper is returned, new_example will be of type Example, - # so the expected method won't exist, causing an AttributeError. - self.fail("Stale wrapper returned") - - def test_stale_instance_cache_implicit(self): - """Implicit instances returned by the ObjCInstance cache are checked for staleness (#249)""" - # Wrap 2 classes with different method lists - Example = ObjCClass("Example") - Thing = ObjCClass("Thing") - - # Create objects of 2 different types. - old = Example.alloc().init() - example = Example.alloc().init() - thing = Thing.alloc().init() - - # Store the reference to Thing on example. - example.thing = thing - - # Deliberately poison the ObjCInstance cache, making the memory address - # for thing point at the "old" example. This matches what happens when a memory - # address is re-allocated by the Objective-C runtime. - ObjCInstance._cached_objects[thing.ptr.value] = old - - # When accessing a property that returns an ObjC id, we don't have - # type information, so the metaclass at time of construction is ObjCInstance, - # rather than an instance of ObjCClass. - # - # Access the thing property to return the generic instance wrapper - new_thing = example.thing - self.assertEqual(new_thing.objc_class, Thing) - self.assertIsInstance(new_thing, ObjCInstance) - - try: - # Try to access an method known to exist on Thing - new_thing.computeSize(NSSize(37, 42)) - except AttributeError: - # If a stale wrapper is returned, new_example will be of type Example, - # so the expected method won't exist, causing an AttributeError. - self.fail("Stale wrapper returned") - def test_compatible_class_name_change(self): """If the class name changes in a compatible way, the wrapper isn't recreated (#257)""" Example = ObjCClass("Example") From 6d89330981495f61d4f10376188a3a8c1723e373 Mon Sep 17 00:00:00 2001 From: samschott Date: Sun, 24 Nov 2024 21:42:31 +0100 Subject: [PATCH 10/66] update test_objcinstance_dealloc --- tests/test_core.py | 62 ++++++++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 29 deletions(-) diff --git a/tests/test_core.py b/tests/test_core.py index 99ce7b64..4f499690 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1914,35 +1914,39 @@ def test_objcinstance_explicit_retained_gc_released(self): # Assert that the obj was deallocated. self.assertIsNone(wr.weak_property) - # def test_objcinstance_dealloc(self): - # class DeallocTester(NSObject): - # attr0 = objc_property() - # attr1 = objc_property(weak=True) - # - # @objc_method - # def dealloc(self): - # self._did_dealloc = True - # - # obj = DeallocTester.alloc().init() - # obj.__dict__["_did_dealloc"] = False - # - # attr0 = NSObject.alloc().init() - # attr1 = NSObject.alloc().init() - # - # obj.attr0 = attr0 - # obj.attr1 = attr1 - # - # self.assertEqual(attr0.retainCount(), 2) - # self.assertEqual(attr1.retainCount(), 1) - # - # # ObjC object will be deallocated, can only access Python attributes afterwards. - # obj.release() - # - # self.assertTrue(obj._did_dealloc, "custom dealloc did not run") - # self.assertEqual( - # attr0.retainCount(), 1, "strong property value was not released" - # ) - # self.assertEqual(attr1.retainCount(), 1, "weak property value was released") + def test_objcinstance_dealloc(self): + + class DeallocTester(NSObject): + did_dealloc = False + + attr0 = objc_property() + attr1 = objc_property(weak=True) + + @objc_method + def dealloc(self): + DeallocTester.did_dealloc = True + + obj = DeallocTester.alloc().init() + + attr0 = NSObject.alloc().init() + attr1 = NSObject.alloc().init() + + obj.attr0 = attr0 + obj.attr1 = attr1 + + self.assertEqual(attr0.retainCount(), 2) + self.assertEqual(attr1.retainCount(), 1) + + # ObjC object will be deallocated, can only access Python attributes afterwards. + with autoreleasepool(): + del obj + gc.collect() + + self.assertTrue(DeallocTester.did_dealloc, "custom dealloc did not run") + self.assertEqual( + attr0.retainCount(), 1, "strong property value was not released" + ) + self.assertEqual(attr1.retainCount(), 1, "weak property value was released") def test_partial_with_override(self): """If one method in a partial is overridden, that doesn't impact lookup of other partial targets""" From 3bb7cccc3e14675c56f129c93861dee9dc70eb5e Mon Sep 17 00:00:00 2001 From: samschott Date: Sun, 24 Nov 2024 21:43:41 +0100 Subject: [PATCH 11/66] correct inline comment Co-authored-by: Malcolm Smith --- src/rubicon/objc/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index f0f6ccd5..4182c1be 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -875,7 +875,7 @@ class or a metaclass, an instance of :class:`ObjCClass` or def __del__(self): # Autorelease our reference on garbage collection of the Python wrapper. We use - # release instead of autorelease allow ObjC to take ownership of an object when + # autorelease instead of release to allow ObjC to take ownership of an object when # it is passed for example to a strong property. send_message(self, "autorelease", restype=objc_id, argtypes=[]) From c0b091c2157775b4d0078a65dd99a28c7718138d Mon Sep 17 00:00:00 2001 From: samschott Date: Sun, 24 Nov 2024 21:44:39 +0100 Subject: [PATCH 12/66] make returned_from_method private --- src/rubicon/objc/api.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 4182c1be..f48e8dfd 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -223,7 +223,7 @@ def __call__(self, receiver, *args, convert_args=True, convert_result=True): # Convert result to python type if it is an instance or class pointer. if self.restype is not None and issubclass(self.restype, objc_id): - result = ObjCInstance(result, returned_from_method=self.name) + result = ObjCInstance(result, _returned_from_method=self.name) return result @@ -795,7 +795,7 @@ def _associated_attr_key_for_name(name): return SEL(f"rubicon.objc.py_attr.{name}") def __new__( - cls, object_ptr, _name=None, _bases=None, _ns=None, returned_from_method=b"" + cls, object_ptr, _name=None, _bases=None, _ns=None, _returned_from_method=b"" ): """The constructor accepts an :class:`~rubicon.objc.runtime.objc_id` or anything that can be cast to one, such as a :class:`~ctypes.c_void_p`, @@ -839,7 +839,7 @@ class or a metaclass, an instance of :class:`ObjCClass` or # Explicitly retain the instance on first handover to Python unless we # received it from a method that gives us ownership already. - if not returned_from_method.startswith( + if not _returned_from_method.startswith( (b"alloc", b"new", b"copy", b"mutableCopy") ): send_message(object_ptr, "retain", restype=objc_id, argtypes=[]) From ab1f76245166b0a1faa8886a686b0912f3837e6a Mon Sep 17 00:00:00 2001 From: samschott Date: Sun, 24 Nov 2024 22:00:17 +0100 Subject: [PATCH 13/66] update ObjCInstance doc string --- src/rubicon/objc/api.py | 15 +++++++++++---- 1 file changed, 11 insertions(+), 4 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index f48e8dfd..280aef05 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -814,10 +814,17 @@ class or a metaclass, an instance of :class:`ObjCClass` or :func:`register_type_for_objcclass`. Creating an :class:`ObjCInstance` from a ``nil`` pointer returns ``None``. - Rubicon currently does not perform any automatic memory management on - the Objective-C object wrapped in an :class:`ObjCInstance`. It is the - user's responsibility to ``retain`` and ``release`` wrapped objects as - needed, like in Objective-C code without automatic reference counting. + Rubicon retains an Objective-C object when it is wrapped in an + :class:`ObjCInstance` and autoreleases it when the :class:`ObjCInstance` is + garbage collected. + + The only exception to this are objects returned by methods which create an + object (starting with "alloc", "new", "copy", or "mutableCopy"). We do not + explicitly retain them because we already own objects created by us, but we do + autorelease them on garbage collection of the Python wrapper. + + This ensures that the :class:`ObjCInstance` can always be used from Python + without segfaults while preventing Rubicon from leaking memory. """ # Make sure that object_ptr is wrapped in an objc_id. From 544d69412cf89a96f3e22ed64d59fbebc1f30273 Mon Sep 17 00:00:00 2001 From: samschott Date: Sun, 24 Nov 2024 22:12:50 +0100 Subject: [PATCH 14/66] updated docs --- docs/how-to/memory-management.rst | 32 ++++++++++++++++++++----------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/docs/how-to/memory-management.rst b/docs/how-to/memory-management.rst index d81e2e9b..12f916d7 100644 --- a/docs/how-to/memory-management.rst +++ b/docs/how-to/memory-management.rst @@ -2,6 +2,9 @@ Memory management for Objective-C instances =========================================== +Reference counting in Objective-C +================================= + Reference counting works differently in Objective-C compared to Python. Python will automatically track where variables are referenced and free memory when the reference count drops to zero whereas Objective-C uses explicit reference @@ -13,8 +16,8 @@ When enabling automatic reference counting (ARC), the appropriate calls for memory management will be inserted for you at compile-time. However, since Rubicon Objective-C operates at runtime, it cannot make use of ARC. -Reference counting in Rubicon Objective-C ------------------------------------------ +Reference management in Rubicon +=============================== In most cases, you won't have to manage reference counts in Python, Rubicon Objective-C will do that work for you. It does so by calling ``retain`` on an @@ -23,19 +26,26 @@ object when Rubicon creates a ``ObjCInstance`` for it on the Python side, and ca the object ensures it is not deallocated while it is still referenced from Python and releasing it again on ``__del__`` ensures that we do not leak memory. -An exception to this is when you manually ``retain`` an object. Rubicon -Objective-C will not keep track of such retain calls and you will be -responsible to insert appropriate ``release`` or ``autorelease`` calls yourself -to prevent leaking memory. +The only exception to this is when you create an object -- which is always done +through methods starting with "alloc", "new", "copy", or "mutableCopy". Rubicon does +not explicitly retain such objects because we own objects created by us, but Rubicon +does autorelease them when the Python wrapper is garbage collected. + +Rubicon Objective-C will not keep track if you additionally manually ``retain`` an +object. You will be responsible to insert appropriate ``release`` or ``autorelease`` +calls yourself to prevent leaking memory. + +Weak references in Objective-C +------------------------------ -You will also need to pay attention to reference counting in case of **weak -references**. In Objective-C, creating a **weak reference** means that the -reference count of the object is not incremented and the object will still be +You will need to pay attention to reference counting in case of **weak +references**. In Objective-C, as in Python, creating a weak reference means that +the reference count of the object is not incremented and the object will be deallocated when no strong references remain. Any weak references to the object are then set to ``nil``. -Some objects will store references to other objects as a weak reference. Such -properties will be declared in the Apple developer documentation as +Some Objective-C objects store references to other objects as a weak reference. +Such properties will be declared in the Apple developer documentation as "@property(weak)" or "@property(assign)". This is commonly the case for delegates. For example, in the code below, the ``NSOutlineView`` only stores a weak reference to the object which is assigned to its delegate property: From b4a1624733f73dae91f8a8768af08c67abb3ecaa Mon Sep 17 00:00:00 2001 From: samschott Date: Sun, 24 Nov 2024 22:27:08 +0100 Subject: [PATCH 15/66] update spellchecker --- docs/spelling_wordlist | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/spelling_wordlist b/docs/spelling_wordlist index b5ed95f3..18050a76 100644 --- a/docs/spelling_wordlist +++ b/docs/spelling_wordlist @@ -1,6 +1,9 @@ Alea +alloc Autorelease +autorelease autoreleased +autoreleases Bugfixes callables CPython @@ -22,6 +25,7 @@ lookups macOS metaclass metaclasses +mutableCopy namespace namespaces ObjC From f0edb5bace93abfe34e6f52b7dfc499a14119185 Mon Sep 17 00:00:00 2001 From: samschott Date: Sun, 24 Nov 2024 22:41:49 +0100 Subject: [PATCH 16/66] update change notes with migration instructions --- changes/256.bugfix.rst | 10 +++++++++- changes/256.removal.rst | 4 ++++ 2 files changed, 13 insertions(+), 1 deletion(-) create mode 100644 changes/256.removal.rst diff --git a/changes/256.bugfix.rst b/changes/256.bugfix.rst index 4620765d..5028dcba 100644 --- a/changes/256.bugfix.rst +++ b/changes/256.bugfix.rst @@ -1,2 +1,10 @@ Retain Objective-C objects when creating Python wrappers and release them when the -Python wrapped is garbage collected. +Python wrapped is garbage collected. This means that manual ``retain`` calls and +subsequent ``release`` or ``autorelease`` calls from Python are no longer needed with +very few exceptions such as: + +1. When implementing methods like ``copy`` that are supposed to create an object, if + the returned object is not actually newly created. +2. When dealing with side effects of methods like ``init`` that may release an object + which is still referenced from Python. See for example + https://github.com/beeware/toga/issues/2468. diff --git a/changes/256.removal.rst b/changes/256.removal.rst new file mode 100644 index 00000000..b4a409ac --- /dev/null +++ b/changes/256.removal.rst @@ -0,0 +1,4 @@ +Rubicon no longer skips releasing an Objective-C objects when its Python wrapped is +garbage collected. This means that fewer ``retain`` than ``release`` calls will cause +segfaults on garbage collection. Review your code carefully for unbalanced ``retain`` +and ``release`` calls before updating. From 22396dc9910d4a3a61b43b614404e0de9623534a Mon Sep 17 00:00:00 2001 From: samschott Date: Mon, 25 Nov 2024 18:28:11 +0100 Subject: [PATCH 17/66] Rephrase removal note Co-authored-by: Malcolm Smith --- changes/256.removal.rst | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/changes/256.removal.rst b/changes/256.removal.rst index b4a409ac..76268246 100644 --- a/changes/256.removal.rst +++ b/changes/256.removal.rst @@ -1,4 +1,5 @@ -Rubicon no longer skips releasing an Objective-C objects when its Python wrapped is +Manual calls to ``release`` or ``autorelease`` no longer cause Rubicon +to skip releasing an Objective-C object when its Python wrapper is garbage collected. This means that fewer ``retain`` than ``release`` calls will cause segfaults on garbage collection. Review your code carefully for unbalanced ``retain`` and ``release`` calls before updating. From 7d51fde24b3b6a23d011fb18109f4d276ddd06d2 Mon Sep 17 00:00:00 2001 From: samschott Date: Mon, 25 Nov 2024 18:28:38 +0100 Subject: [PATCH 18/66] remove unneeded space in doc string Co-authored-by: Malcolm Smith --- src/rubicon/objc/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 280aef05..3a44c7c8 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -823,7 +823,7 @@ class or a metaclass, an instance of :class:`ObjCClass` or explicitly retain them because we already own objects created by us, but we do autorelease them on garbage collection of the Python wrapper. - This ensures that the :class:`ObjCInstance` can always be used from Python + This ensures that the :class:`ObjCInstance` can always be used from Python without segfaults while preventing Rubicon from leaking memory. """ From acfa546760ba7b955b32e4144c89dfc6482e1ec4 Mon Sep 17 00:00:00 2001 From: samschott Date: Mon, 25 Nov 2024 18:30:01 +0100 Subject: [PATCH 19/66] change bugfix to feature note --- changes/{256.bugfix.rst => 256.feature.rst} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename changes/{256.bugfix.rst => 256.feature.rst} (100%) diff --git a/changes/256.bugfix.rst b/changes/256.feature.rst similarity index 100% rename from changes/256.bugfix.rst rename to changes/256.feature.rst From 18e08cc2cea00a660305a71750a939c681a79ca0 Mon Sep 17 00:00:00 2001 From: samschott Date: Mon, 25 Nov 2024 18:31:56 +0100 Subject: [PATCH 20/66] Fix incorrect inline comment Co-authored-by: Malcolm Smith --- src/rubicon/objc/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 3a44c7c8..f7b454af 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -883,7 +883,7 @@ class or a metaclass, an instance of :class:`ObjCClass` or def __del__(self): # Autorelease our reference on garbage collection of the Python wrapper. We use # autorelease instead of release to allow ObjC to take ownership of an object when - # it is passed for example to a strong property. + # it is returned from a factory method. send_message(self, "autorelease", restype=objc_id, argtypes=[]) def __str__(self): From 532fbe00e26aa028a82dbf48273c604df4951f96 Mon Sep 17 00:00:00 2001 From: samschott Date: Mon, 25 Nov 2024 18:34:21 +0100 Subject: [PATCH 21/66] trim trailing whitespace --- changes/256.removal.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changes/256.removal.rst b/changes/256.removal.rst index 76268246..92d3ecce 100644 --- a/changes/256.removal.rst +++ b/changes/256.removal.rst @@ -1,4 +1,4 @@ -Manual calls to ``release`` or ``autorelease`` no longer cause Rubicon +Manual calls to ``release`` or ``autorelease`` no longer cause Rubicon to skip releasing an Objective-C object when its Python wrapper is garbage collected. This means that fewer ``retain`` than ``release`` calls will cause segfaults on garbage collection. Review your code carefully for unbalanced ``retain`` From 52e92c05d0124be555e58322ad6a3fb30b822629 Mon Sep 17 00:00:00 2001 From: samschott Date: Mon, 25 Nov 2024 18:45:04 +0100 Subject: [PATCH 22/66] update test comment --- tests/test_core.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/test_core.py b/tests/test_core.py index 4f499690..570c3063 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1937,7 +1937,9 @@ def dealloc(self): self.assertEqual(attr0.retainCount(), 2) self.assertEqual(attr1.retainCount(), 1) - # ObjC object will be deallocated, can only access Python attributes afterwards. + # Delete the Python wrapper and ensure that the Objective-C object is + # deallocated after ``autorelease`` on garbage collection. This will also + # rigger a decrement in the retain count of attr0. with autoreleasepool(): del obj gc.collect() From efed734d9594f786eebc0185ff202e54e277c344 Mon Sep 17 00:00:00 2001 From: samschott Date: Mon, 25 Nov 2024 18:51:25 +0100 Subject: [PATCH 23/66] check that objects are not deallocated before end of autorelease pool --- tests/test_core.py | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/tests/test_core.py b/tests/test_core.py index 570c3063..b569c266 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1895,8 +1895,11 @@ def test_objcinstance_created_gc_released(self): del obj gc.collect() - # Assert that the obj was deallocated. - self.assertIsNone(wr.weak_property) + self.assertIsNotNone( + wr.weak_property, "object was deallocated before end of autorelease pool" + ) + + self.assertIsNone(wr.weak_property, "object was not deallocated") def test_objcinstance_explicit_retained_gc_released(self): with autoreleasepool(): @@ -1911,8 +1914,11 @@ def test_objcinstance_explicit_retained_gc_released(self): del obj gc.collect() - # Assert that the obj was deallocated. - self.assertIsNone(wr.weak_property) + self.assertIsNotNone( + wr.weak_property, "object was deallocated before end of autorelease pool" + ) + + self.assertIsNone(wr.weak_property, "object was not deallocated") def test_objcinstance_dealloc(self): From ab8a8950aa86cf5cf2bfea65f70aa613d14c722e Mon Sep 17 00:00:00 2001 From: samschott Date: Mon, 25 Nov 2024 19:14:59 +0100 Subject: [PATCH 24/66] merge object lifecycle tests --- tests/test_core.py | 49 +++++++++++++++++++++++----------------------- 1 file changed, 25 insertions(+), 24 deletions(-) diff --git a/tests/test_core.py b/tests/test_core.py index b569c266..458486a9 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1869,53 +1869,54 @@ class TestO: self.assertIsNone(wr_python_object()) - def test_objcinstance_created_retained(self): + def test_objcinstance_returned_lifecycle(self): + """An object is retained when creating an ObjCInstance for it and autoreleased + when the ObjCInstance is garbage collected. + """ with autoreleasepool(): - # Create an object which we don't own. + # Return an object which we don't own. Using str(uuid) here instead of a + # common string ensure that we get have the only reference. NSString = ObjCClass("NSString") obj = NSString.stringWithString(str(uuid.uuid4())) - self.assertEqual(obj.retainCount(), 1) - - def test_objcinstance_explicit_retained(self): - # Create an object which we own. - obj = NSObject.alloc().init() - - self.assertEqual(obj.retainCount(), 1) - - def test_objcinstance_created_gc_released(self): - # Create an object which we own. - obj = NSObject.alloc().init() + # Check that the object is retained when we create the ObjCInstance. + self.assertEqual(obj.retainCount(), 1, "object was not retained") + # Assign the object to an Obj-C weakref and delete it check that it is dealloced. wr = ObjcWeakref.alloc().init() wr.weak_property = obj - # Delete it and make sure that we don't segfault on garbage collection. with autoreleasepool(): del obj gc.collect() self.assertIsNotNone( - wr.weak_property, "object was deallocated before end of autorelease pool" + wr.weak_property, + "object was deallocated before end of autorelease pool", ) self.assertIsNone(wr.weak_property, "object was not deallocated") - def test_objcinstance_explicit_retained_gc_released(self): - with autoreleasepool(): - # Create an object which we don't own. - NSString = ObjCClass("NSString") - obj = NSString.stringWithString(str(uuid.uuid4())) + def test_objcinstance_owned_lifecycle(self): + """An object is not additionally retained when we create it ourselves. It is + autoreleased when the ObjCInstance is garbage collected. + """ + # Create an object which we own. Check that is has a retain count of 1. + obj = NSObject.alloc().init() + + self.assertEqual(obj.retainCount(), 1, "object should be retained only once") - wr = ObjcWeakref.alloc().init() - wr.weak_property = obj + # Assign the object to an Obj-C weakref and delete it check that it is dealloced. + wr = ObjcWeakref.alloc().init() + wr.weak_property = obj - # Delete it and make sure that we don't segfault on garbage collection. + with autoreleasepool(): del obj gc.collect() self.assertIsNotNone( - wr.weak_property, "object was deallocated before end of autorelease pool" + wr.weak_property, + "object was deallocated before end of autorelease pool", ) self.assertIsNone(wr.weak_property, "object was not deallocated") From 30e4277d86e3ef5fae09c4aab4868bdd1fd3397b Mon Sep 17 00:00:00 2001 From: samschott Date: Mon, 25 Nov 2024 23:12:23 +0100 Subject: [PATCH 25/66] add a test case for copyWithZone returning the existing instance with an additional refcount --- tests/test_core.py | 33 +++++++++++++++++++++++++++++++++ 1 file changed, 33 insertions(+) diff --git a/tests/test_core.py b/tests/test_core.py index 458486a9..53557e35 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1921,6 +1921,39 @@ def test_objcinstance_owned_lifecycle(self): self.assertIsNone(wr.weak_property, "object was not deallocated") + def test_objcinstance_fake_copy_lifecycle(self): + + class CopyTest(NSObject): + @objc_method + def copyWithZone_(self): + # Return an immutable `self` with an additional retain count. This + # represents the third option for implementing copyWithZone from + # https://developer.apple.com/documentation/foundation/nscopying#overview. + self.retain() + return self + + obj0 = CopyTest.alloc().init() + obj1 = obj0.copy() + obj2 = obj0.copy() + + self.assertIs(obj0, obj1) + self.assertIs(obj0, obj2) + + # Assign the object to an Obj-C weakref and delete it check that it is dealloced. + wr = ObjcWeakref.alloc().init() + wr.weak_property = obj0 + + with autoreleasepool(): + del obj0, obj1, obj2 + gc.collect() + + self.assertIsNotNone( + wr.weak_property, + "object was deallocated before end of autorelease pool", + ) + + self.assertIsNone(wr.weak_property, "object was not deallocated") + def test_objcinstance_dealloc(self): class DeallocTester(NSObject): From c3a4fe186c4e8a4c5752d514419fe9cbf82dcb90 Mon Sep 17 00:00:00 2001 From: samschott Date: Mon, 25 Nov 2024 23:30:58 +0100 Subject: [PATCH 26/66] release additional refcounts by copy calls on the same ObjCInstance --- src/rubicon/objc/api.py | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index f7b454af..135cad50 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -91,6 +91,10 @@ # the Python objects are not destroyed if they are otherwise no Python references left. _keep_alive_objects = {} +# Methods that return an object with is implicitly retained by the caller. +# See https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmRules.html +_OWNERSHIP_METHOD_PREFIXES = (b"alloc", b"new", b"copy", b"mutableCopy") + def encoding_from_annotation(f, offset=1): argspec = inspect.getfullargspec(inspect.unwrap(f)) @@ -768,6 +772,8 @@ class ObjCInstance: # Refs #251. _instance_lock = threading.RLock() + _python_refcount = 0 + @property def objc_class(self): """The Objective-C object's class, as an :class:`ObjCClass`.""" @@ -840,15 +846,22 @@ class or a metaclass, an instance of :class:`ObjCClass` or # If an ObjCInstance already exists for the Objective-C object, # reuse it instead of creating a second ObjCInstance for the # same object. - return cls._cached_objects[object_ptr.value] + cached_obj = cls._cached_objects[object_ptr.value] + + # If a cached instance was returned from a call such as `copy` or + # `mutableCopy`, we take ownership of an additional refcount. Release + # it here to prevent leaking memory, Python already owns a refcount from + # when the item was put in the cache. + if _returned_from_method.startswith(_OWNERSHIP_METHOD_PREFIXES): + send_message(object_ptr, "release", restype=objc_id, argtypes=[]) + + return cached_obj except KeyError: pass # Explicitly retain the instance on first handover to Python unless we # received it from a method that gives us ownership already. - if not _returned_from_method.startswith( - (b"alloc", b"new", b"copy", b"mutableCopy") - ): + if not _returned_from_method.startswith(_OWNERSHIP_METHOD_PREFIXES): send_message(object_ptr, "retain", restype=objc_id, argtypes=[]) # If the given pointer points to a class, return an ObjCClass instead (if we're not already creating one). From 7bdc31fa62b6044a72ce409443b6f8df993ceaef Mon Sep 17 00:00:00 2001 From: samschott Date: Tue, 26 Nov 2024 10:00:22 +0100 Subject: [PATCH 27/66] rewrite the copy lifecycle test to use NSDictionary instead of a custom object --- tests/test_core.py | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/tests/test_core.py b/tests/test_core.py index 53557e35..a93adf4e 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -29,6 +29,7 @@ CGPoint, CGRect, CGSize, + NSDictionary, NSEdgeInsets, NSEdgeInsetsMake, NSMakeRect, @@ -1922,17 +1923,11 @@ def test_objcinstance_owned_lifecycle(self): self.assertIsNone(wr.weak_property, "object was not deallocated") def test_objcinstance_fake_copy_lifecycle(self): - - class CopyTest(NSObject): - @objc_method - def copyWithZone_(self): - # Return an immutable `self` with an additional retain count. This - # represents the third option for implementing copyWithZone from - # https://developer.apple.com/documentation/foundation/nscopying#overview. - self.retain() - return self - - obj0 = CopyTest.alloc().init() + """If the same object is returned from multipe creation methods, it is still + freed on Python garbage collection.""" + obj0 = NSDictionary.alloc().initWithObjects( + [str(uuid.uuid4())], forKeys=[str(uuid.uuid4())] + ) obj1 = obj0.copy() obj2 = obj0.copy() From 460728b3e6baec2b4947164d7371d5623f533126 Mon Sep 17 00:00:00 2001 From: samschott Date: Tue, 26 Nov 2024 10:01:11 +0100 Subject: [PATCH 28/66] prevent errors on ObjCInstance garbage collection when `send_message` is None --- src/rubicon/objc/api.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 135cad50..581f7838 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -897,7 +897,8 @@ def __del__(self): # Autorelease our reference on garbage collection of the Python wrapper. We use # autorelease instead of release to allow ObjC to take ownership of an object when # it is returned from a factory method. - send_message(self, "autorelease", restype=objc_id, argtypes=[]) + if send_message and objc_id: + send_message(self, "autorelease", restype=objc_id, argtypes=[]) def __str__(self): """Get a human-readable representation of ``self``. From d9c0f626209c405e95529fb2846a29d9fc8e506f Mon Sep 17 00:00:00 2001 From: samschott Date: Tue, 26 Nov 2024 18:47:24 +0100 Subject: [PATCH 29/66] switch copy lifecycle test to use NSString --- tests/test_core.py | 25 +++++++------------------ 1 file changed, 7 insertions(+), 18 deletions(-) diff --git a/tests/test_core.py b/tests/test_core.py index a93adf4e..6c386caa 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -61,6 +61,9 @@ from . import OSX_VERSION, rubiconharness +NSArray = ObjCClass("NSArray") +NSString = ObjCClass("NSString") + class ObjcWeakref(NSObject): weak_property = objc_property(weak=True) @@ -278,10 +281,6 @@ def test_objcprotocol_protocols(self): def test_objcclass_instancecheck(self): """isinstance works with an ObjCClass as the second argument.""" - - NSArray = ObjCClass("NSArray") - NSString = ObjCClass("NSString") - self.assertIsInstance(NSObject.new(), NSObject) self.assertIsInstance(at(""), NSString) self.assertIsInstance(at(""), NSObject) @@ -294,10 +293,6 @@ def test_objcclass_instancecheck(self): def test_objcclass_subclasscheck(self): """issubclass works with an ObjCClass as the second argument.""" - - NSArray = ObjCClass("NSArray") - NSString = ObjCClass("NSString") - self.assertTrue(issubclass(NSObject, NSObject)) self.assertTrue(issubclass(NSString, NSObject)) self.assertTrue(issubclass(NSObject.objc_class, NSObject)) @@ -329,8 +324,6 @@ def test_objcprotocol_instancecheck(self): def test_objcprotocol_subclasscheck(self): """issubclass works with an ObjCProtocol as the second argument.""" - - NSString = ObjCClass("NSString") NSCopying = ObjCProtocol("NSCopying") NSCoding = ObjCProtocol("NSCoding") NSSecureCoding = ObjCProtocol("NSSecureCoding") @@ -448,8 +441,6 @@ def test_method_incorrect_argument_count_send(self): def test_method_varargs_send(self): """A variadic method can be called using send_message.""" - - NSString = ObjCClass("NSString") formatted = send_message( NSString, "stringWithFormat:", @@ -1877,7 +1868,6 @@ def test_objcinstance_returned_lifecycle(self): with autoreleasepool(): # Return an object which we don't own. Using str(uuid) here instead of a # common string ensure that we get have the only reference. - NSString = ObjCClass("NSString") obj = NSString.stringWithString(str(uuid.uuid4())) # Check that the object is retained when we create the ObjCInstance. @@ -1925,11 +1915,10 @@ def test_objcinstance_owned_lifecycle(self): def test_objcinstance_fake_copy_lifecycle(self): """If the same object is returned from multipe creation methods, it is still freed on Python garbage collection.""" - obj0 = NSDictionary.alloc().initWithObjects( - [str(uuid.uuid4())], forKeys=[str(uuid.uuid4())] - ) - obj1 = obj0.copy() - obj2 = obj0.copy() + with autoreleasepool(): + obj0 = NSString.stringWithString(str(uuid.uuid4())) + obj1 = obj0.copy() + obj2 = obj0.copy() self.assertIs(obj0, obj1) self.assertIs(obj0, obj2) From 49d9381596847d3ee250e75190d0394e0b852097 Mon Sep 17 00:00:00 2001 From: samschott Date: Tue, 26 Nov 2024 18:49:33 +0100 Subject: [PATCH 30/66] remove unused import --- tests/test_core.py | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/test_core.py b/tests/test_core.py index 6c386caa..de517525 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -29,7 +29,6 @@ CGPoint, CGRect, CGSize, - NSDictionary, NSEdgeInsets, NSEdgeInsetsMake, NSMakeRect, From e0d7792177682d139e3277824e97ef93047fcf6e Mon Sep 17 00:00:00 2001 From: samschott Date: Tue, 26 Nov 2024 18:54:32 +0100 Subject: [PATCH 31/66] fix spelling mistake --- tests/test_core.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_core.py b/tests/test_core.py index de517525..f99cd937 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1912,7 +1912,7 @@ def test_objcinstance_owned_lifecycle(self): self.assertIsNone(wr.weak_property, "object was not deallocated") def test_objcinstance_fake_copy_lifecycle(self): - """If the same object is returned from multipe creation methods, it is still + """If the same object is returned from multiple creation methods, it is still freed on Python garbage collection.""" with autoreleasepool(): obj0 = NSString.stringWithString(str(uuid.uuid4())) From 715912f75c8552534a15c1f7fd0355c678b51caa Mon Sep 17 00:00:00 2001 From: samschott Date: Tue, 26 Nov 2024 21:29:58 +0100 Subject: [PATCH 32/66] spelling updates Co-authored-by: Malcolm Smith --- src/rubicon/objc/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 581f7838..32a9b233 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -91,7 +91,7 @@ # the Python objects are not destroyed if they are otherwise no Python references left. _keep_alive_objects = {} -# Methods that return an object with is implicitly retained by the caller. +# Methods that return an object which is implicitly retained by the caller. # See https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmRules.html _OWNERSHIP_METHOD_PREFIXES = (b"alloc", b"new", b"copy", b"mutableCopy") From 20e45b6fd14e90ac4b31a48bec280b7845ac38e1 Mon Sep 17 00:00:00 2001 From: samschott Date: Tue, 26 Nov 2024 21:30:08 +0100 Subject: [PATCH 33/66] spelling updates Co-authored-by: Malcolm Smith --- src/rubicon/objc/api.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 32a9b233..9563d2a3 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -771,9 +771,6 @@ class ObjCInstance: # # Refs #251. _instance_lock = threading.RLock() - - _python_refcount = 0 - @property def objc_class(self): """The Objective-C object's class, as an :class:`ObjCClass`.""" From 86b29a4dd8b3c160cc467369274ab01c653540dc Mon Sep 17 00:00:00 2001 From: samschott Date: Tue, 26 Nov 2024 21:30:19 +0100 Subject: [PATCH 34/66] spelling updates Co-authored-by: Malcolm Smith --- tests/test_core.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_core.py b/tests/test_core.py index f99cd937..618374bb 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1962,7 +1962,7 @@ def dealloc(self): # Delete the Python wrapper and ensure that the Objective-C object is # deallocated after ``autorelease`` on garbage collection. This will also - # rigger a decrement in the retain count of attr0. + # trigger a decrement in the retain count of attr0. with autoreleasepool(): del obj gc.collect() From 944328d5d14581651385f0aa3df1e8342599fdc6 Mon Sep 17 00:00:00 2001 From: samschott Date: Tue, 26 Nov 2024 21:34:22 +0100 Subject: [PATCH 35/66] black code formatting --- src/rubicon/objc/api.py | 1 + 1 file changed, 1 insertion(+) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 9563d2a3..c16a47dc 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -771,6 +771,7 @@ class ObjCInstance: # # Refs #251. _instance_lock = threading.RLock() + @property def objc_class(self): """The Objective-C object's class, as an :class:`ObjCClass`.""" From 3b88aaa881e9bcdcfeb1095a98522a6ef80613a2 Mon Sep 17 00:00:00 2001 From: samschott Date: Tue, 26 Nov 2024 22:43:15 +0100 Subject: [PATCH 36/66] rename test case to "immutable copy lifecycle" --- tests/test_core.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_core.py b/tests/test_core.py index 618374bb..39a3ba07 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1911,7 +1911,7 @@ def test_objcinstance_owned_lifecycle(self): self.assertIsNone(wr.weak_property, "object was not deallocated") - def test_objcinstance_fake_copy_lifecycle(self): + def test_objcinstance_immutable_copy_lifecycle(self): """If the same object is returned from multiple creation methods, it is still freed on Python garbage collection.""" with autoreleasepool(): From 58d0276cba7da64780e8d26bb83d650e394d05e4 Mon Sep 17 00:00:00 2001 From: samschott Date: Wed, 27 Nov 2024 22:57:35 +0100 Subject: [PATCH 37/66] improve inline docs --- src/rubicon/objc/api.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index c16a47dc..1a0b1296 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -846,11 +846,12 @@ class or a metaclass, an instance of :class:`ObjCClass` or # same object. cached_obj = cls._cached_objects[object_ptr.value] - # If a cached instance was returned from a call such as `copy` or - # `mutableCopy`, we take ownership of an additional refcount. Release - # it here to prevent leaking memory, Python already owns a refcount from - # when the item was put in the cache. - if _returned_from_method.startswith(_OWNERSHIP_METHOD_PREFIXES): + # A `copy` call can return the original object if it is immutable. This + # is typically done for optimization. If this object is already in our + # cache, we take ownership of an additional reference. Release it here + # to prevent leaking memory. + # See https://developer.apple.com/documentation/foundation/nscopying. + if _returned_from_method.startswith(_RETURNS_RETAINED_PREFIXES): send_message(object_ptr, "release", restype=objc_id, argtypes=[]) return cached_obj From 84e3a9f4fb70c7a44dfa7106e8dbd7cdf08d2bb0 Mon Sep 17 00:00:00 2001 From: samschott Date: Wed, 27 Nov 2024 23:13:51 +0100 Subject: [PATCH 38/66] special handling for init --- src/rubicon/objc/api.py | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 1a0b1296..6affb40a 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -92,8 +92,8 @@ _keep_alive_objects = {} # Methods that return an object which is implicitly retained by the caller. -# See https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/MemoryMgmt/Articles/mmRules.html -_OWNERSHIP_METHOD_PREFIXES = (b"alloc", b"new", b"copy", b"mutableCopy") +# See https://clang.llvm.org/docs/AutomaticReferenceCounting.html#semantics-of-method-families. +_RETURNS_RETAINED_PREFIXES = (b"init", b"alloc", b"new", b"copy", b"mutableCopy") def encoding_from_annotation(f, offset=1): @@ -214,6 +214,16 @@ def __call__(self, receiver, *args, convert_args=True, convert_result=True): else: converted_args = args + # Init methods consume their `self` argument (the receiver), see + # https://clang.llvm.org/docs/AutomaticReferenceCounting.html#semantics-of-init. + # To avoid segfaults on garbage collection if `init` does not return `self` but + # a different object or None, we issue an additional retain. This needs to be + # done before calling the method. + # Note that if `init` does return the same object, it will already be in our + # cache and balanced with a `release` on cache retrieval. + if self.name.startswith(b"init"): + send_message(receiver, "retain", restype=objc_id, argtypes=[]) + result = send_message( receiver, self.selector, @@ -860,7 +870,7 @@ class or a metaclass, an instance of :class:`ObjCClass` or # Explicitly retain the instance on first handover to Python unless we # received it from a method that gives us ownership already. - if not _returned_from_method.startswith(_OWNERSHIP_METHOD_PREFIXES): + if not _returned_from_method.startswith(_RETURNS_RETAINED_PREFIXES): send_message(object_ptr, "retain", restype=objc_id, argtypes=[]) # If the given pointer points to a class, return an ObjCClass instead (if we're not already creating one). From ab46b9d9848d70c85802af1c156ce3b9f860bb68 Mon Sep 17 00:00:00 2001 From: samschott Date: Thu, 28 Nov 2024 01:00:20 +0100 Subject: [PATCH 39/66] add tests for init object change --- tests/test_core.py | 43 ++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 42 insertions(+), 1 deletion(-) diff --git a/tests/test_core.py b/tests/test_core.py index 39a3ba07..f62b14e5 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -55,12 +55,22 @@ send_super, types, ) -from rubicon.objc.runtime import autoreleasepool, get_ivar, libobjc, objc_id, set_ivar +from rubicon.objc.runtime import ( + autoreleasepool, + get_ivar, + libobjc, + load_library, + objc_id, + set_ivar, +) from rubicon.objc.types import __LP64__ from . import OSX_VERSION, rubiconharness +appkit = load_library("AppKit") + NSArray = ObjCClass("NSArray") +NSImage = ObjCClass("NSImage") NSString = ObjCClass("NSString") @@ -1937,6 +1947,37 @@ def test_objcinstance_immutable_copy_lifecycle(self): self.assertIsNone(wr.weak_property, "object was not deallocated") + def test_objcinstance_init_change_lifecycle(self): + """We do not leak memory if init returns a different object than it + received in alloc.""" + with autoreleasepool(): + obj_allocated = NSString.alloc() + obj_initialized = obj_allocated.initWithString(str(uuid.uuid4())) + + self.assertNotEqual(obj_allocated.ptr.value, obj_initialized.ptr.value) + + # Assign the object to an Obj-C weakref and delete it check that it is dealloced. + wr = ObjcWeakref.alloc().init() + wr.weak_property = obj_initialized + + with autoreleasepool(): + del obj_allocated, obj_initialized + gc.collect() + + self.assertIsNotNone( + wr.weak_property, + "object was deallocated before end of autorelease pool", + ) + + self.assertIsNone(wr.weak_property, "object was not deallocated") + + def test_objcinstance_init_none(self): + """We do segfault if init returns a different object than it received in alloc.""" + with autoreleasepool(): + image = NSImage.alloc().initWithContentsOfFile("/no/file/here") + + self.assertIsNone(image) + def test_objcinstance_dealloc(self): class DeallocTester(NSObject): From 23051224aa795f9f206b18fc6a4674ba7ac636d0 Mon Sep 17 00:00:00 2001 From: samschott Date: Thu, 28 Nov 2024 22:10:29 +0100 Subject: [PATCH 40/66] implement proper method family detection --- src/rubicon/objc/api.py | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 6affb40a..663e5eec 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -93,7 +93,18 @@ # Methods that return an object which is implicitly retained by the caller. # See https://clang.llvm.org/docs/AutomaticReferenceCounting.html#semantics-of-method-families. -_RETURNS_RETAINED_PREFIXES = (b"init", b"alloc", b"new", b"copy", b"mutableCopy") +_RETURNS_RETAINED_FAMILIES = {"init", "alloc", "new", "copy", "mutableCopy"} + + +def _get_method_family(method_name: str) -> str: + # See https://clang.llvm.org/docs/AutomaticReferenceCounting.html#method-families. + method_name = method_name.lstrip("_").split(":")[0] + leading_lowercases = [] + for c in method_name: + if c.isupper(): + break + leading_lowercases.append(c) + return "".join(leading_lowercases) def encoding_from_annotation(f, offset=1): @@ -221,7 +232,7 @@ def __call__(self, receiver, *args, convert_args=True, convert_result=True): # done before calling the method. # Note that if `init` does return the same object, it will already be in our # cache and balanced with a `release` on cache retrieval. - if self.name.startswith(b"init"): + if _get_method_family(self.name.decode()) == "init": send_message(receiver, "retain", restype=objc_id, argtypes=[]) result = send_message( @@ -849,6 +860,8 @@ class or a metaclass, an instance of :class:`ObjCClass` or if not object_ptr.value: return None + method_family = _get_method_family(_returned_from_method.decode()) + with ObjCInstance._instance_lock: try: # If an ObjCInstance already exists for the Objective-C object, @@ -861,7 +874,7 @@ class or a metaclass, an instance of :class:`ObjCClass` or # cache, we take ownership of an additional reference. Release it here # to prevent leaking memory. # See https://developer.apple.com/documentation/foundation/nscopying. - if _returned_from_method.startswith(_RETURNS_RETAINED_PREFIXES): + if method_family in _RETURNS_RETAINED_FAMILIES: send_message(object_ptr, "release", restype=objc_id, argtypes=[]) return cached_obj @@ -870,7 +883,7 @@ class or a metaclass, an instance of :class:`ObjCClass` or # Explicitly retain the instance on first handover to Python unless we # received it from a method that gives us ownership already. - if not _returned_from_method.startswith(_RETURNS_RETAINED_PREFIXES): + if method_family not in _RETURNS_RETAINED_FAMILIES: send_message(object_ptr, "retain", restype=objc_id, argtypes=[]) # If the given pointer points to a class, return an ObjCClass instead (if we're not already creating one). From 2e4eccb2edea49fa16a48672f305bcb0bd0472dc Mon Sep 17 00:00:00 2001 From: samschott Date: Thu, 28 Nov 2024 23:32:17 +0100 Subject: [PATCH 41/66] ensure partial methods are loaded from all superclasses --- src/rubicon/objc/api.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 663e5eec..3fa57199 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -1571,17 +1571,22 @@ def _load_methods(self): methods_ptr = libobjc.class_copyMethodList(self, byref(methods_ptr_count)) - if self.superclass is not None: - if self.superclass.methods_ptr is None: - with self.superclass.cache_lock: - self.superclass._load_methods() + # Traverse superclasses and load methods. + superclass = self.superclass + + while superclass is not None: + if superclass.methods_ptr is None: + with superclass.cache_lock: + superclass._load_methods() # Prime this class' partials list with a list from the superclass. - for first, superpartial in self.superclass.partial_methods.items(): + for first, superpartial in superclass.partial_methods.items(): partial = ObjCPartialMethod(first) self.partial_methods[first] = partial partial.methods.update(superpartial.methods) + superclass = superclass.superclass + for i in range(methods_ptr_count.value): method = methods_ptr[i] name = libobjc.method_getName(method).name.decode("utf-8") From 29895400e0d3fafa5d776f156df918f474a42e4b Mon Sep 17 00:00:00 2001 From: samschott Date: Fri, 29 Nov 2024 19:13:05 +0100 Subject: [PATCH 42/66] remove unneeded whitespace Co-authored-by: Russell Keith-Magee --- tests/test_core.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_core.py b/tests/test_core.py index f62b14e5..b3f70f27 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1416,7 +1416,7 @@ class Ivars(NSObject): def test_class_properties(self): """A Python class can have ObjC properties with synthesized getters and setters - of ObjCInstance type .""" + of ObjCInstance type.""" NSURL = ObjCClass("NSURL") From 0bc749cbf75791bdd14b3e8f07a537b2d5d6b9b4 Mon Sep 17 00:00:00 2001 From: samschott Date: Fri, 29 Nov 2024 19:12:26 +0100 Subject: [PATCH 43/66] improved release-on-cache-hit documentation --- src/rubicon/objc/api.py | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 3fa57199..79f62e54 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -869,11 +869,17 @@ class or a metaclass, an instance of :class:`ObjCClass` or # same object. cached_obj = cls._cached_objects[object_ptr.value] - # A `copy` call can return the original object if it is immutable. This - # is typically done for optimization. If this object is already in our - # cache, we take ownership of an additional reference. Release it here - # to prevent leaking memory. - # See https://developer.apple.com/documentation/foundation/nscopying. + # We can get a cache hit for methods that return an implicitly retained + # object. This is typically the case when: + # + # 1. A `copy` returns the original object if it is immutable. This is + # typically done for optimization. See + # https://developer.apple.com/documentation/foundation/nscopying. + # 2. An `init` call returns an object which we already own from a + # previous `alloc` call. See `init` handling in ObjCMethod. __call__. + # + # If the object is already in our cache, we end up owning more than one + # refcount. We release this additional refcount to prevent memory leaks. if method_family in _RETURNS_RETAINED_FAMILIES: send_message(object_ptr, "release", restype=objc_id, argtypes=[]) From 04981e35c85d35149f165bf501bb8775d01256f2 Mon Sep 17 00:00:00 2001 From: samschott Date: Fri, 29 Nov 2024 19:18:49 +0100 Subject: [PATCH 44/66] updated change notes --- changes/256.feature.rst | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/changes/256.feature.rst b/changes/256.feature.rst index 5028dcba..5430b4fc 100644 --- a/changes/256.feature.rst +++ b/changes/256.feature.rst @@ -1,10 +1,5 @@ Retain Objective-C objects when creating Python wrappers and release them when the Python wrapped is garbage collected. This means that manual ``retain`` calls and subsequent ``release`` or ``autorelease`` calls from Python are no longer needed with -very few exceptions such as: - -1. When implementing methods like ``copy`` that are supposed to create an object, if - the returned object is not actually newly created. -2. When dealing with side effects of methods like ``init`` that may release an object - which is still referenced from Python. See for example - https://github.com/beeware/toga/issues/2468. +very few exceptions, for example when writing implementations of ``copy`` that return an +existing object. From 54ed55c08b61959dfab5f725dae76c31b994e9ff Mon Sep 17 00:00:00 2001 From: samschott Date: Fri, 29 Nov 2024 19:33:31 +0100 Subject: [PATCH 45/66] add test for get_method_family --- src/rubicon/objc/api.py | 10 ++++++---- tests/test_core.py | 7 +++++++ 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 79f62e54..d6d85469 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -96,8 +96,10 @@ _RETURNS_RETAINED_FAMILIES = {"init", "alloc", "new", "copy", "mutableCopy"} -def _get_method_family(method_name: str) -> str: - # See https://clang.llvm.org/docs/AutomaticReferenceCounting.html#method-families. +def get_method_family(method_name: str) -> str: + """Returns the method family from the method name. See + https://clang.llvm.org/docs/AutomaticReferenceCounting.html#method-families for + documentation on method families and corresponding selector names.""" method_name = method_name.lstrip("_").split(":")[0] leading_lowercases = [] for c in method_name: @@ -232,7 +234,7 @@ def __call__(self, receiver, *args, convert_args=True, convert_result=True): # done before calling the method. # Note that if `init` does return the same object, it will already be in our # cache and balanced with a `release` on cache retrieval. - if _get_method_family(self.name.decode()) == "init": + if get_method_family(self.name.decode()) == "init": send_message(receiver, "retain", restype=objc_id, argtypes=[]) result = send_message( @@ -860,7 +862,7 @@ class or a metaclass, an instance of :class:`ObjCClass` or if not object_ptr.value: return None - method_family = _get_method_family(_returned_from_method.decode()) + method_family = get_method_family(_returned_from_method.decode()) with ObjCInstance._instance_lock: try: diff --git a/tests/test_core.py b/tests/test_core.py index b3f70f27..500fc573 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -55,6 +55,7 @@ send_super, types, ) +from rubicon.objc.api import get_method_family from rubicon.objc.runtime import ( autoreleasepool, get_ivar, @@ -2183,3 +2184,9 @@ def work(): thread.start() work() thread.join() + + def test_get_method_family(self): + self.assertEqual(get_method_family("perform"), "perform") + self.assertEqual(get_method_family("performWith:"), "perform") + self.assertEqual(get_method_family("_performWith:"), "perform") + self.assertEqual(get_method_family("_perform:with:"), "perform") From c6096c2869c754fcbfd0193e1f7a1d8b098e1c7a Mon Sep 17 00:00:00 2001 From: samschott Date: Fri, 29 Nov 2024 21:26:03 +0100 Subject: [PATCH 46/66] remove loop that breaks early on method loading --- src/rubicon/objc/api.py | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index d6d85469..b529163b 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -1003,6 +1003,7 @@ def __getattr__(self, name): # ObjCBoundMethod, so that it will be able to keep the ObjCInstance # alive for chained calls like MyClass.alloc().init() where the # object created by alloc() is not assigned to a variable. + method = None # If there's a property with this name; return the value directly. # If the name ends with _, we can shortcut this step, because it's @@ -1015,19 +1016,15 @@ def __getattr__(self, name): # See if there's a partial method starting with the given name, # either on self's class or any of the superclasses. cls = self.objc_class - while cls is not None: - # Load the class's methods if we haven't done so yet. - with cls.cache_lock: - if cls.methods_ptr is None: - cls._load_methods() + # Load the class's methods if we haven't done so yet. + with cls.cache_lock: + if cls.methods_ptr is None: + cls._load_methods() - try: - method = cls.partial_methods[name] - break - except KeyError: - cls = cls.superclass - else: - method = None + try: + method = cls.partial_methods[name] + except KeyError: + pass if method is None or set(method.methods) == {()}: # Find a method whose full name matches the given name if no partial From a28c901421eae41273b41b83719fdb101bbf7c3e Mon Sep 17 00:00:00 2001 From: samschott Date: Fri, 29 Nov 2024 22:25:01 +0100 Subject: [PATCH 47/66] make method loading slightly clearer --- src/rubicon/objc/api.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index b529163b..4d7508d0 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -1572,10 +1572,6 @@ def _load_methods(self): if self.methods_ptr is not None: raise RuntimeError(f"{self}._load_methods cannot be called more than once") - methods_ptr_count = c_uint(0) - - methods_ptr = libobjc.class_copyMethodList(self, byref(methods_ptr_count)) - # Traverse superclasses and load methods. superclass = self.superclass @@ -1592,6 +1588,10 @@ def _load_methods(self): superclass = superclass.superclass + # Load methods for this class. + methods_ptr_count = c_uint(0) + methods_ptr = libobjc.class_copyMethodList(self, byref(methods_ptr_count)) + for i in range(methods_ptr_count.value): method = methods_ptr[i] name = libobjc.method_getName(method).name.decode("utf-8") From 1b306e22ccda28a6c904f407a601f394c2832c0a Mon Sep 17 00:00:00 2001 From: samschott Date: Fri, 29 Nov 2024 22:52:38 +0100 Subject: [PATCH 48/66] extract and document method name to tuple logic --- src/rubicon/objc/api.py | 49 ++++++++++++++++++++++++++++------------- 1 file changed, 34 insertions(+), 15 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 4d7508d0..939cc823 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -109,6 +109,31 @@ def get_method_family(method_name: str) -> str: return "".join(leading_lowercases) +def method_name_to_tuple(name: str) -> (str, tuple[str, ...]): + """ + Performs the following transformation: + + "methodWithArg0:withArg1:withArg2" -> "methodWithArg0", ("", "withArg1", "withArg2") + "methodWithArg0:" -> "methodWithArg0", ("", ) + "method" -> "method", () + + The first element of the returned tuple is the "base name" of the method. The second + element is a tuple with its argument names. + """ + # Selectors end with a colon if the method takes arguments. + if name.endswith(":"): + first, *rest, _ = name.split(":") + # Insert an empty string in order to indicate that the method + # takes a first argument as a positional argument. + rest.insert(0, "") + rest = tuple(rest) + else: + first = name + rest = () + + return first, rest + + def encoding_from_annotation(f, offset=1): argspec = inspect.getfullargspec(inspect.unwrap(f)) hints = typing.get_type_hints(f) @@ -262,7 +287,10 @@ def __init__(self, name_start): super().__init__() self.name_start = name_start - self.methods = {} # Initialized in ObjCClass._load_methods + + # A dictionary mapping from a tuple of argument names to the full method name. + # Initialized in ObjCClass._load_methods + self.methods: dict[tuple[str, ...], str] = {} def __repr__(self): return f"{type(self).__qualname__}({self.name_start!r})" @@ -1597,24 +1625,15 @@ def _load_methods(self): name = libobjc.method_getName(method).name.decode("utf-8") self.instance_method_ptrs[name] = method - # Selectors end with a colon if the method takes arguments. - if name.endswith(":"): - first, *rest, _ = name.split(":") - # Insert an empty string in order to indicate that the method - # takes a first argument as a positional argument. - rest.insert(0, "") - rest = tuple(rest) - else: - first = name - rest = () + base_name, argument_names = method_name_to_tuple(name) try: - partial = self.partial_methods[first] + partial = self.partial_methods[base_name] except KeyError: - partial = ObjCPartialMethod(first) - self.partial_methods[first] = partial + partial = ObjCPartialMethod(base_name) + self.partial_methods[base_name] = partial - partial.methods[rest] = name + partial.methods[argument_names] = name # Set the list of methods for the class to the computed list. self.methods_ptr = methods_ptr From 849749eaa589c07e4bb0b7ec68df8e0aa54073db Mon Sep 17 00:00:00 2001 From: samschott Date: Fri, 29 Nov 2024 22:53:10 +0100 Subject: [PATCH 49/66] fall back to full method usage if partial method lookup fails --- src/rubicon/objc/api.py | 17 ++++++++++------- 1 file changed, 10 insertions(+), 7 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 939cc823..67349656 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -311,18 +311,21 @@ def __call__(self, receiver, first_arg=_sentinel, **kwargs): try: name = self.methods[rest] except KeyError: + # Reconstruct the full method name from arguments. if first_arg is self._sentinel: - specified_sel = self.name_start + name = self.name_start else: - specified_sel = f"{self.name_start}:{':'.join(kwargs.keys())}:" - raise ValueError( - f"Invalid selector {specified_sel}. Available selectors are: " - f"{', '.join(sel for sel in self.methods.values())}" - ) from None + name = f"{self.name_start}:{':'.join(kwargs.keys())}:" meth = receiver.objc_class._cache_method(name) - return meth(receiver, *args) + if meth: + return meth(receiver, *args) + + raise ValueError( + f"Invalid selector {name}. Available selectors are: " + f"{', '.join(sel for sel in self.methods.values())}" + ) from None class ObjCBoundMethod: From 15593c1dca4fa0f4d017d767906218bbebfec1b6 Mon Sep 17 00:00:00 2001 From: samschott Date: Fri, 29 Nov 2024 22:59:15 +0100 Subject: [PATCH 50/66] update partial method cache after successful lookup --- src/rubicon/objc/api.py | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 67349656..e2a5cb0c 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -308,18 +308,25 @@ def __call__(self, receiver, first_arg=_sentinel, **kwargs): args.insert(0, first_arg) rest = ("",) + order + # Try to use cached ObjCBoundMethod try: name = self.methods[rest] + meth = receiver.objc_class._cache_method(name) + return meth(receiver, *args) except KeyError: - # Reconstruct the full method name from arguments. - if first_arg is self._sentinel: - name = self.name_start - else: - name = f"{self.name_start}:{':'.join(kwargs.keys())}:" + pass + + # Reconstruct the full method name from arguments and look up actual method. + if first_arg is self._sentinel: + name = self.name_start + else: + name = f"{self.name_start}:{':'.join(kwargs.keys())}:" meth = receiver.objc_class._cache_method(name) if meth: + # Update methods cache and call method. + self.methods[rest] = name return meth(receiver, *args) raise ValueError( From 39c548e028da68f95effc093ff166afe4f3dab74 Mon Sep 17 00:00:00 2001 From: samschott Date: Sat, 30 Nov 2024 13:30:18 +0100 Subject: [PATCH 51/66] Revert "remove loop that breaks early on method loading" This reverts commit c6096c2869c754fcbfd0193e1f7a1d8b098e1c7a. --- src/rubicon/objc/api.py | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index e2a5cb0c..7e624d47 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -1041,7 +1041,6 @@ def __getattr__(self, name): # ObjCBoundMethod, so that it will be able to keep the ObjCInstance # alive for chained calls like MyClass.alloc().init() where the # object created by alloc() is not assigned to a variable. - method = None # If there's a property with this name; return the value directly. # If the name ends with _, we can shortcut this step, because it's @@ -1054,15 +1053,19 @@ def __getattr__(self, name): # See if there's a partial method starting with the given name, # either on self's class or any of the superclasses. cls = self.objc_class - # Load the class's methods if we haven't done so yet. - with cls.cache_lock: - if cls.methods_ptr is None: - cls._load_methods() + while cls is not None: + # Load the class's methods if we haven't done so yet. + with cls.cache_lock: + if cls.methods_ptr is None: + cls._load_methods() - try: - method = cls.partial_methods[name] - except KeyError: - pass + try: + method = cls.partial_methods[name] + break + except KeyError: + cls = cls.superclass + else: + method = None if method is None or set(method.methods) == {()}: # Find a method whose full name matches the given name if no partial From b78c41de8cbc1c405059e3072336ee9d50b548cf Mon Sep 17 00:00:00 2001 From: samschott Date: Sat, 30 Nov 2024 13:31:51 +0100 Subject: [PATCH 52/66] Revert "ensure partial methods are loaded from all superclasses" This reverts commit 2e4eccb2edea49fa16a48672f305bcb0bd0472dc. # Conflicts: # src/rubicon/objc/api.py --- src/rubicon/objc/api.py | 15 +++++---------- 1 file changed, 5 insertions(+), 10 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 7e624d47..6ed73658 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -1613,22 +1613,17 @@ def _load_methods(self): if self.methods_ptr is not None: raise RuntimeError(f"{self}._load_methods cannot be called more than once") - # Traverse superclasses and load methods. - superclass = self.superclass - - while superclass is not None: - if superclass.methods_ptr is None: - with superclass.cache_lock: - superclass._load_methods() + if self.superclass is not None: + if self.superclass.methods_ptr is None: + with self.superclass.cache_lock: + self.superclass._load_methods() # Prime this class' partials list with a list from the superclass. - for first, superpartial in superclass.partial_methods.items(): + for first, superpartial in self.superclass.partial_methods.items(): partial = ObjCPartialMethod(first) self.partial_methods[first] = partial partial.methods.update(superpartial.methods) - superclass = superclass.superclass - # Load methods for this class. methods_ptr_count = c_uint(0) methods_ptr = libobjc.class_copyMethodList(self, byref(methods_ptr_count)) From cb996ad3aff6c549afdbe3dcd090c11f28696929 Mon Sep 17 00:00:00 2001 From: samschott Date: Sat, 30 Nov 2024 15:58:07 +0100 Subject: [PATCH 53/66] Reapply "ensure partial methods are loaded from all superclasses" This reverts commit b78c41de8cbc1c405059e3072336ee9d50b548cf. --- src/rubicon/objc/api.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 6ed73658..7e624d47 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -1613,17 +1613,22 @@ def _load_methods(self): if self.methods_ptr is not None: raise RuntimeError(f"{self}._load_methods cannot be called more than once") - if self.superclass is not None: - if self.superclass.methods_ptr is None: - with self.superclass.cache_lock: - self.superclass._load_methods() + # Traverse superclasses and load methods. + superclass = self.superclass + + while superclass is not None: + if superclass.methods_ptr is None: + with superclass.cache_lock: + superclass._load_methods() # Prime this class' partials list with a list from the superclass. - for first, superpartial in self.superclass.partial_methods.items(): + for first, superpartial in superclass.partial_methods.items(): partial = ObjCPartialMethod(first) self.partial_methods[first] = partial partial.methods.update(superpartial.methods) + superclass = superclass.superclass + # Load methods for this class. methods_ptr_count = c_uint(0) methods_ptr = libobjc.class_copyMethodList(self, byref(methods_ptr_count)) From 6cf88a55c2dbc566a89b451dc73ecdfe285d09ce Mon Sep 17 00:00:00 2001 From: Sam Schott Date: Mon, 2 Dec 2024 23:45:04 +0100 Subject: [PATCH 54/66] centralize logic for method family --- src/rubicon/objc/api.py | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 7e624d47..006c218a 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -259,7 +259,8 @@ def __call__(self, receiver, *args, convert_args=True, convert_result=True): # done before calling the method. # Note that if `init` does return the same object, it will already be in our # cache and balanced with a `release` on cache retrieval. - if get_method_family(self.name.decode()) == "init": + method_family = get_method_family(self.name.decode()) + if method_family == "init": send_message(receiver, "retain", restype=objc_id, argtypes=[]) result = send_message( @@ -274,8 +275,11 @@ def __call__(self, receiver, *args, convert_args=True, convert_result=True): return result # Convert result to python type if it is an instance or class pointer. + # Explicitly retain the instance on first handover to Python unless we + # received it from a method that gives us ownership already. if self.restype is not None and issubclass(self.restype, objc_id): - result = ObjCInstance(result, _returned_from_method=self.name) + implicitly_owned = method_family in _RETURNS_RETAINED_FAMILIES + result = ObjCInstance(result, _implicitly_owned=implicitly_owned) return result @@ -860,7 +864,7 @@ def _associated_attr_key_for_name(name): return SEL(f"rubicon.objc.py_attr.{name}") def __new__( - cls, object_ptr, _name=None, _bases=None, _ns=None, _returned_from_method=b"" + cls, object_ptr, _name=None, _bases=None, _ns=None, _implicitly_owned=False ): """The constructor accepts an :class:`~rubicon.objc.runtime.objc_id` or anything that can be cast to one, such as a :class:`~ctypes.c_void_p`, @@ -900,8 +904,6 @@ class or a metaclass, an instance of :class:`ObjCClass` or if not object_ptr.value: return None - method_family = get_method_family(_returned_from_method.decode()) - with ObjCInstance._instance_lock: try: # If an ObjCInstance already exists for the Objective-C object, @@ -920,7 +922,7 @@ class or a metaclass, an instance of :class:`ObjCClass` or # # If the object is already in our cache, we end up owning more than one # refcount. We release this additional refcount to prevent memory leaks. - if method_family in _RETURNS_RETAINED_FAMILIES: + if _implicitly_owned: send_message(object_ptr, "release", restype=objc_id, argtypes=[]) return cached_obj @@ -929,7 +931,7 @@ class or a metaclass, an instance of :class:`ObjCClass` or # Explicitly retain the instance on first handover to Python unless we # received it from a method that gives us ownership already. - if method_family not in _RETURNS_RETAINED_FAMILIES: + if not _implicitly_owned: send_message(object_ptr, "retain", restype=objc_id, argtypes=[]) # If the given pointer points to a class, return an ObjCClass instead (if we're not already creating one). From aa48b5d95b2e0042178e54216c414b45ee904875 Mon Sep 17 00:00:00 2001 From: samschott Date: Wed, 4 Dec 2024 22:18:15 +0100 Subject: [PATCH 55/66] update test description Co-authored-by: Malcolm Smith --- tests/test_core.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_core.py b/tests/test_core.py index 500fc573..8e14db4f 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1973,7 +1973,7 @@ def test_objcinstance_init_change_lifecycle(self): self.assertIsNone(wr.weak_property, "object was not deallocated") def test_objcinstance_init_none(self): - """We do segfault if init returns a different object than it received in alloc.""" + """We do not segfault if init returns nil.""" with autoreleasepool(): image = NSImage.alloc().initWithContentsOfFile("/no/file/here") From 7fa9e718f4ac02b6d257e841daa24cf42ec2a92c Mon Sep 17 00:00:00 2001 From: samschott Date: Wed, 4 Dec 2024 22:19:43 +0100 Subject: [PATCH 56/66] update inline comments --- src/rubicon/objc/api.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 006c218a..3e6a5855 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -254,9 +254,9 @@ def __call__(self, receiver, *args, convert_args=True, convert_result=True): # Init methods consume their `self` argument (the receiver), see # https://clang.llvm.org/docs/AutomaticReferenceCounting.html#semantics-of-init. - # To avoid segfaults on garbage collection if `init` does not return `self` but - # a different object or None, we issue an additional retain. This needs to be - # done before calling the method. + # To ensure the receiver pointer remains valid if `init` does not return `self` + # but a different object or None, we issue an additional retain. This needs to + # be done before calling the method. # Note that if `init` does return the same object, it will already be in our # cache and balanced with a `release` on cache retrieval. method_family = get_method_family(self.name.decode()) From 3be21903208b8569e869921933de6f82522af555 Mon Sep 17 00:00:00 2001 From: samschott Date: Wed, 4 Dec 2024 22:39:22 +0100 Subject: [PATCH 57/66] fix method family detection --- src/rubicon/objc/api.py | 15 ++++++++------- tests/test_core.py | 10 ++++++---- 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 3e6a5855..b6f1a4cf 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -100,13 +100,14 @@ def get_method_family(method_name: str) -> str: """Returns the method family from the method name. See https://clang.llvm.org/docs/AutomaticReferenceCounting.html#method-families for documentation on method families and corresponding selector names.""" - method_name = method_name.lstrip("_").split(":")[0] - leading_lowercases = [] - for c in method_name: - if c.isupper(): - break - leading_lowercases.append(c) - return "".join(leading_lowercases) + first_component = method_name.lstrip("_").split(":")[0] + for family in _RETURNS_RETAINED_FAMILIES: + if first_component.startswith(family): + remainder = first_component.removeprefix(family) + if remainder == "" or remainder[0].isupper(): + return family + + return "" def method_name_to_tuple(name: str) -> (str, tuple[str, ...]): diff --git a/tests/test_core.py b/tests/test_core.py index 8e14db4f..8b30a3b3 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -2186,7 +2186,9 @@ def work(): thread.join() def test_get_method_family(self): - self.assertEqual(get_method_family("perform"), "perform") - self.assertEqual(get_method_family("performWith:"), "perform") - self.assertEqual(get_method_family("_performWith:"), "perform") - self.assertEqual(get_method_family("_perform:with:"), "perform") + self.assertEqual(get_method_family("mutableCopy"), "mutableCopy") + self.assertEqual(get_method_family("mutableCopy:"), "mutableCopy") + self.assertEqual(get_method_family("_mutableCopy:"), "mutableCopy") + self.assertEqual(get_method_family("_mutableCopy:with:"), "mutableCopy") + self.assertEqual(get_method_family("_mutableCopyWith:"), "mutableCopy") + self.assertEqual(get_method_family("_mutableCopying:"), "") From dded73ea30f543a1f5aac35341502f341b27b80a Mon Sep 17 00:00:00 2001 From: samschott Date: Wed, 4 Dec 2024 23:01:00 +0100 Subject: [PATCH 58/66] add test case for alloc without init --- tests/test_core.py | 31 +++++++++++++++++++++++++++---- 1 file changed, 27 insertions(+), 4 deletions(-) diff --git a/tests/test_core.py b/tests/test_core.py index 8b30a3b3..63841707 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1883,7 +1883,7 @@ def test_objcinstance_returned_lifecycle(self): # Check that the object is retained when we create the ObjCInstance. self.assertEqual(obj.retainCount(), 1, "object was not retained") - # Assign the object to an Obj-C weakref and delete it check that it is dealloced. + # Assign the object to an Obj-C weakref and delete it to check that it is dealloced. wr = ObjcWeakref.alloc().init() wr.weak_property = obj @@ -1907,7 +1907,7 @@ def test_objcinstance_owned_lifecycle(self): self.assertEqual(obj.retainCount(), 1, "object should be retained only once") - # Assign the object to an Obj-C weakref and delete it check that it is dealloced. + # Assign the object to an Obj-C weakref and delete it to check that it is dealloced. wr = ObjcWeakref.alloc().init() wr.weak_property = obj @@ -1933,7 +1933,7 @@ def test_objcinstance_immutable_copy_lifecycle(self): self.assertIs(obj0, obj1) self.assertIs(obj0, obj2) - # Assign the object to an Obj-C weakref and delete it check that it is dealloced. + # Assign the object to an Obj-C weakref and delete it to check that it is dealloced. wr = ObjcWeakref.alloc().init() wr.weak_property = obj0 @@ -1957,7 +1957,7 @@ def test_objcinstance_init_change_lifecycle(self): self.assertNotEqual(obj_allocated.ptr.value, obj_initialized.ptr.value) - # Assign the object to an Obj-C weakref and delete it check that it is dealloced. + # Assign the object to an Obj-C weakref and delete it to check that it is dealloced. wr = ObjcWeakref.alloc().init() wr.weak_property = obj_initialized @@ -1972,6 +1972,29 @@ def test_objcinstance_init_change_lifecycle(self): self.assertIsNone(wr.weak_property, "object was not deallocated") + def test_objcinstance_alloc_lifecycle(self): + """We properly retain and release objects that are allocated but never + initialized.""" + with autoreleasepool(): + obj_allocated = NSObject.alloc() + + self.assertEqual(obj_allocated.retainCount(), 1) + + # Assign the object to an Obj-C weakref and delete it to check that it is dealloced. + wr = ObjcWeakref.alloc().init() + wr.weak_property = obj_allocated + + with autoreleasepool(): + del obj_allocated + gc.collect() + + self.assertIsNotNone( + wr.weak_property, + "object was deallocated before end of autorelease pool", + ) + + self.assertIsNone(wr.weak_property, "object was not deallocated") + def test_objcinstance_init_none(self): """We do not segfault if init returns nil.""" with autoreleasepool(): From 199f8073a4aab8f0b763c720dcf11d500c1a8649 Mon Sep 17 00:00:00 2001 From: samschott Date: Wed, 4 Dec 2024 23:06:41 +0100 Subject: [PATCH 59/66] race free interpreter shutdown handling --- src/rubicon/objc/api.py | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index b6f1a4cf..15545bae 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -968,8 +968,12 @@ def __del__(self): # Autorelease our reference on garbage collection of the Python wrapper. We use # autorelease instead of release to allow ObjC to take ownership of an object when # it is returned from a factory method. - if send_message and objc_id: + try: send_message(self, "autorelease", restype=objc_id, argtypes=[]) + except (NameError, TypeError): + # Handle interpreter shutdown gracefully where send_message might be deleted + # (NameError) or set to None (TypeError). + pass def __str__(self): """Get a human-readable representation of ``self``. From d0961b45f7606fddcb5606f1f7f9af531aa37c20 Mon Sep 17 00:00:00 2001 From: samschott Date: Wed, 4 Dec 2024 23:08:34 +0100 Subject: [PATCH 60/66] black formatting --- src/rubicon/objc/api.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 15545bae..f3b666c8 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -968,12 +968,12 @@ def __del__(self): # Autorelease our reference on garbage collection of the Python wrapper. We use # autorelease instead of release to allow ObjC to take ownership of an object when # it is returned from a factory method. - try: + try: send_message(self, "autorelease", restype=objc_id, argtypes=[]) - except (NameError, TypeError): - # Handle interpreter shutdown gracefully where send_message might be deleted - # (NameError) or set to None (TypeError). - pass + except (NameError, TypeError): + # Handle interpreter shutdown gracefully where send_message might be deleted + # (NameError) or set to None (TypeError). + pass def __str__(self): """Get a human-readable representation of ``self``. From a8e91934eb87c31eb28545a3b6848b33894a3b03 Mon Sep 17 00:00:00 2001 From: samschott Date: Thu, 5 Dec 2024 00:09:20 +0100 Subject: [PATCH 61/66] more precise family determination to follow the exact rules laid out in ARC docs Co-authored-by: Malcolm Smith --- src/rubicon/objc/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index f3b666c8..4134a866 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -104,7 +104,7 @@ def get_method_family(method_name: str) -> str: for family in _RETURNS_RETAINED_FAMILIES: if first_component.startswith(family): remainder = first_component.removeprefix(family) - if remainder == "" or remainder[0].isupper(): + if remainder == "" or not remainder[0].islower(): return family return "" From 296cf83d28d934a54a8943450293ab645bf74ea8 Mon Sep 17 00:00:00 2001 From: samschott Date: Thu, 5 Dec 2024 00:11:01 +0100 Subject: [PATCH 62/66] update tests for method family determination to check for non-lowercase separators Co-authored-by: Malcolm Smith --- tests/test_core.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_core.py b/tests/test_core.py index 63841707..8c801ff9 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -2214,4 +2214,5 @@ def test_get_method_family(self): self.assertEqual(get_method_family("_mutableCopy:"), "mutableCopy") self.assertEqual(get_method_family("_mutableCopy:with:"), "mutableCopy") self.assertEqual(get_method_family("_mutableCopyWith:"), "mutableCopy") + self.assertEqual(get_method_family("_mutableCopy_with:"), "mutableCopy") self.assertEqual(get_method_family("_mutableCopying:"), "") From 93300f3815f54e166b5df0e3f100e714250fb7f5 Mon Sep 17 00:00:00 2001 From: samschott Date: Thu, 5 Dec 2024 00:25:18 +0100 Subject: [PATCH 63/66] fix typo in method_name_to_tuple doc string Co-authored-by: Malcolm Smith --- src/rubicon/objc/api.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/rubicon/objc/api.py b/src/rubicon/objc/api.py index 4134a866..a4349cfa 100644 --- a/src/rubicon/objc/api.py +++ b/src/rubicon/objc/api.py @@ -114,7 +114,7 @@ def method_name_to_tuple(name: str) -> (str, tuple[str, ...]): """ Performs the following transformation: - "methodWithArg0:withArg1:withArg2" -> "methodWithArg0", ("", "withArg1", "withArg2") + "methodWithArg0:withArg1:withArg2:" -> "methodWithArg0", ("", "withArg1", "withArg2") "methodWithArg0:" -> "methodWithArg0", ("", ) "method" -> "method", () From 2be945b3b9d93f960a0f7d8815231a19d6119ecf Mon Sep 17 00:00:00 2001 From: samschott Date: Thu, 5 Dec 2024 23:30:09 +0100 Subject: [PATCH 64/66] more exhaustive refcount tests --- tests/test_core.py | 168 ++++++++++++++++++++++++++------------------- 1 file changed, 96 insertions(+), 72 deletions(-) diff --git a/tests/test_core.py b/tests/test_core.py index 8c801ff9..e0fe40f7 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -22,6 +22,7 @@ ) from decimal import Decimal from enum import Enum +from typing import Callable from rubicon.objc import ( SEL, @@ -32,6 +33,7 @@ NSEdgeInsets, NSEdgeInsetsMake, NSMakeRect, + NSMutableArray, NSObject, NSObjectProtocol, NSPoint, @@ -91,6 +93,26 @@ class struct_large(Structure): _fields_ = [("x", c_char * 17)] +def assert_lifecycle( + test: unittest.TestCase, object_constructor: Callable[[], ObjCInstance] +) -> None: + obj = object_constructor() + + wr = ObjcWeakref.alloc().init() + wr.weak_property = obj + + with autoreleasepool(): + del obj + gc.collect() + + test.assertIsNotNone( + wr.weak_property, + "object was deallocated before end of autorelease pool", + ) + + test.assertIsNone(wr.weak_property, "object was not deallocated") + + class RubiconTest(unittest.TestCase): def test_sel_by_name(self): self.assertEqual(SEL(b"foobar").name, b"foobar") @@ -1898,102 +1920,104 @@ def test_objcinstance_returned_lifecycle(self): self.assertIsNone(wr.weak_property, "object was not deallocated") - def test_objcinstance_owned_lifecycle(self): - """An object is not additionally retained when we create it ourselves. It is - autoreleased when the ObjCInstance is garbage collected. + def test_objcinstance_alloc_lifecycle(self): + """We properly retain and release objects that are allocated but never + initialized.""" + + def create_object(): + with autoreleasepool(): + return NSObject.alloc() + + assert_lifecycle(self, create_object) + + def test_objcinstance_alloc_init_lifecycle(self): + """An object is not additionally retained when we create and initialize it + through an alloc().init() chain. It is autoreleased when the ObjCInstance is + garbage collected. """ - # Create an object which we own. Check that is has a retain count of 1. - obj = NSObject.alloc().init() - self.assertEqual(obj.retainCount(), 1, "object should be retained only once") + def create_object(): + return NSObject.alloc().init() - # Assign the object to an Obj-C weakref and delete it to check that it is dealloced. - wr = ObjcWeakref.alloc().init() - wr.weak_property = obj + assert_lifecycle(self, create_object) - with autoreleasepool(): - del obj - gc.collect() + def test_objcinstance_new_lifecycle(self): + """An object is not additionally retained when we create and initialize it with + a new call. It is autoreleased when the ObjCInstance is garbage collected. + """ - self.assertIsNotNone( - wr.weak_property, - "object was deallocated before end of autorelease pool", - ) + def create_object(): + return NSObject.new() - self.assertIsNone(wr.weak_property, "object was not deallocated") + assert_lifecycle(self, create_object) - def test_objcinstance_immutable_copy_lifecycle(self): - """If the same object is returned from multiple creation methods, it is still - freed on Python garbage collection.""" - with autoreleasepool(): - obj0 = NSString.stringWithString(str(uuid.uuid4())) - obj1 = obj0.copy() - obj2 = obj0.copy() + def test_objcinstance_copy_lifecycle(self): + """An object is not additionally retained when we create and initialize it with + a copy call. It is autoreleased when the ObjCInstance is garbage collected. + """ - self.assertIs(obj0, obj1) - self.assertIs(obj0, obj2) + def create_object(): + obj = NSMutableArray.alloc().init() + copy = obj.copy() - # Assign the object to an Obj-C weakref and delete it to check that it is dealloced. - wr = ObjcWeakref.alloc().init() - wr.weak_property = obj0 + # Check that the copy is a new object. + self.assertIsNot(obj, copy) + self.assertNotEqual(obj.ptr.value, copy.ptr.value) - with autoreleasepool(): - del obj0, obj1, obj2 - gc.collect() + return copy - self.assertIsNotNone( - wr.weak_property, - "object was deallocated before end of autorelease pool", - ) + assert_lifecycle(self, create_object) - self.assertIsNone(wr.weak_property, "object was not deallocated") + def test_objcinstance_mutable_copy_lifecycle(self): + """An object is not additionally retained when we create and initialize it with + a mutableCopy call. It is autoreleased when the ObjCInstance is garbage collected. + """ - def test_objcinstance_init_change_lifecycle(self): - """We do not leak memory if init returns a different object than it - received in alloc.""" - with autoreleasepool(): - obj_allocated = NSString.alloc() - obj_initialized = obj_allocated.initWithString(str(uuid.uuid4())) + def create_object(): + obj = NSMutableArray.alloc().init() + copy = obj.mutableCopy() - self.assertNotEqual(obj_allocated.ptr.value, obj_initialized.ptr.value) + # Check that the copy is a new object. + self.assertIsNot(obj, copy) + self.assertNotEqual(obj.ptr.value, copy.ptr.value) - # Assign the object to an Obj-C weakref and delete it to check that it is dealloced. - wr = ObjcWeakref.alloc().init() - wr.weak_property = obj_initialized + return copy - with autoreleasepool(): - del obj_allocated, obj_initialized - gc.collect() + assert_lifecycle(self, create_object) - self.assertIsNotNone( - wr.weak_property, - "object was deallocated before end of autorelease pool", - ) + def test_objcinstance_immutable_copy_lifecycle(self): + """If the same object is returned from multiple creation methods, it is still + freed on Python garbage collection.""" - self.assertIsNone(wr.weak_property, "object was not deallocated") + def create_object(): + with autoreleasepool(): + obj = NSString.stringWithString(str(uuid.uuid4())) + copy = obj.copy() - def test_objcinstance_alloc_lifecycle(self): - """We properly retain and release objects that are allocated but never - initialized.""" - with autoreleasepool(): - obj_allocated = NSObject.alloc() + # Check that the copy the same object as the original. + self.assertIs(obj, copy) + self.assertEqual(obj.ptr.value, copy.ptr.value) - self.assertEqual(obj_allocated.retainCount(), 1) + return obj - # Assign the object to an Obj-C weakref and delete it to check that it is dealloced. - wr = ObjcWeakref.alloc().init() - wr.weak_property = obj_allocated + assert_lifecycle(self, create_object) - with autoreleasepool(): - del obj_allocated - gc.collect() + def test_objcinstance_init_change_lifecycle(self): + """We do not leak memory if init returns a different object than it + received in alloc.""" - self.assertIsNotNone( - wr.weak_property, - "object was deallocated before end of autorelease pool", - ) + def create_object(): + with autoreleasepool(): + obj_allocated = NSString.alloc() + obj_initialized = obj_allocated.initWithString(str(uuid.uuid4())) - self.assertIsNone(wr.weak_property, "object was not deallocated") + # Check that the initialized object is a different one than the allocated. + self.assertIsNot(obj_allocated, obj_initialized) + self.assertNotEqual(obj_allocated.ptr.value, obj_initialized.ptr.value) + + return obj_initialized + + assert_lifecycle(self, create_object) def test_objcinstance_init_none(self): """We do not segfault if init returns nil.""" From 2387c02787bfbfdedee2674b0618b3baa0fe2f18 Mon Sep 17 00:00:00 2001 From: samschott Date: Thu, 5 Dec 2024 23:39:07 +0100 Subject: [PATCH 65/66] remove duplicate code from test_objcinstance_returned_lifecycle --- tests/test_core.py | 28 ++++++---------------------- 1 file changed, 6 insertions(+), 22 deletions(-) diff --git a/tests/test_core.py b/tests/test_core.py index e0fe40f7..1b5e273b 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1894,31 +1894,15 @@ class TestO: self.assertIsNone(wr_python_object()) def test_objcinstance_returned_lifecycle(self): - """An object is retained when creating an ObjCInstance for it and autoreleased - when the ObjCInstance is garbage collected. + """An object is retained when creating an ObjCInstance for it without implicit + ownership It is autoreleased when the ObjCInstance is garbage collected. """ - with autoreleasepool(): - # Return an object which we don't own. Using str(uuid) here instead of a - # common string ensure that we get have the only reference. - obj = NSString.stringWithString(str(uuid.uuid4())) - - # Check that the object is retained when we create the ObjCInstance. - self.assertEqual(obj.retainCount(), 1, "object was not retained") - - # Assign the object to an Obj-C weakref and delete it to check that it is dealloced. - wr = ObjcWeakref.alloc().init() - wr.weak_property = obj - with autoreleasepool(): - del obj - gc.collect() - - self.assertIsNotNone( - wr.weak_property, - "object was deallocated before end of autorelease pool", - ) + def create_object(): + with autoreleasepool(): + return NSString.stringWithString(str(uuid.uuid4())) - self.assertIsNone(wr.weak_property, "object was not deallocated") + assert_lifecycle(self, create_object) def test_objcinstance_alloc_lifecycle(self): """We properly retain and release objects that are allocated but never From 51ba75bdbfad1e83ff9fee6f9de5789b145c0ebd Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Thu, 5 Dec 2024 23:34:14 +0000 Subject: [PATCH 66/66] Fix typo --- tests/test_core.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_core.py b/tests/test_core.py index 1b5e273b..47454c5a 100644 --- a/tests/test_core.py +++ b/tests/test_core.py @@ -1895,7 +1895,7 @@ class TestO: def test_objcinstance_returned_lifecycle(self): """An object is retained when creating an ObjCInstance for it without implicit - ownership It is autoreleased when the ObjCInstance is garbage collected. + ownership. It is autoreleased when the ObjCInstance is garbage collected. """ def create_object():