Updated memory management

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.

src/rubicon/objc/api.py

@@ -91,9 +91,47 @@
 # 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")
+# 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_FAMILIES = {"init", "alloc", "new", "copy", "mutableCopy"}
+
+
+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)
+
+
+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):
@@ -214,6 +252,17 @@ 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.
+        method_family = get_method_family(self.name.decode())
+        if method_family == "init":
+            send_message(receiver, "retain", restype=objc_id, argtypes=[])
+
         result = send_message(
             receiver,
             self.selector,
@@ -226,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
 
@@ -239,7 +291,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})"
@@ -257,21 +312,31 @@ 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:
-            if first_arg is self._sentinel:
-                specified_sel = 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
+            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)
 
-        return meth(receiver, *args)
+        if meth:
+            # Update methods cache and call method.
+            self.methods[rest] = name
+            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:
@@ -772,8 +837,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`."""
@@ -801,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`,
@@ -848,11 +911,18 @@ 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):
+                # 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 _implicitly_owned:
                     send_message(object_ptr, "release", restype=objc_id, argtypes=[])
 
                 return cached_obj
@@ -861,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 not _returned_from_method.startswith(_OWNERSHIP_METHOD_PREFIXES):
+            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).
@@ -897,7 +967,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``.
@@ -1544,44 +1615,40 @@ 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)
+        # Traverse superclasses and load methods.
+        superclass = self.superclass
 
-        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()
+        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))
+
         for i in range(methods_ptr_count.value):
             method = methods_ptr[i]
             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