[3.4] bpo-26617: Ensure gc tracking is off when invoking weakref callbacks. (#2695)

* [3.4] bpo-26617: Ensure gc tracking is off when invoking weakref callbacks.
(cherry picked from commit 8f657c35b978b681e6e919f08358992e1aed7dc1)

* Rewrite a NEWS entry as a NEWS.d entry.

3 files changed, 22 insertions, 12 deletions

diff --git a/Lib/test/test_weakref.py b/Lib/test/test_weakref.py
index 4313c1d5c85..28f012f084f 100644
--- a/Lib/test/test_weakref.py
+++ b/Lib/test/test_weakref.py
@@ -827,6 +827,14 @@ class ReferencesTestCase(TestBase):
         with self.assertRaises(AttributeError):
             ref1.__callback__ = lambda ref: None
 
+    def test_callback_gcs(self):
+        class ObjectWithDel(Object):
+            def __del__(self): pass
+        x = ObjectWithDel(1)
+        ref1 = weakref.ref(x, lambda ref: support.gc_collect())
+        del x
+        support.gc_collect()
+
 
 class SubclassableWeakrefTestCase(TestBase):
 
diff --git a/Misc/NEWS.d/next/Core and Builtins/2017-07-15-13-55-22.bpo-26617.Gh5LvN.rst b/Misc/NEWS.d/next/Core and Builtins/2017-07-15-13-55-22.bpo-26617.Gh5LvN.rst
new file mode 100644
index 00000000000..c3a41396df8
--- /dev/null
+++ b/Misc/NEWS.d/next/Core and Builtins/2017-07-15-13-55-22.bpo-26617.Gh5LvN.rst
@@ -0,0 +1 @@
+Fix crash when GC runs during weakref callbacks.
diff --git a/Objects/typeobject.c b/Objects/typeobject.c
index ca5355a7daf..2fae4d73590 100644
--- a/Objects/typeobject.c
+++ b/Objects/typeobject.c
@@ -1116,11 +1116,6 @@ subtype_dealloc(PyObject *self)
     Py_TRASHCAN_SAFE_BEGIN(self);
     --_PyTrash_delete_nesting;
     -- tstate->trash_delete_nesting;
-    /* DO NOT restore GC tracking at this point.  weakref callbacks
-     * (if any, and whether directly here or indirectly in something we
-     * call) may trigger GC, and if self is tracked at that point, it
-     * will look like trash to GC and GC will try to delete self again.
-     */
 
     /* Find the nearest base with a different tp_dealloc */
     base = type;
@@ -1131,30 +1126,36 @@ subtype_dealloc(PyObject *self)
 
     has_finalizer = type->tp_finalize || type->tp_del;
 
-    /* Maybe call finalizer; exit early if resurrected */
-    if (has_finalizer)
-        _PyObject_GC_TRACK(self);
-
     if (type->tp_finalize) {
+        _PyObject_GC_TRACK(self);
         if (PyObject_CallFinalizerFromDealloc(self) < 0) {
             /* Resurrected */
             goto endlabel;
         }
+        _PyObject_GC_UNTRACK(self);
     }
-    /* If we added a weaklist, we clear it.      Do this *before* calling
-       tp_del, clearing slots, or clearing the instance dict. */
+    /*
+      If we added a weaklist, we clear it. Do this *before* calling tp_del,
+      clearing slots, or clearing the instance dict.
+
+      GC tracking must be off at this point. weakref callbacks (if any, and
+      whether directly here or indirectly in something we call) may trigger GC,
+      and if self is tracked at that point, it will look like trash to GC and GC
+      will try to delete self again.
+    */
     if (type->tp_weaklistoffset && !base->tp_weaklistoffset)
         PyObject_ClearWeakRefs(self);
 
     if (type->tp_del) {
+        _PyObject_GC_TRACK(self);
         type->tp_del(self);
         if (self->ob_refcnt > 0) {
             /* Resurrected */
             goto endlabel;
         }
+        _PyObject_GC_UNTRACK(self);
     }
     if (has_finalizer) {
-        _PyObject_GC_UNTRACK(self);
         /* New weakrefs could be created during the finalizer call.
            If this occurs, clear them out without calling their
            finalizers since they might rely on part of the object