[Python] Set memory policy to "strict"

The ROOT Python interfaces have many memory leaks, which is a major pain
point for people using it for long-running scripts in batch jobs.

One source of memory leaks was indentified to be the "heuristic memory
policy" of cppyy. This means that cppyy assumes that every non-const
pointer member function argument was interpreted as the object taking
ownership if the argument.

For examle, take the non-owning RooLinkedList container. It has a
`RooLinkedList::Add(RooAbsArg *arg)` method. ROOT wrongly assumes that
this means the RooLinkedList takes ownership of arg, and it drops the
ROOT overship. Nobody feels responsible for deleting the object
anymore, and there is a memory leak or `arg`.

That particular leak was reported in this forum post:
https://root-forum.cern.ch/t/memory-leak-in-fits/56249

Function parameters of type `T *` are very common in ROOT, and only
rarely do they imply ownership transfer. So changing the memory policy
to "strict" would surely fix also many other memory leaks that are not
reported so far. In fact, upstream cppyy doesn't even have this
heuristic memory policy anymore! So moving ROOT also to the strict
memory policy closes the gap between ROOT and cppyy.

The potential drawback of this change are crashes in usercode if memory
is not properly managed. But these problems should either be fixed by:

  * the user

  * dedicated pythonizations for these methods to manage shared
    ownership via Python reference counters (i.e., setting the parameter
    as an attribute of the object that the member function was called
    on)

This follows up on PR #4294, in particular it reverts 3a12063.

Author: guitargeek

README/ReleaseNotes/v640/index.md

@@ -39,7 +39,7 @@ The following people have contributed to this new version:
 ## Removals
 
 * The `TH1K` class was removed. `TMath::KNNDensity` can be used in its stead.
-* The `TObject` equality operator pythonization (`TObject.__eq__`) that was deprecated in ROOT 6.38 and scheduled for removal in ROOT 6.40 is removed.
+* The `TObject` equality operator Pythonization (`TObject.__eq__`) that was deprecated in ROOT 6.38 and scheduled for removal in ROOT 6.40 is removed.
 * Comparing C++ `nullptr` objects with `None` in Python now raises a `TypeError`, as announced in the ROOT 6.38 release notes. Use truth-value checks like `if not x` or `x is None` instead.
 * The `TGLIncludes.h` and `TGLWSIncludes.h` that were deprecated in ROOT 6.38 and scheduled for removal are gone now. Please include your required headers like `<GL/gl.h>` or `<GL/glu.h>` directly.
 * The GLEW headers (`GL/eglew.h`, `GL/glew.h`, `GL/glxew.h`, and `GL/wglew.h`) that were installed when building ROOT with `builtin_glew=ON` are no longer installed. This is done because ROOT is moving away from GLEW for loading OpenGL extensions.
@@ -113,6 +113,60 @@ As part of this migration, the following build options are deprecated. From ROOT
 
 ROOT dropped support for Python 3.9, meaning ROOT now requires at least Python 3.10.
 
+### Changed ownership policy for non-`const` pointer member function parameters
+
+If you have a member function taking a raw pointer, like `MyClass::foo(T *obj)`,
+calling such method on a Python object `my_instance` of type `MyClass`
+would assume that the memory ownership of `obj` transfers to `my_instance`.
+
+However, this resulted in many memory leaks, since many functions with such a
+signature actually don't take ownership of the object.
+
+Now, the Python interface of ROOT will not make this assumption anymore.
+Because of this change, some double-deletes or dangling references
+might creep up in your scripts. These need to be fixed by properly managing
+object lifetime with Python references.
+
+You can fix the dangling references problem for example via:
+
+  1. Assigning the object to a Python variable
+  2. Creating an owning collection that keeps the objects alive
+  3. Writing a Pythonization for the member function that transfers the ownership if needed
+
+The double-delete problems can be fixed via:
+
+  1. Dropping the ownership on the Python side with `ROOT.SetOwnership(obj, False)`
+  3. Writing a Pythonization for the member function that drops the ownership on the Python side as above
+
+This affects for example the `TList::Add(TObject *obj)` member function, which
+will not transfer ownership from PyROOT to the TList anymore. The new policy
+fixes a memory leak, but at the same time it is not possible anymore to create
+the contained elements in place:
+
+```python
+# A TList is by default a non-owning container
+my_list = ROOT.TList()
+
+
+# This is working, but resulted in memory leak prior to ROOT 6.40:
+obj_1 = ROOT.TObjString("obj_1")
+my_list.Add(obj_1)
+
+
+# This is not allowed anymore, as the temporary would be
+# deleted immediately leaving a dangling pointer:
+my_list.Add(ROOT.TObjString("obj_2"))
+
+# Python reference count to contained object is now zero,
+# TList contains dangling pointer!
+```
+
+**Note:** You can change back to the old policy by calling
+`ROOT.SetMemoryPolicy(ROOT.kMemoryHeuristics)` after importing ROOT, but this
+should be only used for debugging purposes and this function might be removed
+in the future!
+
+
 ## Command-line utilities
 
 ## JavaScript ROOT

bindings/pyroot/pythonizations/CMakeLists.txt

@@ -38,6 +38,7 @@ if(roofit)
         ROOT/_pythonization/_roofit/_roojsonfactorywstool.py
         ROOT/_pythonization/_roofit/_roomcstudy.py
         ROOT/_pythonization/_roofit/_roomsgservice.py
+        ROOT/_pythonization/_roofit/_rooplot.py
         ROOT/_pythonization/_roofit/_rooprodpdf.py
         ROOT/_pythonization/_roofit/_roorealvar.py
         ROOT/_pythonization/_roofit/_roosimultaneous.py

bindings/pyroot/pythonizations/python/ROOT/_facade.py

@@ -192,11 +192,6 @@ def _finalSetup(self):
         if not self.gROOT.IsBatch() and self.PyConfig.StartGUIThread:
             self.app.init_graphics()
 
-        # Set memory policy to kUseHeuristics.
-        # This restores the default in PyROOT which was changed
-        # by new Cppyy
-        self.SetHeuristicMemoryPolicy(True)
-
         # The automatic conversion of ordinary obejcts to smart pointers is
         # disabled for ROOT because it can cause trouble with overload
         # resolution. If a function has overloads for both ordinary objects and

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_memory_utils.py

@@ -63,3 +63,28 @@ def _SetDirectory_SetOwnership(self, dir):
         import ROOT
 
         ROOT.SetOwnership(self, False)
+
+
+def declare_cpp_owned_arg(position, name, positional_args, keyword_args, condition=lambda _: True):
+    """
+    Helper function to drop Python ownership of a specific function argument
+    with a given position and name, referring to the C++ signature.
+
+    positional_args and keyword_args should be the usual args and kwargs passed
+    to the function, and condition is an optional condition on which the Python
+    ownership is dropped.
+    """
+    import ROOT
+
+    # has to match the C++ argument name
+    if name in keyword_args:
+        arg = keyword_args[name]
+    elif len(positional_args) > position:
+        arg = positional_args[0]
+    else:
+        # This can happen if the function was called with too few arguments.
+        # arguments. In that case we should not do anything.
+        return
+
+    if condition(arg):
+        ROOT.SetOwnership(arg, False)

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/__init__.py

@@ -40,6 +40,7 @@
 from ._roojsonfactorywstool import RooJSONFactoryWSTool
 from ._roomcstudy import RooMCStudy
 from ._roomsgservice import RooMsgService
+from ._rooplot import RooPlot
 from ._rooprodpdf import RooProdPdf
 from ._roorealvar import RooRealVar
 from ._roosimultaneous import RooSimultaneous
@@ -69,6 +70,7 @@
     RooJSONFactoryWSTool,
     RooMCStudy,
     RooMsgService,
+    RooPlot,
     RooProdPdf,
     RooRealVar,
     RooSimultaneous,

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/_rooabspdf.py

@@ -31,13 +31,19 @@ def _pack_cmd_args(*args, **kwargs):
     assert len(kwargs) == 0
 
     # Put RooCmdArgs in a RooLinkedList
-    cmdList = ROOT.RooLinkedList()
+    cmd_list = ROOT.RooLinkedList()
     for cmd in args:
         if not isinstance(cmd, ROOT.RooCmdArg):
             raise TypeError("This function only takes RooFit command arguments.")
-        cmdList.Add(cmd)
+        cmd_list.Add(cmd)
 
-    return cmdList
+    # The RooLinkedList passed to functions like fitTo() is expected to be
+    # non-owning. To make sure that the RooCmdArgs live long enough, we attach
+    # then as an attribute of the output list, such that the Python reference
+    # counter doesn't hit zero.
+    cmd_list._owning_pylist = args
+
+    return cmd_list
 
 
 class RooAbsPdf(RooAbsReal):

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_roofit/_rooplot.py

@@ -0,0 +1,20 @@
+# Authors:
+# * Jonas Rembser 09/2023
+
+################################################################################
+# Copyright (C) 1995-2020, Rene Brun and Fons Rademakers.                      #
+# All rights reserved.                                                         #
+#                                                                              #
+# For the licensing terms see $ROOTSYS/LICENSE.                                #
+# For the list of contributors see $ROOTSYS/README/CREDITS.                    #
+################################################################################
+
+
+class RooPlot(object):
+    def addObject(self, *args, **kwargs):
+        from ROOT._pythonization._memory_utils import declare_cpp_owned_arg
+
+        # Python should transfer the ownership to the RooPlot
+        declare_cpp_owned_arg(0, "obj", args, kwargs)
+
+        return self._addObject(*args, **kwargs)

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tcollection.py

@@ -93,11 +93,26 @@ def _iter_pyz(self):
         yield o
 
 
+def _TCollection_Add(self, *args, **kwargs):
+    from ROOT._pythonization._memory_utils import declare_cpp_owned_arg
+
+    def condition(_):
+        return self.IsOwner()
+
+    declare_cpp_owned_arg(0, "obj", args, kwargs, condition=condition)
+
+    self._Add(*args, **kwargs)
+
+
 @pythonization('TCollection')
 def pythonize_tcollection(klass):
     # Parameters:
     # klass: class to be pythonized
 
+    # Pythonize Add()
+    klass._Add = klass.Add
+    klass.Add = _TCollection_Add
+
     # Add Python lists methods
     klass.append = klass.Add
     klass.remove = _remove_pyz

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tgraph.py

@@ -181,6 +181,9 @@
 
 import cppyy
 
+from . import pythonization
+
+
 def set_size(self, buf):
     # Parameters:
     # - self: graph object
@@ -213,3 +216,26 @@ def set_size(self, buf):
 # Add the composite to the list of pythonizors
 cppyy.py.add_pythonization(comp)
 
+def _TMultiGraph_Add(self, *args, **kwargs):
+    """
+    The TMultiGraph takes ownership of the added graphs, unless we're using the
+    Add(TMultiGraph*) overload, in which cases it adopts the graphs inside the
+    other TMultiGraph.
+    """
+    from ROOT._pythonization._memory_utils import declare_cpp_owned_arg
+
+    def condition(gr):
+        import ROOT
+
+        return gr and not isinstance(gr, ROOT.TMultiGraph)
+
+    declare_cpp_owned_arg(0, "graph", args, kwargs, condition=condition)
+
+    self._Add(*args, **kwargs)
+
+
+@pythonization("TMultiGraph")
+def pythonize_tmultigraph(klass):
+    # Pythonizations for TMultiGraph::Add
+    klass._Add = klass.Add
+    klass.Add = _TMultiGraph_Add

bindings/pyroot/pythonizations/python/ROOT/_pythonization/_tseqcollection.py

@@ -200,7 +200,10 @@ def _reverse_pyz(self):
         return
 
     t = tuple(self)
+    was_owner = self.IsOwner()
+    self.SetOwner(False)
     self.Clear()
+    self.SetOwner(was_owner)
     for elem in t:
         self.AddAt(elem, 0)
 
@@ -222,7 +225,10 @@ def _sort_pyz(self, *args, **kwargs):
         # Sort in a Python list copy
         pylist = list(self)
         pylist.sort(*args, **kwargs)
+        was_owner = self.IsOwner()
+        self.SetOwner(False)
         self.Clear()
+        self.SetOwner(was_owner)
         self.extend(pylist)
 
 
@@ -241,11 +247,30 @@ def _index_pyz(self, val):
     return idx
 
 
+def _TSeqCollection_AddAt(self, *args, **kwargs):
+    from ROOT._pythonization._memory_utils import declare_cpp_owned_arg
+
+    def condition(_):
+        return self.IsOwner()
+
+    declare_cpp_owned_arg(0, "obj", args, kwargs, condition=condition)
+
+    self._AddAt(*args, **kwargs)
+
+
 @pythonization('TSeqCollection')
 def pythonize_tseqcollection(klass):
+    from ROOT._pythonization._tcollection import _TCollection_Add
+
     # Parameters:
     # klass: class to be pythonized
 
+    # Pythonize Add() methods
+    klass._Add = klass.Add
+    klass.Add = _TCollection_Add
+    klass._AddAt = klass.AddAt
+    klass.AddAt = _TSeqCollection_AddAt
+
     # Item access methods
     klass.__getitem__ = _getitem_pyz
     klass.__setitem__ = _setitem_pyz
@@ -257,3 +282,17 @@ def pythonize_tseqcollection(klass):
     klass.reverse = _reverse_pyz
     klass.sort    = _sort_pyz
     klass.index   = _index_pyz
+
+
+@pythonization("TList")
+def pythonize_tlist(klass):
+    from ROOT._pythonization._tcollection import _TCollection_Add
+
+    # Parameters:
+    # klass: class to be pythonized
+
+    # Pythonize Add() methods
+    klass._Add = klass.Add
+    klass.Add = _TCollection_Add
+    klass._AddAt = klass.AddAt
+    klass.AddAt = _TSeqCollection_AddAt