Below sample program, will raise "Modules/gcmodule.c:110: gc_decref: Assertion "gc_get_refs(g) > 0" failed: refcount is too small" on Python 3.8.2 debug build.
On 3.7.6 debug build, "Modules/gcmodule.c:277: visit_decref: Assertion `_PyGCHead_REFS(gc) != 0' failed." is raised.

import collections
import gc
import weakref

hooks_dict = collections.OrderedDict()
hooks_dict_ref = weakref.ref(hooks_dict)
gc.collect()

print('Hello world')

The complete error message on 3.8.2 debug build is

Modules/gcmodule.c:110: gc_decref: Assertion "gc_get_refs(g) > 0" failed: refcount is too small
Memory block allocated at (most recent call first):
  File "/home/$USER/test.py", line 6

object  : <weakref at 0x7ff788208a70; to 'collections.OrderedDict' at 0x7ff7881fab90>
type    : weakref
refcount: 1
address : 0x7ff788208a70
Fatal Python error: _PyObject_AssertFailed
Python runtime state: initialized

Current thread 0x00007ff789f9c080 (most recent call first):
  File "/home/$USER/test.py", line 7 in <module>
zsh: abort      PYTHONTRACEMALLOC=1 python ~/test.py

Thanks for the succinct example! Although you don't need the print() ;-)

I can confirm this crashes the same way under a current master build (albeit on Windows 64-bit).

gc is calculating how many references in the current generation are accounted for by intra-generation references, and gc's visit_decref() is getting called back by odictobject.c's odict_traverse(), on line

    Py_VISIT(od->od_weakreflist);

gc has found a second pointer to od->od_weakreflist, which is more than its refcount (1) claims exist.

Python's weakref implementation gives me headaches, so I'm adding Raymond to the nosy list under the hope the problem will be obvious to him.

I'm suspecting that maybe we shouldn't be doing

    Py_VISIT(od->od_weakreflist);

at all - best I can tell from a quick (non-exhaustive!) scan, the objects in the weakref list aren't incref'ed to begin with. And even if they were, that line would only be looking at the head of the list, ignoring all the non-head weakrefs after the head.

Yes, I don't think other weakref-supporting objects traverse the weakreflist in their tp_traverse.

I concur with Antoine and Tim. The GC already has the machinery to deal with weak references in the correct way (even more after recent bugfixes regarding callbacks). Traversing the weak reference list in incorrect because the object does not "own" the weak references to it, as the weak references can die even if the object is alive. Also, as Tim mentions, the traverse will be called on the head of the list, in the same way if you do object.__weakref__ you will only get the HEAD of the list:

>>> import weakref
>>> class A: ...
>>> a = A()
>>> w1 = weakref.ref(a)
>>> w2 = weakref.ref(a, lambda *args: None) # Use a callback to avoid re-using the original weakref
>>> id(w1)
4328697104
>>> id(w2)
4328758864
>>> id(a.__weakref__)
4328697104

I think that this is not very well documented, as there is no pointers on what should and should not be traversed in https://docs.python.org/3.8/c-api/typeobj.html#c.PyTypeObject.tp_traverse.

I will prepare a PR to the documentation if everybody agrees and another one removing the traverse unless someone sees that something else is at play.

After some thought, I'm sure the diagnosis is correct: the weakref list must be made invisible to gc. That is, simply don't traverse it at all. The crash is exactly what's expected from traversing objects a container doesn't own references to.

I agree the tp_traverse docs should point out that weakref lists are special this way, but I think the problem is unique to them - can't think of another case where a container points to an object it doesn't own a reference to.

I agree the tp_traverse docs should point out that weakref lists are special this way, but I think the problem is unique to them - can't think of another case where a container points to an object it doesn't own a reference to.

Agreed, I was thinking a small warning or something because this is not the first time I see similar errors or users confused about what they should and should not track (with this I mention that we should say that "only objects that are *owned* should be tracked, and then the weakref warning or something similar). I will prepare a PR soon.

New changeset 0c2b509 by Pablo Galindo in branch 'master':
bpo-39778: Don't traverse weak-reference lists OrderedDict's tp_traverse and tp_clear (GH-18749)
0c2b509

New changeset 69ded39 by Miss Islington (bot) in branch '3.7':
bpo-39778: Don't traverse weak-reference lists OrderedDict's tp_traverse and tp_clear (GH-18749)
69ded39

New changeset 9ddcb91 by Pablo Galindo in branch '3.8':
[3.8] bpo-39778: Don't traverse weak-reference lists OrderedDict's tp_traverse and tp_clear (GH-18749) (GH-18756)
9ddcb91

New changeset 6df421f by Pablo Galindo in branch 'master':
bpo-39778: Add clarification about tp_traverse and ownership (GH-18754)
6df421f

New changeset 72fff60 by Miss Islington (bot) in branch '3.7':
bpo-39778: Add clarification about tp_traverse and ownership (GH-18754)
72fff60

New changeset 1827fc3 by Miss Islington (bot) in branch '3.8':
bpo-39778: Add clarification about tp_traverse and ownership (GH-18754)
1827fc3

It seems like OrderedDict was the only type which visited weak references:

$ scm.py grep 'VISIT.*weak'
master: Grep 'VISIT.*weak' -- <4284 filenames>

==============================================

Objects/odictobject.c:1457: Py_VISIT(od->od_weakreflist);

I could not reproduce the crash and from the discussion it seems resolved. Is there anything left here?

I could not reproduce the crash and from the discussion it seems resolved. Is there anything left here?

No, this should be fixed by now. I just forgot to close the issue. Thanks for the ping!

I'm thinking that edit to tp_dealloc was incorrect. The PyClear call should have been replaced (not removed) with the pattern shown in the C APIs docs ( https://docs.python.org/3/extending/newtypes.html?highlight=pyobject_clearw#weak-reference-support ):

    if (self->weakreflist != NULL)
        PyObject_ClearWeakRefs((PyObject *) self);

Also a test should be added to make sure the weakref callbacks are still occurring.

I'm thinking that edit to tp_dealloc was incorrect.

What edit are you referring to? PR 18749 only touches tp_clear and tp_traverse, not tp_dealloc

Also, the ordered dict dealloc is already doing that:

False alarm. I misread the diff. All is well.