bpo-44881: Integrate GC untrack into trashcan begin.

Include/cpython/object.h

@@ -469,9 +469,6 @@ function with a pair of macros:
 static void
 mytype_dealloc(mytype *p)
 {
-    ... declarations go here ...
-
-    PyObject_GC_UnTrack(p);        // must untrack first
     Py_TRASHCAN_BEGIN(p, mytype_dealloc)
     ... The body of the deallocator goes here, including all calls ...
     ... to Py_DECREF on contained objects.                         ...

Misc/NEWS.d/next/Core and Builtins/2021-08-10-14-46-10.bpo-44881.M1bcMW.rst

@@ -0,0 +1,9 @@
+Change `Py_TRASHCAN_BEGIN()` to automatically call `PyObject_GC_UnTrack()`.
+Previously, the untrack function must be explicitly called before
+`Py_TRASHCAN_BEGIN()`. There have been a number of bugs over the years
+related to not doing this particular dance just right.  This change makes it
+harder to do things incorrectly.  That avoids some hard to find bugs (e.g.
+only triggered when object nesting gets deep enough). Extensions that still
+call `PyObject_GC_UnTrack()` explictly will still work correctly but the
+call is redundant after this change.  It would still be needed for the
+extension to work correctly with both older and newer versions of Python.

Modules/_elementtree.c

@@ -664,8 +664,6 @@ element_gc_clear(ElementObject *self)
 static void
 element_dealloc(ElementObject* self)
 {
-    /* bpo-31095: UnTrack is needed before calling any callbacks */
-    PyObject_GC_UnTrack(self);
     Py_TRASHCAN_BEGIN(self, element_dealloc)
 
     if (self->weakreflist != NULL)

Objects/descrobject.c

@@ -1265,7 +1265,6 @@ typedef struct {
 static void
 wrapper_dealloc(wrapperobject *wp)
 {
-    PyObject_GC_UnTrack(wp);
     Py_TRASHCAN_BEGIN(wp, wrapper_dealloc)
     Py_XDECREF(wp->descr);
     Py_XDECREF(wp->self);

Objects/dictobject.c

@@ -1924,13 +1924,11 @@ _PyDict_FromKeys(PyObject *cls, PyObject *iterable, PyObject *value)
 static void
 dict_dealloc(PyDictObject *mp)
 {
+    Py_TRASHCAN_BEGIN(mp, dict_dealloc)
     PyObject **values = mp->ma_values;
     PyDictKeysObject *keys = mp->ma_keys;
     Py_ssize_t i, n;
 
-    /* bpo-31095: UnTrack is needed before calling any callbacks */
-    PyObject_GC_UnTrack(mp);
-    Py_TRASHCAN_BEGIN(mp, dict_dealloc)
     if (values != NULL) {
         if (values != empty_values) {
             for (i = 0, n = mp->ma_keys->dk_nentries; i < n; i++) {

Objects/frameobject.c

@@ -615,10 +615,6 @@ static PyGetSetDef frame_getsetlist[] = {
 static void _Py_HOT_FUNCTION
 frame_dealloc(PyFrameObject *f)
 {
-    if (_PyObject_GC_IS_TRACKED(f)) {
-        _PyObject_GC_UNTRACK(f);
-    }
-
     Py_TRASHCAN_BEGIN(f, frame_dealloc);
     PyCodeObject *co = NULL;
 

Objects/listobject.c

@@ -330,9 +330,8 @@ PyList_Append(PyObject *op, PyObject *newitem)
 static void
 list_dealloc(PyListObject *op)
 {
-    Py_ssize_t i;
-    PyObject_GC_UnTrack(op);
     Py_TRASHCAN_BEGIN(op, list_dealloc)
+    Py_ssize_t i;
     if (op->ob_item != NULL) {
         /* Do it backwards, for Christian Tismer.
            There's a simple test case where somehow this reduces

Objects/object.c

@@ -2101,6 +2101,11 @@ _PyTrash_thread_deposit_object(PyObject *op)
 {
     PyThreadState *tstate = _PyThreadState_GET();
     _PyObject_ASSERT(op, _PyObject_IS_GC(op));
+    /* untrack is done by _PyTrash_begin() so the following assert must be
+     * true.  Previously _PyTrash_begin() did not untrack itself and it was the
+     * responsibility of the users of the trashcan mechanism to ensure that
+     * untrack was called first.
+     */
     _PyObject_ASSERT(op, !_PyObject_GC_IS_TRACKED(op));
     _PyObject_ASSERT(op, Py_REFCNT(op) == 0);
     _PyGCHead_SET_PREV(_Py_AS_GC(op), tstate->trash_delete_later);
@@ -2150,6 +2155,10 @@ _PyTrash_thread_destroy_chain(void)
 int
 _PyTrash_begin(PyThreadState *tstate, PyObject *op)
 {
+    _PyObject_ASSERT(op, _PyObject_IS_GC(op));
+    if (_PyObject_GC_IS_TRACKED(op)) {
+        _PyObject_GC_UNTRACK(op);
+    }
     if (tstate->trash_delete_nesting >= _PyTrash_UNWIND_LEVEL) {
         /* Store the object (to be deallocated later) and jump past
          * Py_TRASHCAN_END, skipping the body of the deallocator */

Objects/odictobject.c

@@ -1365,7 +1365,6 @@ static PyGetSetDef odict_getset[] = {
 static void
 odict_dealloc(PyODictObject *self)
 {
-    PyObject_GC_UnTrack(self);
     Py_TRASHCAN_BEGIN(self, odict_dealloc)
 
     Py_XDECREF(self->od_inst_dict);

Objects/setobject.c

@@ -482,12 +482,10 @@ set_next(PySetObject *so, Py_ssize_t *pos_ptr, setentry **entry_ptr)
 static void
 set_dealloc(PySetObject *so)
 {
+    Py_TRASHCAN_BEGIN(so, set_dealloc)
     setentry *entry;
     Py_ssize_t used = so->used;
 
-    /* bpo-31095: UnTrack is needed before calling any callbacks */
-    PyObject_GC_UnTrack(so);
-    Py_TRASHCAN_BEGIN(so, set_dealloc)
     if (so->weakreflist != NULL)
         PyObject_ClearWeakRefs((PyObject *) so);
 

Objects/tupleobject.c

@@ -262,9 +262,8 @@ PyTuple_Pack(Py_ssize_t n, ...)
 static void
 tupledealloc(PyTupleObject *op)
 {
-    Py_ssize_t len =  Py_SIZE(op);
-    PyObject_GC_UnTrack(op);
     Py_TRASHCAN_BEGIN(op, tupledealloc)
+    Py_ssize_t len =  Py_SIZE(op);
     if (len > 0) {
         Py_ssize_t i = len;
         while (--i >= 0) {

Objects/typeobject.c

@@ -1381,9 +1381,6 @@ subtype_dealloc(PyObject *self)
 
     /* We get here only if the type has GC */
 
-    /* UnTrack and re-Track around the trashcan macro, alas */
-    /* See explanation at end of function for full disclosure */
-    PyObject_GC_UnTrack(self);
     Py_TRASHCAN_BEGIN(self, subtype_dealloc);
 
     /* Find the nearest base with a different tp_dealloc */
@@ -1487,43 +1484,6 @@ subtype_dealloc(PyObject *self)
 
   endlabel:
     Py_TRASHCAN_END
-
-    /* Explanation of the weirdness around the trashcan macros:
-
-       Q. What do the trashcan macros do?
-
-       A. Read the comment titled "Trashcan mechanism" in object.h.
-          For one, this explains why there must be a call to GC-untrack
-          before the trashcan begin macro.      Without understanding the
-          trashcan code, the answers to the following questions don't make
-          sense.
-
-       Q. Why do we GC-untrack before the trashcan and then immediately
-          GC-track again afterward?
-
-       A. In the case that the base class is GC-aware, the base class
-          probably GC-untracks the object.      If it does that using the
-          UNTRACK macro, this will crash when the object is already
-          untracked.  Because we don't know what the base class does, the
-          only safe thing is to make sure the object is tracked when we
-          call the base class dealloc.  But...  The trashcan begin macro
-          requires that the object is *untracked* before it is called.  So
-          the dance becomes:
-
-         GC untrack
-         trashcan begin
-         GC track
-
-       Q. Why did the last question say "immediately GC-track again"?
-          It's nowhere near immediately.
-
-       A. Because the code *used* to re-track immediately.      Bad Idea.
-          self has a refcount of 0, and if gc ever gets its hands on it
-          (which can happen if any weakref callback gets invoked), it
-          looks like trash to gc too, and gc also tries to delete self
-          then.  But we're already deleting self.  Double deallocation is
-          a subtle disaster.
-    */
 }
 
 static PyTypeObject *solid_base(PyTypeObject *type);

Python/hamt.c

@@ -1155,7 +1155,6 @@ hamt_node_bitmap_dealloc(PyHamtNode_Bitmap *self)
     Py_ssize_t len = Py_SIZE(self);
     Py_ssize_t i;
 
-    PyObject_GC_UnTrack(self);
     Py_TRASHCAN_BEGIN(self, hamt_node_bitmap_dealloc)
 
     if (len > 0) {
@@ -1559,13 +1558,11 @@ hamt_node_collision_traverse(PyHamtNode_Collision *self,
 static void
 hamt_node_collision_dealloc(PyHamtNode_Collision *self)
 {
+    Py_TRASHCAN_BEGIN(self, hamt_node_collision_dealloc)
     /* Collision's tp_dealloc */
 
     Py_ssize_t len = Py_SIZE(self);
 
-    PyObject_GC_UnTrack(self);
-    Py_TRASHCAN_BEGIN(self, hamt_node_collision_dealloc)
-
     if (len > 0) {
 
         while (--len >= 0) {

Python/traceback.c

@@ -168,7 +168,6 @@ static PyGetSetDef tb_getsetters[] = {
 static void
 tb_dealloc(PyTracebackObject *tb)
 {
-    PyObject_GC_UnTrack(tb);
     Py_TRASHCAN_BEGIN(tb, tb_dealloc)
     Py_XDECREF(tb->tb_next);
     Py_XDECREF(tb->tb_frame);