Tk Source Code

On a system where the default visual has 24-bit depth but supports visuals with 32-bit depth, attempting to draw Xft text in a toplevel with 32-bit depth causes an X11 error:

% winfo depth .
24
% toplevel .t -visual {best 32}
.t
% winfo depth .t
32
% pack [label .t.l -text updog]
% X Error of failed request:  BadValue (integer parameter out of range for operation)
  Major opcode of failed request:  91 (X_QueryColors)
  Value in failed request:  0xff000000
  Serial number of failed request:  539
  Current serial number in output stream:  539

This error occurs when LookUpColor() calls XQueryColor(). I think this happens because the code in tkUnixRFont.c incorrectly assumes that only the default visual and colormap for the screen will be used. I am not very familiar with visuals and colormaps in Xlib, but I might still try to cargo cult a fix for this.

Fixed [cf71a9dad0031fe3|here]

Thanks to all people who cooperated to get this fixed!

Candidate for Tk 9.0.3???

Please see the new branch bug-c23f79ef96-retry which contains Christian's fix and non-regression test for just this ticket separated out from the bug-c23f79ef96 branch. It can be easily merged into core-9-0-branch (only 1 conflict with a comment near the end of visual.test).

The [66e803e4] state of branch bug-c23f79ef96 is the state to be merged with the fixes for the present ticket.

While this merges nicely into core-8-6-branch, there are the following conflicts when trying to merge into trunk:

MERGE generic/tkFont.c
MERGE generic/tkFont.h
MERGE generic/ttk/ttkCache.c
***** 4 merge conflicts in generic/ttk/ttkCache.c
MERGE library/choosedir.tcl
MERGE library/clrpick.tcl
MERGE library/comdlg.tcl
MERGE library/dialog.tcl
MERGE library/msgbox.tcl
***** 4 merge conflicts in library/msgbox.tcl
MERGE library/tkfbox.tcl
***** 2 merge conflicts in library/tkfbox.tcl
MERGE library/xmfbox.tcl
***** 2 merge conflicts in library/xmfbox.tcl
MERGE tests/visual.test
MERGE tests/xmfbox.test
***** 1 merge conflict in tests/xmfbox.test
MERGE unix/tkUnixRFont.c
WARNING: 5 merge conflicts

ttkCache.c is easy, the rest is less easy.

Especially in library/msgbox.tcl, the changes conflict with the fix for [e2cec2fa41].

Having thought at this today a bit more I'm aligned with your feeling. We should (and with fossil we can) merge the fixes for the present ticket, but not yet the one for [fde9dc2392] (let alone [923174fff] for which we don't have a fix yet).

I'll proceed and close the present ticket. Thanks to all!

The issue I reported seems adequately addressed. But I am a bit lost with all of the subsequent issues and changes.

I wonder if it would be better to merge the changes meant for this ticket first before deciding to merge the changes for [fde9dc239260]. (I would not find it confusing if merges were instead done from an earlier check-in of a branch, rather than by “backing out” recent changes and then merging a branch. But I do not use fossil, so maybe I incorrectly assume it can do this.)

Christopher, I'd like to get your feedback on my questions below (see 2023-12-06 20:06:34) before I merge.

Schelte's tests look ok, but since you're the originator of this ticket, your opinion is valuable as well especially since you said this ticket "seems unfixable". Thanks!

Hmmm... Trying again in core-8-6-branch with a fresh and fossil updated build of Tcl and Tk I can now again reproduce the original crash. No idea why I could not before.

It's Debian 11 x86_64 with KDE on Xorg.

It's strange because without the patches I could trigger the X error reported by Christopher, but now I can't any more, the text is just not displayed without the patches.

Francois,

> ... instead the text is simply not displayed in the label.

what is your X11 setup for testing? Mine are Debian 9 and 11 x86_64
with GNOME on Xorg or GNOME on Wayland, CentOS 6 x86_64 (old Xorg),
CentOS 5 (even older Xorg). And I can see the text in all setups.

Christopher, do you consider to work to be finished on this ticket? Are you confident in the non-regression and do we have a sufficient level of testing? Do you think we can merge?

Thanks!

I have added a non-regression test visual-9.1 in the test suite. This test is inspired from the originally crashing script.

What is a bit strange is that I no longer succeed in making this script crash in core-8-6-branch. I can't seem to be able to trigger the X11 error, instead the text is simply not displayed in the label.

This works indeed. Committed now, thanks.

What about in the Create proc

 ...
 bind $data(okBtn) <Destroy> {::tk::dialog::file::Destroyed %W}
 ...

and the Destroyed proc being

  proc ::tk::dialog::file::Destroyed {okBtn} {
    variable selectFilePath

    bind $okBtn <Destroy> {}
    set selectFilePath ""
  }

if the idea of the whole business is to have an
emergency exit to vwait in case the window goes away
by other means.

The test suite hangs because a Tcl error triggers, it waits for the user to acknowledge the error.

For me clearing the <Destroy> binding within CancelCmd smells like
a bug since the Cancel button does not destroy the dialog. When
the dialog is reopened for the same parent it gets reused but not
recreated and the <Destroy> binding is vanished from the previous
invocation of CancelCmd.

Now the fine questions are: What is the real reason for the hang?
Is the clearing of <Destroy> necessary at all?

Understood (regarding the documentation).

CI at Github reveals that filebox.test hangs now. From a quick analysis of [7608b5e7] I'm wondering why the "bind $data(okBtn) <Destroy> {}" statement was moved from proc ::tk::dialog::file::CancelCmd to proc ::tk::dialog::file::Destroyed ? Reverting this fixes the hang. This is now done in [01e397eb5e]. Could you please review and confirm? Thanks!

BTW, I don't think that documentation needs to be updated,
since (provided everything really works now) things should be
as advertised decades ago regarding the -screen and -visual options.

Right, thanks! Now committed.

Francois,

to be equivalent to the !NEED_EXTRA_INFO your newly introduced
if block should be

  return cachedPtr ? cachedPtr->objPtr : NULL;

instead. Indeed my initial fault as usually.

The following patch fixes the crash and lets the test suite pass 100% for ttk, but it leads me to questioning the logic regarding newEntry in Ttk_Use(). If newEntry is false, shouldn't we already have a non-NULL cachedPtr?

Index: generic/ttk/ttkCache.c
==================================================================
--- generic/ttk/ttkCache.c
+++ generic/ttk/ttkCache.c
@@ -350,11 +350,13 @@
     if (!newEntry) {
 #if !NEED_EXTRA_INFO
 	return Tcl_GetHashValue(entryPtr);
 #else
 	Ttk_Cached *cachedPtr = Tcl_GetHashValue(entryPtr);
-	return cachedPtr->objPtr;
+	if (cachedPtr) {
+	    return cachedPtr->objPtr;
+	}
 #endif
     }
 
     cacheObj = Tcl_DuplicateObj(objPtr);
     Tcl_IncrRefCount(cacheObj);

All four patches produced by Christian are now committed in a bugfix branch for easier testing.

I had to apply all this manually by copy/paste, I hope I didn't make too many errors. If another pair of eyes could check I would not mind at all, thanks!

The script originally provided in the report now works (Linux with Xft). However, unless I made a mistake when applying the patches, the work must not be finished since the test suite segfaults during (ttk) entry-7.1 test (Linux with Xft again).

Could we perhaps add some tests specific to this ticket? Also the documentation could need updates I think.

My next attempt in fixing the caching in ttk regarding fonts,
colors, and borders is here

  https://www.androwish.org/home/info/4979992f45ef6cb4

For the 10k X11 display connections challenge, there's a TK_USE_POLL
define here (and at other places)

  https://www.androwish.org/home/file?ci=tip&name=jni/sdl2tk/unix/tkUnixEvent.c

which can be borrowed for Tk 9.x using the benefits of epoll/kqueue notifiers
on POSIX platforms.

Another proof-of-concept update is in

  https://www.androwish.org/home/info/08edf82b9b7e94e5

most of it dealing with vwait in modal dialogs. This
was modeled around elements ::tk::Priv which isn't always
working when ::tk::ScreenChanged is upvar'ing this
array behind the curtain while vwait'ing on it is still
in progress.

For the record: I highly doubt that the original code did
function smoothly for the last say 15 to 20 years.

Christopher,

indeed there are more issues in Tk, which are related to tickets [bd2f04ea]
and [a80e5ff97e], and which need be fixed in the ttkCache.c module (which
tells its own story in a BUGS/TODO comment, BTW). For another
proof-of-concept solution please refer to

  https://www.androwish.org/home/info/8891f0f0927cb844

Now for the 10k X11 display connections challenge...

Christian, thanks for the patch. I have briefly tried it but the results seem promising. However I still wonder if there is an issue where code elsewhere (inside or outside core Tk) and documentation still assumes Tk fonts are per-screen, and if changing that is not allowed for Tk 8.x.

See this proof-of-concept patch

 https://www.androwish.org/home/info/843e00e7fc400009

Christopher,

I believe your patch should be called "half-of-the-solution.diff" instead,
if in tkFont.h and tkFont.c some ifdef'ery regarding HAVE_XFT is added
with the additional fields Visual *visual and Colormap colormap in the
TkFont struct. The former comparison for the screen field must be enhanced
to test for Tk_Visual(tkwin) and Tk_Colormap(tkwin), too (I think 5 places).
So the toplevel (generic) font caching takes both visual and colormap into
account when font rendering uses libxft.

This increasingly seems unfixable. Tk_DrawChars() is meant to be used with an arbitrary X drawable (on the screen where the font is valid, presumably), but does not provide a visual, let alone a colormap. And Xlib, maybe intentionally, does not appear to allow retrieving the visual for an arbitrary drawable.

There is a limitation in the generic code which I think makes this issue more difficult to fix. Tk only manages fonts by screen, and assumes fonts are usable at any depth. But Xft fonts created at one depth are not usable at other depths.

The attached patch is not a solution to this issue; it is just to illustrate how trying to fix the BadValue error by creating and drawing the Xft font with the “correct” visual and colormap instead leads to a different error (when Tk calls XftDrawGlyphFontSpec(); the X server error comes from ProcRenderCreatePicture()) when the same font is then used at a different depth on the same screen:

X Error of failed request:  BadMatch (invalid parameter attributes)
  Major opcode of failed request:  139 (RENDER)
  Minor opcode of failed request:  4 (RenderCreatePicture)
  Serial number of failed request:  1406
  Current serial number in output stream:  1407