gh-138791: fix crash when visiting a non-consumed timer handler

[picnixz]

In 283380a, I forgot to take into account the following:

• a timer handler may not necessarily be destroyed when we deallocate the window.
• whenever a timer handler is created, there are actually two references that are created; one regular and one that is used as client data and sent to Tcl.

When the handler is called, this extra reference is cleared and the Tcl handler is destroyed. However, if the object is destroyed before that, this never happens, so we're having a leak.

cc @serhiy-storchaka

• Issue: Heap types implemented in C must have Py_TPFLAGS_HAVE_GC and implement tp_traverse #116946

• Issue: Crash with non-consumed Tk timer handler #138791

[serhiy-storchaka]

I think that the point of this extra reference is that TkttObject should not be collected until the timer fires. You call createtimerhandler(), do not save the result, and it should live until the timer function is invoked, then it can be collected.

So, it cannot be cleared if self->func is not NULL, and if self->func is NULL, there is no longer an extra reference.

If self->func is not NULL, just leave Tktt_Traverse().

[picnixz]

Oh maybe here is what I should do:

• In Dealloc(): should I eagerly clear the timer or should I leave a dangling callback?
• In Clear(): should I just clear the function?
• In Traverse(): I should just visit self->func?

[serhiy-storchaka]

Dealloc() is only called when the refcount is 0. This cannot happen before the timer was triggered.

Clear() should not clear the function before it was invoked. When it has been invoked, nothing to clear.

The loop created by TkttObject himself should not be broken. But we want to break larger loops which contain a TkttObject (TkttObject -> func -> ... ->TkttObject). And it would be nice if gc.get_referrers() and gc.get_referents() work with TkttObject too.

Hmm. Then we should not touch Tktt_Traverse, but make Tktt_Clear a no-op. Or just set tp_clear to NULL. Not every GC-class should have tp_clear.

[picnixz]

But how would you ensure that the following doesn't leak at the end of the execution:

root = tkinter.Tk()
func = lambda *_, **__: None
func.evil = type(root.tk.createtimerhandler(10000, print))
root.tk.createtimerhandler(10000, func)
root.destroy()

Note that the following crashes for me:

root = tkinter.Tk()
func = lambda *_, **__: None
func.evil = type(root.tk.createtimerhandler(10000, print))
root.tk.createtimerhandler(10000, func)
root.mainloop()

If after executing the code, close the window and close the REPL, I get (on main):

python: ./Modules/_tkinter.c:2784: int Tktt_Traverse(PyObject *, visitproc, void *): Assertion `TkttObject_Check(op)' failed.

[ZeroIntensity]

I might be missing something, but won't this just lead to infinite recursion?

[ZeroIntensity]

Again, I'm probably missing something -- why do we have that extra reference in the first place?

[serhiy-storchaka]

To prevent the result of createtimerhandler() from being collected before the timer fires.

[serhiy-storchaka]

I tried different options, but I couldn't get rid of the crash. #138331 should be reverted.

[picnixz]

Mmh, yes, if adding the GC causes this, let's revert this part. I don't think we need to revert the GC support on TkAppObject though.

[serhiy-storchaka]

Please test that reverting only TkttObject helps.

[picnixz]

I'll work on this one during the sprint and tomorrow, but offline. I'll need to think about how to prevent leaks and crashes while keeping the object alive.

[ZeroIntensity]

Shall we start making use of the sprint label?

[picnixz]

I wanted to make it a draft (I'm on mobile only). And I think we can use the sprint label yes

[picnixz]

I'm going to close this one until I've decided what to do with the GC for Tk. I don't think it's something I can fix in a matter of hours.