Tk Source Code

OS: Windows 7, Windows 10. 

1. Unpack the archive https://bitbucket.org/budden/clcon_en/downloads/yar-2016-09-22-with-tcl-tk-bug.zip to the root of c: so that c:\yar is created. 
2. Start c:\yar\bin\yar.cmd . 
3. Do not allow firewall to open your computer :) But allow connections from localhost. 
4. Press Control-O (open file for edit) 
5. Paste c:\yar\tcl-8.6.6\build\bigfile.tcl into file open dialog
6. In the editor window opened, press Control-End to go to the end of file. 
7. Press and hold backspace key. You'll see trash character "" printed sometimes. 
8. Attach with Visual Studio to wish86tg.exe process. 
9. Set a breakpoint in the C:\yar\tcl-8.6.6\build\tcl8.6.6\win\tclWinNotify.c at line 520, which contains the words: 

status = 2; // set a breakpoint here - we maybe catched the issue


This line is modified by me especially to underline the earliest location where the problem shows up. 

10. Switch to editor window back.
11. Press and hold backspace.
12. Note that trash character appears after the breakpoint fires. 
13. c:\yar\tcl-8.6.6\build\build-debug-tcl.bat and  c:\yar\tcl-8.6.6\build\build-debug-tk.bat are provided for your convenience. 

The quote from comp.lang.tcl with my analysis: 

<<Tcl_WaitForEvent obtains windows messages from the queue and calls TranslateMessage and DispatchMessage on each. TranslateMessage generates WM_CHAR for WM_KEYDOWN and posts it to the event queue.
Here is the place where problem originate from. In the subsequent processing it is naively assumed that no messages are posted between WM_KEYDOWN and WM_CHAR, which is not always the case. I'm not a Tcl expert, but I guess code like "after 0", or "after $time" can post messages at any time. I experimentally found that if Tcl_WaitForEvent obtained WM_KEYDOWN, and when another WM_USER is on the queue (I believe it is actually WM_WAKEUP defined by Tcl), then I get garbage key presses. I don't know exactly what is WM_WAKEUP, but I know I can post many events to the Tcl or Tk event queue (don't remember actual difference) due to "fileevent $sock readable" handlers I have, which in turn do something on my edit widget where I have trash keypresses.
 
Windows messages seem to be obtained by Tk with TkWinChildProc - it calls Tk_TranslateWinEvent to convert windows message to Tk event, which in turns calls GetTranslatedKey for WM_KEYDOWN. GetTranslatedKey assumes (sic!) that WM_CHAR is on top of the queue. Most of the time it is true, but I see no proof why it should always be the case. It is just the naive belief, nothing more. >>

Thanks, budden, for confirming the fix. Closing

Hi! Seem to work now (as of 8.6.7-rc)

As soon as budden confirms that the patch works, this ticket can be closed.

You can download the sources with the patch applied here: http://core.tcl.tk/tk/info/7095b72171524268. Just click on the "Tarball" or "ZIP archive" button (whatever you prefer)

Regards, Jan NIjtmans

I want to congratulate the great team for this action !!!

To donate is best to the Tcl Association IMHO: http://wiki.tcl.tk/17045 Nevertheless, there is no address etc to donate.

Thanks, that's fine! I have two questions:
1. When it makes sense to download a new version with the patch applied? 
2. How do I donate?

Proposed fix is now on core-8-5-branch, core-8-6-branch and trunk.

Christian, I like this approach. Patch looks good to me, and works fine as far as I can see (I don't have the devices to really check it). I'll commit your patch in a few days (to core-8-5-branch, core-8-6-branch and trunk), unless François is faster than me ;-)

Most likely, the processing can be improved further, but let's start with this one. Those further improvements I would like to do for trunk only, but Christian's patch is safe for all branches.

Regarding the potential WM_QUIT, please review the changes in

  http://core.tcl.tk/tk/info/5d5b146b0b033bb6

which break the relevant loops when no WM_(SYS)CHAR is seen in the
filtered "PM_NOREMOVE"ing PeekMessage() calls.

Hi! Thanks! Your response was quite fast. 

As far as I get it, there are three versions of one patch. So I need to apply only the latest one:

--- tkWinX.c.orig       2016-09-10 20:44:19.991713371 +0200
+++ tkWinX.c    2016-09-24 11:35:49.803594037 +0200

I had the sources dated 2016-07-22, downloaded from http://www.tcl.tk/software/tcltk/download.html 

I applied the changes manually because the patch failed to my older version. After that, I checked my test case and most of the keybingings in my application. All is fine so far. 

But there is something to improve. 

1. What if there are extra WM_CHARs in the queue due to some queued keyboard input? I see XMaxTransChars == 4, so we can consume more WM_CHARs than was actually generated by the WM_KEYWDOWN initiated this call to GetTranslatedKey. Maybe I simply don't see why it is impossible, but I don't see. 

If that's actual problem, I hope one can use lParam to identify the correspondence between WM_CHAR and WM_KEYDOWN. But still I don't know what to do if there are several identical subsequent WM_KEYDOWNs. This might have something in common with auto-repeat bits in WM_CHAR/WM_KEYDOWN, but this might be tricky to process. 

Another alternative might be to post some "fake" WM_CHAR after the call to TranslateMessage so that GetTranslatedKey extracted it and consider it is a "stop" command. But I don't know Windows API well enough to be sure. 

2. According to MSDN, https://msdn.microsoft.com/en-us/library/windows/desktop/ms644943(v=vs.85).aspx , PeekMessage retrieves WM_QUIT regardless of filter, but I see no code to process this situation.

Christian,

great contributions !!!!

Budden,

could you please verify the two patches proposed by Wizard-Christian ?

• Rerstrict message filter of PeekMessage (Patch below dated 2016-09-24 08:44:02 )
• Eliminate GetMessage (Patch dated 2016-09-24 09:35:18)

I am sorry, can not make a branch, not on good computer for nearly 3 weeks.

Thank you all, Harald

The patch eliminating GetMessage() wasn't quite correct, it must be:

--- tkWinX.c.orig       2016-09-10 20:44:19.991713371 +0200
+++ tkWinX.c    2016-09-24 11:35:49.803594037 +0200
@@ -92,7 +92,7 @@
 static void            GenerateXEvent(HWND hwnd, UINT message,
                            WPARAM wParam, LPARAM lParam);
 static unsigned int    GetState(UINT message, WPARAM wParam, LPARAM lParam);
-static void            GetTranslatedKey(XKeyEvent *xkey);
+static void            GetTranslatedKey(XKeyEvent *xkey, UINT type);
 static void            UpdateInputLanguage(int charset);
 static int             HandleIMEComposition(HWND hwnd, LPARAM lParam);
 

@@ -1157,7 +1157,8 @@
            event.type = KeyPress;
            event.xany.send_event = -1;
            event.xkey.keycode = wParam;
-           GetTranslatedKey(&event.xkey);
+           GetTranslatedKey(&event.xkey, (message == WM_KEYDOWN) ? WM_CHAR :
+                   WM_SYSCHAR);
            break;
 
        case WM_SYSKEYUP:
@@ -1229,9 +1230,8 @@
                if (IsDBCSLeadByte((BYTE) wParam)) {
                    MSG msg;
 
-                   if ((PeekMessage(&msg, NULL, 0, 0, PM_NOREMOVE) != 0)
-                           && (msg.message == WM_CHAR)) {
-                       GetMessage(&msg, NULL, 0, 0);
+                   if (PeekMessage(&msg, NULL, WM_CHAR, WM_CHAR,
+                               PM_REMOVE) != 0) {
                        event.xkey.nbytes = 2;
                        event.xkey.trans_chars[1] = (char) msg.wParam;
                   }
@@ -1370,19 +1370,15 @@
 
 static void
 GetTranslatedKey(
-    XKeyEvent *xkey)
+    XKeyEvent *xkey,
+    UINT type)
 {
     MSG msg;
 
     xkey->nbytes = 0;
 
     while ((xkey->nbytes < XMaxTransChars)
-           && PeekMessageA(&msg, NULL, 0, 0, PM_NOREMOVE)) {
-       if ((msg.message != WM_CHAR) && (msg.message != WM_SYSCHAR)) {
-           break;
-       }
-
-       GetMessageA(&msg, NULL, 0, 0);
+           && (PeekMessageA(&msg, NULL, type, type, PM_REMOVE) != 0)) {
 
        /*
         * If this is a normal character message, we may need to strip off the

After reading the patch again, I think it can be simplified further:

--- tkWinX.c.orig       2016-09-10 20:44:19.991713371 +0200
+++ tkWinX.c    2016-09-24 10:56:50.881497805 +0200
@@ -92,7 +92,7 @@
 static void            GenerateXEvent(HWND hwnd, UINT message,
                            WPARAM wParam, LPARAM lParam);
 static unsigned int    GetState(UINT message, WPARAM wParam, LPARAM lParam);
-static void            GetTranslatedKey(XKeyEvent *xkey);
+static void            GetTranslatedKey(XKeyEvent *xkey, UINT type);
 static void            UpdateInputLanguage(int charset);
 static int             HandleIMEComposition(HWND hwnd, LPARAM lParam);
 

@@ -1157,7 +1157,8 @@
            event.type = KeyPress;
            event.xany.send_event = -1;
            event.xkey.keycode = wParam;
-           GetTranslatedKey(&event.xkey);
+           GetTranslatedKey(&event.xkey, (message == WM_KEYDOWN) ? WM_CHAR :
+                   WM_SYSCHAR);
            break;
 
        case WM_SYSKEYUP:
@@ -1229,9 +1230,7 @@
                if (IsDBCSLeadByte((BYTE) wParam)) {
                    MSG msg;
 
-                   if ((PeekMessage(&msg, NULL, 0, 0, PM_NOREMOVE) != 0)
-                           && (msg.message == WM_CHAR)) {
-                       GetMessage(&msg, NULL, 0, 0);
+                   if (PeekMessage(&msg, NULL, WM_CHAR, WM_CHAR, 0) != 0) {
                        event.xkey.nbytes = 2;
                        event.xkey.trans_chars[1] = (char) msg.wParam;
                   }
@@ -1370,19 +1369,15 @@
 
 static void
 GetTranslatedKey(
-    XKeyEvent *xkey)
+    XKeyEvent *xkey,
+    UINT type)
 {
     MSG msg;
 
     xkey->nbytes = 0;
 
     while ((xkey->nbytes < XMaxTransChars)
-           && PeekMessageA(&msg, NULL, 0, 0, PM_NOREMOVE)) {
-       if ((msg.message != WM_CHAR) && (msg.message != WM_SYSCHAR)) {
-           break;
-       }
-
-       GetMessageA(&msg, NULL, 0, 0);
+           && (PeekMessageA(&msg, NULL, type, type, 0) != 0)) {
 
        /*
         * If this is a normal character message, we may need to strip off the

If the problem is caused by interleaving of arbitrary WM_* messages in the
expected stream of WM_(SYS)KEYDOWN followed by zero or more WM_(SYS)CHAR
messages, then maybe it is sufficient to filter on WM_CHAR (and WM_SYSCHAR)
at the two places in tkWinX.c where PeekMessage() and GetMessage() are
used and not to break early due to other interleaved WM_* messages.

Potential patch against core-8-6-branch:

--- tkWinX.c.orig       2016-09-10 20:44:19.991713371 +0200
+++ tkWinX.c    2016-09-24 10:36:55.551724471 +0200
@@ -92,7 +92,7 @@
 static void            GenerateXEvent(HWND hwnd, UINT message,
                            WPARAM wParam, LPARAM lParam);
 static unsigned int    GetState(UINT message, WPARAM wParam, LPARAM lParam);
-static void            GetTranslatedKey(XKeyEvent *xkey);
+static void            GetTranslatedKey(XKeyEvent *xkey, UINT type);
 static void            UpdateInputLanguage(int charset);
 static int             HandleIMEComposition(HWND hwnd, LPARAM lParam);
 

@@ -1157,7 +1157,8 @@
            event.type = KeyPress;
            event.xany.send_event = -1;
            event.xkey.keycode = wParam;
-           GetTranslatedKey(&event.xkey);
+           GetTranslatedKey(&event.xkey, (message == WM_KEYDOWN) ? WM_CHAR :
+                   WM_SYSCHAR);
            break;
 
        case WM_SYSKEYUP:
@@ -1229,9 +1230,9 @@
                if (IsDBCSLeadByte((BYTE) wParam)) {
                    MSG msg;
 
-                   if ((PeekMessage(&msg, NULL, 0, 0, PM_NOREMOVE) != 0)
-                           && (msg.message == WM_CHAR)) {
-                       GetMessage(&msg, NULL, 0, 0);
+                   if (PeekMessage(&msg, NULL, WM_CHAR, WM_CHAR,
+                               PM_NOREMOVE) != 0) {
+                       GetMessage(&msg, NULL, WM_CHAR, WM_CHAR);
                        event.xkey.nbytes = 2;
                        event.xkey.trans_chars[1] = (char) msg.wParam;
                   }
@@ -1370,19 +1371,17 @@
 
 static void
 GetTranslatedKey(
-    XKeyEvent *xkey)
+    XKeyEvent *xkey,
+    UINT type)
 {
     MSG msg;
 
     xkey->nbytes = 0;
 
     while ((xkey->nbytes < XMaxTransChars)
-           && PeekMessageA(&msg, NULL, 0, 0, PM_NOREMOVE)) {
-       if ((msg.message != WM_CHAR) && (msg.message != WM_SYSCHAR)) {
-           break;
-       }
+           && PeekMessageA(&msg, NULL, type, type, PM_NOREMOVE)) {
 
-       GetMessageA(&msg, NULL, 0, 0);
+       GetMessageA(&msg, NULL, type, type);
 
        /*
         * If this is a normal character message, we may need to strip off the

I hope the issue would show in English version too. At least, the test case I suggested does not require entering Cyrillic characters.

Thank you for the ticket.

clt thread "unable to enter text into edit", 2016-09-20

I have an open-source application (8.6.6 version of Tk) which uses edit widget. As I type, some information is passed through sockets, and it is initiated by key events.

After switching to Windows 7 this does not work any more: some cyrillic characters are irrelevant. I type 19 cyrillic characters successfully and 20-th is irrelevant, then good characters again, then irrelevant again and so on. When I disable socket I/O, all works. ASCII characters are ok. This problem was in 8.6.4 also.

I do not use threaded features of tcl.

I tried debugging and found that something goes wrong here:

static void
GetTranslatedKey(XKeyEvent *xkey)
{
    MSG msg;
    xkey->nbytes = 0;
    while ((xkey->nbytes < XMaxTransChars)
	    && PeekMessageA(&msg, NULL, 0, 0, PM_NOREMOVE)) {
	if ((msg.message != WM_CHAR) && (msg.message != WM_SYSCHAR)) {
	    break;
	}
	GetMessageA(&msg, NULL, 0, 0);
}

Normally, I successfully find WM_CHAR with PeekMessageA. But sometimes PeekMessage sees message with msg.message = 1024 (WM_USER), and in this case logic breaks and irrelevant key is entered.

Tcl_WaitForEvent obtains windows messages from the queue and calls TranslateMessage and DispatchMessage on each. TranslateMessage generates WM_CHAR for WM_KEYDOWN and posts it to the event queue. Here is the place where problem originate from. In the subsequent processing it is naively assumed that no messages are posted between WM_KEYDOWN and WM_CHAR, which is not always the case. I'm not a Tcl expert, but I guess code like "after 0", or "after $time" can post messages at any time. I experimentally found that if Tcl_WaitForEvent obtained WM_KEYDOWN, and when another WM_USER is on the queue (I believe it is actually WM_WAKEUP defined by Tcl), then I get garbage key presses. I don't know exactly what is WM_WAKEUP, but I know I can post many events to the Tcl or Tk event queue (don't remember actual difference) due to "fileevent $sock readable" handlers I have, which in turn do something on my edit widget where I have trash keypresses.

Windows messages seem to be obtained by Tk with TkWinChildProc - it calls Tk_TranslateWinEvent to convert windows message to Tk event, which in turns calls GetTranslatedKey for WM_KEYDOWN. GetTranslatedKey assumes (sic!) that WM_CHAR is on top of the queue. Most of the time it is true, but I see no proof why it should always be the case. It is just the naive belief, nothing more.

HaO: The WM_USER messages are probably due to communication of the Socket message notifier thread to the main thread.

Thats why it appears only on network traffic.

APN:

Not directly relevant to your question, but for the benefit of anyone capable of fixing the bug, there is also (in theory) a possibility of buffer overflow here.

The while loop checks there is room for one byte, but for PowerPen devices (whatever those are), it happily sticks in a second byte without checking if there is actually room for it.

Conversation with apn on tcl chat:

I took a look at the sources. Something that needs to be fixed for sure.

Make that loop ignore intermediate posts in the queue, do PeekMessage for only one type ?

My first inclination is to do away with the ascii calls and use unicode apis which I hope will give the entire character in one go. But changing all the Windows calls to GetMessageW etc is a big step and I'd be worried about what effects it might have

The other option is to actually keep a state machine which will be non-trivial. I don't think just ignoring intermediate posts will do but I don't understand the processing there sufficiently.

The biggest issue with being a Tk C newbie is that unlike Tcl, it is very hard to test and verify you have not broken anything

In theory, it seems to be that any number of intermediate WM_ messages may occur between the two keyboard messages so I don't see an option other than keeping state. I would hope with the Unicode api, this does not happen

Other issue is how to actually test this stuff since it will not happen in English environs at least