python · encukou · Sep 11, 2019 · Aug 17, 2019 · Aug 17, 2019 · Aug 17, 2019
diff --git a/Lib/test/test_capi.py b/Lib/test/test_capi.py
@@ -38,6 +38,92 @@ class InstanceMethod:
 
 class CAPITest(unittest.TestCase):
 
+    def test_subclass_of_heap_gc_ctype_with_tpdealloc_decrefs_once(self):
+        class HeapGcCTypeSubclass(_testcapi.HeapGcCType):
+            def __init__(self):
+                self.value2 = 20
+                super().__init__()
+
+        subclass_instance = HeapGcCTypeSubclass()
+        type_refcnt = sys.getrefcount(HeapGcCTypeSubclass)
+
+        # Test that subclass instance was fully created
+        self.assertEqual(subclass_instance.value, 10)
+        self.assertEqual(subclass_instance.value2, 20)
+
+        # Test that the type reference count is only decremented once
+        del subclass_instance
+        self.assertEqual(type_refcnt - 1, sys.getrefcount(HeapGcCTypeSubclass))
+
+    def test_subclass_of_heap_gc_ctype_with_del_modifying_dunder_class_only_decrefs_once(self):
+        class A(_testcapi.HeapGcCType):
+            def __init__(self):
+                self.value2 = 20
+                super().__init__()
+
+        class B(A):
+            def __init__(self):
+                super().__init__()
+
+            def __del__(self):
+                self.__class__ = A
+                A.refcnt_in_del = sys.getrefcount(A)
+                B.refcnt_in_del = sys.getrefcount(B)
+
+        subclass_instance = B()
+        type_refcnt = sys.getrefcount(B)
+        new_type_refcnt = sys.getrefcount(A)
+
+        # Test that subclass instance was fully created
+        self.assertEqual(subclass_instance.value, 10)
+        self.assertEqual(subclass_instance.value2, 20)
+
+        del subclass_instance
+
+        # Test that setting __class__ modified the reference counts of the types
+        self.assertEqual(type_refcnt - 1, B.refcnt_in_del)
+        self.assertEqual(new_type_refcnt + 1, A.refcnt_in_del)
+
+        # Test that the original type already has decreased its refcnt
+        self.assertEqual(type_refcnt - 1, sys.getrefcount(B))
+
+        # Test that subtype_dealloc decref the newly assigned __class__ only once
+        self.assertEqual(new_type_refcnt, sys.getrefcount(A))
+
+    def test_c_subclass_of_heap_ctype_with_tpdealloc_decrefs_once(self):
+        subclass_instance = _testcapi.HeapCTypeSubclass()
+        type_refcnt = sys.getrefcount(_testcapi.HeapCTypeSubclass)
+
+        # Test that subclass instance was fully created
+        self.assertEqual(sub
10000
class_instance.value, 10)
+        self.assertEqual(subclass_instance.value2, 20)
+
+        # Test that the type reference count is only decremented once
+        del subclass_instance
+        self.assertEqual(type_refcnt - 1, sys.getrefcount(_testcapi.HeapCTypeSubclass))
+
+    def test_c_subclass_of_heap_ctype_with_del_modifying_dunder_class_only_decrefs_once(self):
+        subclass_instance = _testcapi.HeapCTypeSubclassWithFinalizer()
+        type_refcnt = sys.getrefcount(_testcapi.HeapCTypeSubclassWithFinalizer)
+        new_type_refcnt = sys.getrefcount(_testcapi.HeapCTypeSubclass)
+
+        # Test that subclass instance was fully created
+        self.assertEqual(subclass_instance.value, 10)
+        self.assertEqual(subclass_instance.value2, 20)
+
+        # The tp_finalize slot will set __class__ to HeapCTypeSubclass
+        del subclass_instance
+
+        # Test that setting __class__ modified the reference counts of the types
+        self.assertEqual(type_refcnt - 1, _testcapi.HeapCTypeSubclassWithFinalizer.refcnt_in_del)
+        self.assertEqual(new_type_refcnt + 1, _testcapi.HeapCTypeSubclass.refcnt_in_del)
+
+        # Test that the original type already has decreased its refcnt
+        self.assertEqual(type_refcnt - 1, sys.getrefcount(_testcapi.HeapCTypeSubclassWithFinalizer))
+
+        # Test that subtype_dealloc decref the newly assigned __class__ only once
+        self.assertEqual(new_type_refcnt, sys.getrefcount(_testcapi.HeapCTypeSubclass))
+
     def test_instancemethod(self):
         inst = InstanceMethod()
         self.assertEqual(id(inst), inst.id())

diff --git a/Misc/NEWS.d/next/C API/2019-08-17-13-50-21.bpo-37879.CZeUem.rst b/Misc/NEWS.d/next/C API/2019-08-17-13-50-21.bpo-37879.CZeUem.rst
@@ -0,0 +1,2 @@
+Fix subtype_dealloc to suppress the type decref when the base type is a C
+heap type
diff --git a/Modules/_testcapimodule.c b/Modules/_testcapimodule.c
@@ -6012,6 +6012,180 @@ static struct PyModuleDef _testcapimodule = {
     NULL
 };
 
+typedef struct {
+    PyObject_HEAD
+    int value;
+} HeapCTypeObject;
+
+static struct PyMemberDef heapctype_members[] = {
+    {"value", T_INT, offsetof(HeapCTypeObject, value)},
+    {NULL} /* Sentinel */
+};
+
+static int
+heapctype_init(PyObject *self, PyObject *args, PyObject *kwargs)
+{
+    ((HeapCTypeObject *)self)->value = 10;
+    return 0;
+}
+
+static void
+heapgcctype_dealloc(HeapCTypeObject *self)
+{
+    PyTypeObject *tp = Py_TYPE(self);
+    PyObject_GC_UnTrack(self);
+    PyObject_GC_Del(self);
+    Py_DECREF(tp);
+}
+
+static PyType_Slot HeapGcCType_slots[] = {
+    {Py_tp_init, heapctype_init},
+    {Py_tp_members, heapctype_members},
+    {Py_tp_dealloc, heapgcctype_dealloc},
+    {0, 0},
+};
+
+static PyType_Spec HeapGcCType_spec = {
+    "_testcapi.HeapGcCType",
+    sizeof(HeapCTypeObject),
+    0,
+    Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE | Py_TPFLAGS_HAVE_GC,
+    HeapGcCType_slots
+};
+
+static void
+heapctype_dealloc(HeapCTypeObject *self)
+{
+    PyTypeObject *tp = Py_TYPE(self);
+    PyObject_Del(self);
+    Py_DECREF(tp);
+}
+
+static PyType_Slot HeapCType_slots[] = {
+    {Py_tp_init, heapctype_init},
+    {Py_tp_members, heapctype_members},
+    {Py_tp_dealloc, heapctype_dealloc},
+    {0, 0},
+};
+
+static PyType_Spec HeapCType_spec = {
+    "_testcapi.HeapCType",
+    sizeof(HeapCTypeObject),
+    0,
+    Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,
+    HeapCType_slots
+};
+
+typedef struct {
+    HeapCTypeObject base;
+    int value2;
+} HeapCTypeSubclassObject;
+
+static int
+heapctypesubclass_init(PyObject *self, PyObject *args, PyObject *kwargs)
+{
+    /* Get HeapCTypeSubclass */
+    PyObject *m = PyState_FindModule(&_testcapimodule);
+    if (m == NULL) {
+        return -1;
+    }
+    PyTypeObject *ctypesubclass = (PyTypeObject *)PyObject_GetAttrString(m, "HeapCTypeSubclass");
+    if (ctypesubclass == NULL) {
+        return -1;
+    }
+
+    PyTypeObject *base = (PyTypeObject *)PyType_GetSlot(ctypesubclass, Py_tp_base);
+    Py_DECREF(ctypesubclass);
+    initproc base_init = PyType_GetSlot(base, Py_tp_init);
+    if (base_init(self, args, kwargs) < 0) {
+        return -1;
+    }
+    ((HeapCTypeSubclassObject *)self)->value2 = 20;
+    return 0;
+}
+
+static struct PyMemberDef heapctypesubclass_members[] = {
+    {"value", T_INT, offsetof(HeapCTypeObject, value)},
+    {"value2", T_INT, offsetof(HeapCTypeSubclassObject, value2)},
+    {NULL} /* Sentinel */
+};
+
+static PyType_Slot HeapCTypeSubclass_slots[] = {
+    {Py_tp_init, heapctypesubclass_init},
+    {Py_tp_members, heapctypesubclass_members},
+    {0, 0},
+};
+
+static PyType_Spec HeapCTypeSubclass_spec = {
+    "_testcapi.HeapCTypeSubclass",
+    sizeof(HeapCTypeSubclassObject),
+    0,
+    Py_TPFLAGS_DEFAULT | Py_TPFLAGS_BASETYPE,
+    HeapCTypeSubclass_slots
+};
+
+static int
+heapctypesubclasswithfinalizer_init(PyObject *self, PyObject *args, PyObject *kwargs)
+{
+    PyTypeObject *base = (PyTypeObject *)PyType_GetSlot(Py_TYPE(self), Py_tp_base);
+    initproc base_init = PyType_GetSlot(base, Py_tp_init);
+    base_init(self, args, kwargs);
+    return 0;
+}
+
+static void
+heapctypesubclasswithfinalizer_finalize(PyObject *self)
+{
+    PyObject *error_type, *error_value, *error_traceback, *m, *oldtype, *newtype;
+
+    /* Save the current exception, if any. */
+    PyErr_Fetch(&error_type, &error_value, &error_traceback);
+
+    m = PyState_FindModule(&_testcapimodule);
+    if (m == NULL) {
+        return;
+    }
+    oldtype = PyObject_GetAttrString(m, "HeapCTypeSubclassWithFinalizer");
+    newtype = PyObject_GetAttrString(m, "HeapCTypeSubclass");
+    if (oldtype == NULL || newtype == NULL) {
+        goto cleanup_finalize;
+    }
+
+    if (PyObject_SetAttrString(self, "__class__", newtype) < 0) {
+        goto cleanup_finalize;
+    }
+    if (PyObject_SetAttrString(
+        oldtype, "refcnt_in_del", PyLong_FromSsize_t(Py_REFCNT(oldtype))) < 0) {
+        goto cleanup_finalize;
+    }
+    if (PyObject_SetAttrString(
+        newtype, "refcnt_in_del", PyLong_FromSsize_t(Py_REFCNT(newtype))) < 0) {
+        goto cleanup_finalize;
+    }
+
+cleanup_finalize:
+    Py_XDECREF(oldtype);
+    Py_XDECREF(newtype);
+
+    /* Restore the saved exception. */
+    PyErr_Restore(error_type, error_value, error_traceback);
+}
+
+static PyType_Slot HeapCTypeSubclassWithFinalizer_slots[] = {
+    {Py_tp_init, heapctypesubclasswithfinalizer_init},
+    {Py_tp_members, heapctypesubclass_members},
+    {Py_tp_finalize, heapctypesubclasswithfinalizer_finalize},
+    {0, 0},
+};
+
+static PyType_Spec HeapCTypeSubclassWithFinalizer_spec = {
+    "_testcapi.HeapCTypeSubclassWithFinalizer",
+    sizeof(HeapCTypeSubclassObject),
+    0,
+    Py_TPFLAGS_DEFAULT | Py_TPFLAGS_HAVE_FINALIZE,
+    HeapCTypeSubclassWithFinalizer_slots
+};
+
 /* Per PEP 489, this module will not be converted to multi-phase initialization
  */
 
@@ -6129,5 +6303,40 @@ PyInit__testcapi(void)
     TestError = PyErr_NewException("_testcapi.error", NULL, NULL);
     Py_INCREF(TestError);
     PyModule_AddObject(m, "error", TestError);
+
+    PyObject *HeapGcCType = PyType_FromSpec(&HeapGcCType_spec);
+    if (HeapGcCType == NULL) {
+        return NULL;
    }
+    PyModule_AddObject(m, "HeapGcCType", HeapGcCType);
+
+    PyObject *HeapCType = PyType_FromSpec(&HeapCType_spec);
+    if (HeapCType == NULL) {
+        return NULL;
+    }
+    PyObject *subclass_bases = PyTuple_Pack(1, HeapCType);
+    if (subclass_bases == NULL) {
+        return NULL;
+    }
+    PyObject *HeapCTypeSubclass = PyType_FromSpecWithBases(&HeapCTypeSubclass_spec, subclass_bases);
+    if (HeapCTypeSubclass == NULL) {
+        return NULL;
+    }
+    Py_DECREF(subclass_bases);
+    PyModule_AddObject(m, "HeapCTypeSubclass", HeapCTypeSubclass);
+
+    PyObject *subclass_with_finalizer_bases = PyTuple_Pack(1, HeapCTypeSubclass);
+    if (subclass_with_finalizer_bases == NULL) {
+        return NULL;
+    }
+    PyObject *HeapCTypeSubclassWithFinalizer = PyType_FromSpecWithBases(
+        &HeapCTypeSubclassWithFinalizer_spec, subclass_with_finalizer_bases);
+    if (HeapCTypeSubclassWithFinalizer == NULL) {
+        return NULL;
+    }
+    Py_DECREF(subclass_with_finalizer_bases);
+    PyModule_AddObject(m, "HeapCTypeSubclassWithFinalizer", HeapCTypeSubclassWithFinalizer);
+
+    PyState_AddModule(m, &_testcapimodule);
     return m;
 }
diff --git a/Objects/typeobject.c b/Objects/typeobject.c
@@ -1157,11 +1157,9 @@ subtype_dealloc(PyObject *self)
     /* Test whether the type has GC exactly once */
 
     if (!PyType_IS_GC(type)) {
-        /* It's really rare to find a dynamic type that doesn't have
-           GC; it can only happen when deriving from 'object' and not
-           adding any slots or instance variables.  This allows
-           certain simplifications: there's no need to call
-           clear_slots(), or DECREF the dict, or clear weakrefs. */
+        /* A non GC dyanmic type allows certain simplifications:
+           there's no need to call clear_slots(), or DECREF the dict,
+           or clear weakrefs. */
 
         /* Maybe call finalizer; exit early if resurrected */
         if (type->tp_finalize) {
@@ -1177,7 +1175,6 @@ subtype_dealloc(PyObject *self)
         /* Find the nearest base with a different tp_dealloc */
         base = type;
         while ((basedealloc = base->tp_dealloc) == subtype_dealloc) {
-            assert(Py_SIZE(base) == 0);
             base = base->tp_base;
             assert(base);
         }
@@ -1189,8 +1186,10 @@ subtype_dealloc(PyObject *self)
         assert(basedealloc);
         basedealloc(self);
 
-        /* Can't reference self beyond this point */
-        Py_DECREF(type);
+       /* Only decref if the base type is not already a heap allocated type.
+          Otherwise, basedealloc should have decref'd it already */
+        if (type->tp_flags & Py_TPFLAGS_HEAPTYPE && !(base->tp_flags & Py_TPFLAGS_HEAPTYPE))
+                                                Py_DECREF(type);
 
         /* Done */
         return;
@@ -1289,8 +1288,9 @@ subtype_dealloc(PyObject *self)
 
     /* Can't reference self beyond this point. It's possible tp_del switched
        our type from a HEAPTYPE to a non-HEAPTYPE, so be careful about
-       reference counting. */
-    if (type->tp_flags & Py_TPFLAGS_HEAPTYPE)
+       reference counting. Only decref if the base type is not already a heap
+       allocated type. Otherwise, basedealloc should have decref'd it already */
+    if (type->tp_flags & Py_TPFLAGS_HEAPTYPE && !(base->tp_flags & Py_TPFLAGS_HEAPTYPE))
       Py_DECREF(type);
 
   endlabel:
Original file line number	Diff line number	Diff line change
		@@ -0,0 +1,2 @@
		Fix subtype_dealloc to suppress the type decref when the base type is a C
		heap type