Tk Source Code

I observed this issue under cgimage-drawing, but I believe it is an existing issue being exposed (possibly also due to running on slow hardware) rather than a newly introduced one. I believe it is likely the same issue reported in [2958768] (which I do not believe was a duplicate of [456548]).

I occasionally encounter window-2.12 failing due to an unexpected error (shown in red) from the child script:

waiting
can't invoke "event" command: application has been destroyed
    while executing
"event generate $w <<ThemeChanged>>"
    (procedure "ttk::ThemeChanged" line 6)
    invoked from within
"ttk::ThemeChanged"
ringing the bell -> can't invoke "bell" command: application has been destroyed
done waiting
bell -> can't invoke "bell" command: application has been destroyed
update ->

(Note that window-2.12 runs exec such that it reports stderr from the child script after all stdout rather than in chronological order; maybe this can be improved for clarity.)

Even when trying the child script by itself, this error would not always appear, as it only happens when the ThemeChangedProc() idle handler doesn’t get handled before after 100 {destroy .}. I think this reveals an issue where outstanding calls to ThemeChangedProc() are not canceled when Tk is destroyed: Ttk_StylePkgFree(), the function responsible for doing Tcl_CancelIdleCall(ThemeChangedProc, …), is given to Tcl_SetAssocData() and so is only called when the interp is destroyed rather than when Tk is destroyed.

I’m not aware what should be used instead of Tcl_SetAssocData(). If there is no readily available fix for this, then as a workaround the window-2.12 test can wait longer for Tk to settle, or at least do update idletasks, before doing destroy ..

Thanks, I think our release manager will do that.

Fix is now merged in core-8-6-branch and in trunk.

Many thanks again!

Only other suggestion I have would be to include the error message itself in the blurb for this fix in the 8.6.14 release notes.

To alleviate any doubts, I have tried to build Tk with a C++ compiler (specifically, passing CC=g++ ./configure... on Linux). There are lots of build errors, so I was right when stating we have lost C++ build compatibility anyway.

More specifically, the first build error in ttkTheme.c is on line 995 where an explicit cast is missing. This position is way *after* the two lines changed in [fadca8dc], so the cast removals on these lines do not worsen the situation regarding building with a C++ compiler.

I like [67ac1ac13e] more since it's just a direct call to the handler from the right place in the destroy code, instead of making use of a more complicated Tcl_TraceCommand() machinery. As already mentioned, both ways do work but I prefer simplicity to ease code maintenance.

Thanks for the remark about superfluous casts, action taken in [fadca8dc]. Note however that the original cast in Ttk_RegisterCleanup() was added in [5da24ae4], which is implementation of TIP #557. This TIP strives to let Tcl/Tk compile with a C++ compiler. It says that the CI tool of this time (Travis) made specific builds using a C++ compiler in order to detect breakage of C++ builds. I think we have lost C++ build compatibility for a while because I don't see any build with a C++ compiler in the current CI Github Actions.

Anything more to do before merging, in your opinion?

I am not sure [67ac1ac13e] is any better or worse. Had I found where the other …PkgFree functions were used and/or had not thought of using Tcl_TraceCommand(interp, ".", …), I probably would have tried something similar. Presumably [fcbbbaa3] would be the only option if Tile were still a separate package. But [67ac1ac13e] is fine especially if there is some downside to relying on Tcl_TraceCommand() which I’m overlooking.

Casting the result of GetStylePackageData(interp) to (StylePackageData*) (as also done in Ttk_RegisterCleanup()) should not be necessary.

[fcbbbaa3] works because on destroying "." a DestroyNotify event is generated and immediately serviced in Tk_DestroyWindow() (tkWindow.c:1483). This triggers FrameEventProc(), which deletes the "." command. During this deletion process, Tcl_DeleteCommandFromToken() calls all command traces with flags TCL_TRACE_DELETE (tclBasic.c:3173), which includes our new handler Ttk_TkDestroyedHandler(). As a consequence, when returning from "destroy ." no pending <<ThemeChanged>> can still remain in the event queue. I think this answers your question about why it works.

I had a look at an alternate approach, consisting in calling Ttk_TkDestroyedHandler() directly (i.e. without resorting to the trace delete machinery) at the time Tk is destroyed. This is [67ac1ac13e]. With this patch I cannot reproduce the problem anymore. It's maybe less than totally ideal (regarding the memory hoarding you described), but as you mentioned, this memory hoarding issue is happening everytime Tcl_SetAssocData() is used in Tk and is therefore a broader (minor IMHO) problem. What do you think of [67ac1ac13e]?

I am fine with closing this ticket if [fcbbbaa3] is considered good enough; again, I think the important thing is others agreeing *why* it eliminates the issue.

Proposal: let's merge [fcbbbaa3] since this fix appears to work, and keep this ticket open until someone comes up either with a better fix or with a reasoning about why [fcbbbaa3] already is the best fix. Agreed?

I think the main reason I would want investigate the alternative to relying on a deletion callback passed to Tcl_SetAssocData() is to avoid hoarding memory which could be freed after Tk is destroyed rather than when the interpreter is deleted. But there are already several other instances of this issue in core Tk.

Ttk_StylePkgInit() is called from Ttk_Init() and much later than other …PkgInit() functions, so I am not sure whether adjusting Ttk_StylePkgFree() to be used like other …PkgFree() functions is as straightforward as it might look.

And I would also wonder if using Tcl_TraceCommand(interp, ".", …) is actually a better approach than Tk manually invoking …PkgFree() functions. But it would be better to know how Tk extensions usually/should clean themselves up (if they do).

OK. I can reproduce by sourcing the script you posted on 2023-04-04 22:36:26.

I have committed your patch based on Tcl_TraceCommand() in a bugfix branch. With this patch I can no longer reproduce.

Your alternate proposal based on calling Ttk_StylePkgFree() from Tk_DestroyWindow() remains to be investigated, if deemed better.

In this case, however, I would hope that Tcl developers might be more convinced by the bug description of why this error possible and why the attached proposed fix or similar should eliminate that possibility, than by a script to empirically suggest whether the bug is present or gone.

The most reliable way to trigger this that I know of is still by running the window-2.12 child script in tclsh with after 100 {destroy .} changed to after 0 {destroy .}:

package require Tk
after 1000 {set forever 1}
#after 100 {destroy .}
after 0 {destroy .}
after 200 {catch bell msg; puts "ringing the bell -> $msg"}
after 250 {update idletasks}
after 300 {update}
puts "waiting"
vwait forever
puts "done waiting"
catch {bell} msg
puts "bell -> $msg"
catch update msg
puts "update -> $msg"

I have also now tried the modified script on Windows and it triggers the error. So I have seen the error on Windows, X11, and the alternative Aqua implementation discussed on [3e9e82bcfe] (but not the current Aqua implementation, which I think is likely due to event handling peculiarities it has compared to other platforms).

Sounds like a brilliant idea.

The problem, I think, is that first of all we would need a way to reliably trigger the issue. Only then we could confirm whether such a fix works or not.

Sorry, I should have said: …instead of using Tcl_SetAssocData(interp, PKG_ASSOC_KEY, Ttk_StylePkgFree, pkgPtr) as done currently.

Other …PkgFree() functions are called from Tk_DestroyWindow(), so I wonder if that is where Ttk_StylePkgFree() should be called from rather than trying to use Tcl_TraceCommand(interp, ".", TCL_TRACE_DELETE, Ttk_TkDestroyedHandler, pkgPtr).

For reference, the current approach was introduced by https://sourceforge.net/p/tktable/tile/ci/b023b92/

Oops. Yes you're right. I could reproduce [2958768fff], then I had put my (wrong) proposed fix in and could no longer reproduce. I thought it was OK, but I realize now that this is a race condition: my proposed fix won't work. Sorry for the noise.

If Tk is destroyed then the winfo command will be unavailable.

This ticket is a duplicate of [2958768fff].

Another approach to fix this is the following one liner:

Index: library/ttk/ttk.tcl
==================================================================
--- library/ttk/ttk.tcl
+++ library/ttk/ttk.tcl
@@ -56,10 +56,11 @@
 #	Called from [::ttk::style theme use].
 #	Sends a <<ThemeChanged>> virtual event to all widgets.
 #
 proc ::ttk::ThemeChanged {} {
     set Q .
+    if {![winfo exists $Q]} {return}
     while {[llength $Q]} {
 	set QN [list]
 	foreach w $Q {
 	    event generate $w <<ThemeChanged>>
 	    foreach child [winfo children $w] {

Isn't this easier? What do you think?

If Tcl always handles delete trace callbacks before interpreter destroyed callbacks, then would it be appropriate to instead use a separate callback to cancel idle calls to ThemeChangedProc() and register it with Tcl_TraceCommand(interp, ".", TCL_TRACE_DELETE, …, pkgPtr)? See attached patch. I have not encountered any existing C API usage of Tcl_TraceCommand(interp, ".", …) in core Tk.

I think it would also be nice to have a dedicated test for this issue, but I have not figured out how to reliably trigger the issue on all platforms, even with after 0 {destroy .}; so far I have only observed it on X11 with 8.6.12, and Aqua with cgimage-drawing (but not core-8-6-branch; maybe it is a bug for it not to occur); I have not tried on Windows.