[PATCH v2 0/1] MR9260: winemac: Make macdrv_ime_process_key synchronous.
This is a simplification of https://gitlab.winehq.org/wine/wine/-/merge_requests/9246. -- v2: winemac: Make macdrv_ime_process_key synchronous. https://gitlab.winehq.org/wine/wine/-/merge_requests/9260
From: Marc-Aurel Zent <mzent(a)codeweavers.com> --- dlls/winemac.drv/cocoa_window.m | 18 ++++++------------ dlls/winemac.drv/event.c | 18 ------------------ dlls/winemac.drv/keyboard.c | 20 ++++++-------------- dlls/winemac.drv/macdrv.h | 1 - dlls/winemac.drv/macdrv_cocoa.h | 9 +-------- dlls/winemac.drv/macdrv_main.c | 4 ---- 6 files changed, 13 insertions(+), 57 deletions(-) diff --git a/dlls/winemac.drv/cocoa_window.m b/dlls/winemac.drv/cocoa_window.m index 2dd7d180c24..7728857a479 100644 --- a/dlls/winemac.drv/cocoa_window.m +++ b/dlls/winemac.drv/cocoa_window.m @@ -4040,12 +4040,11 @@ uint32_t macdrv_window_background_color(void) * processed by input sources (AKA IMEs). This is only called when there is an * active non-keyboard input source. */ -void macdrv_ime_process_key(int keyc, unsigned int flags, int repeat, void *himc, - int *done, void *ime_done_event) +int macdrv_ime_process_key(int keyc, unsigned int flags, int repeat, void *himc) { - OnMainThreadAsync(^{ - BOOL ret; - macdrv_event* event; + __block BOOL ret; + + OnMainThread(^{ WineWindow* window = (WineWindow*)[NSApp keyWindow]; if (![window isKindOfClass:[WineWindow class]]) { @@ -4077,14 +4076,9 @@ void macdrv_ime_process_key(int keyc, unsigned int flags, int repeat, void *himc } else ret = FALSE; - - event = macdrv_create_event(SENT_TEXT_INPUT, window); - event->sent_text_input.handled = ret; - event->sent_text_input.done = done; - event->sent_text_input.ime_done_event = ime_done_event; - [[window queue] postEvent:event]; - macdrv_release_event(event); }); + + return (int)ret; } void macdrv_clear_ime_text(void) diff --git a/dlls/winemac.drv/event.c b/dlls/winemac.drv/event.c index 6841fd3603e..05f2eca50e3 100644 --- a/dlls/winemac.drv/event.c +++ b/dlls/winemac.drv/event.c @@ -61,7 +61,6 @@ static const char *dbgstr_event(int type) "QUERY_EVENT_NO_PREEMPT_WAIT", "REASSERT_WINDOW_POSITION", "RELEASE_CAPTURE", - "SENT_TEXT_INPUT", "STATUS_ITEM_MOUSE_BUTTON", "STATUS_ITEM_MOUSE_MOVE", "WINDOW_BROUGHT_FORWARD", @@ -138,7 +137,6 @@ static macdrv_event_mask get_event_mask(DWORD mask) event_mask |= event_mask_for_type(QUERY_EVENT_NO_PREEMPT_WAIT); event_mask |= event_mask_for_type(REASSERT_WINDOW_POSITION); event_mask |= event_mask_for_type(RELEASE_CAPTURE); - event_mask |= event_mask_for_type(SENT_TEXT_INPUT); event_mask |= event_mask_for_type(WINDOW_BROUGHT_FORWARD); event_mask |= event_mask_for_type(WINDOW_CLOSE_REQUESTED); event_mask |= event_mask_for_type(WINDOW_DRAG_BEGIN); @@ -185,19 +183,6 @@ static void macdrv_im_set_text(const macdrv_event *event) free(text); } -/*********************************************************************** - * macdrv_sent_text_input - */ -static void macdrv_sent_text_input(const macdrv_event *event) -{ - HANDLE ime_done_event = event->sent_text_input.ime_done_event; - - TRACE_(imm)("handled: %s\n", event->sent_text_input.handled ? "TRUE" : "FALSE"); - - *event->sent_text_input.done = event->sent_text_input.handled ? 1 : -1; - if (ime_done_event) NtSetEvent(ime_done_event, NULL); -} - /************************************************************************** * drag_operations_to_dropeffects @@ -475,9 +460,6 @@ void macdrv_handle_event(const macdrv_event *event) case RELEASE_CAPTURE: macdrv_release_capture(hwnd, event); break; - case SENT_TEXT_INPUT: - macdrv_sent_text_input(event); - break; case STATUS_ITEM_MOUSE_BUTTON: macdrv_status_item_mouse_button(event); break; diff --git a/dlls/winemac.drv/keyboard.c b/dlls/winemac.drv/keyboard.c index 6c615d10e3d..527ee85ff57 100644 --- a/dlls/winemac.drv/keyboard.c +++ b/dlls/winemac.drv/keyboard.c @@ -1089,7 +1089,8 @@ UINT macdrv_ImeProcessKey(HIMC himc, UINT wparam, UINT lparam, const BYTE *key_s WORD scan = HIWORD(lparam) & 0x1ff, vkey = LOWORD(wparam); BOOL repeat = !!(lparam >> 30), pressed = !(lparam >> 31); unsigned int flags; - int keyc, done = 0; + int keyc; + UINT ret; TRACE("himc %p, scan %#x, vkey %#x, repeat %u, pressed %u\n", himc, scan, vkey, repeat, pressed); @@ -1138,19 +1139,10 @@ UINT macdrv_ImeProcessKey(HIMC himc, UINT wparam, UINT lparam, const BYTE *key_s if (keyc >= ARRAY_SIZE(thread_data->keyc2vkey)) return 0; TRACE("flags 0x%08x keyc 0x%04x\n", flags, keyc); - - if (!thread_data->ime_done_event) - { - NTSTATUS status; - status = NtCreateEvent(&thread_data->ime_done_event, EVENT_ALL_ACCESS, NULL, - SynchronizationEvent, FALSE); - if (status != STATUS_SUCCESS) ERR("NtCreateEvent call failed.\n"); - } - - macdrv_ime_process_key(keyc, flags, repeat, himc, &done, thread_data->ime_done_event); - NtUserMsgWaitForMultipleObjectsEx(1, &thread_data->ime_done_event, INFINITE, 0, 0); - - return done > 0; + ret = (UINT)macdrv_ime_process_key(keyc, flags, repeat, himc); + macdrv_ProcessEvents(QS_POSTMESSAGE | QS_SENDMESSAGE); + TRACE("returning %u\n", ret); + return ret; } diff --git a/dlls/winemac.drv/macdrv.h b/dlls/winemac.drv/macdrv.h index 5b9515f086f..22e96fa1129 100644 --- a/dlls/winemac.drv/macdrv.h +++ b/dlls/winemac.drv/macdrv.h @@ -112,7 +112,6 @@ static inline RECT rect_from_cgrect(CGRect cgrect) HKL active_keyboard_layout; WORD keyc2vkey[128]; WORD keyc2scan[128]; - HANDLE ime_done_event; }; extern struct macdrv_thread_data *macdrv_init_thread_data(void); diff --git a/dlls/winemac.drv/macdrv_cocoa.h b/dlls/winemac.drv/macdrv_cocoa.h index bd6af9ed25e..e6999670cb5 100644 --- a/dlls/winemac.drv/macdrv_cocoa.h +++ b/dlls/winemac.drv/macdrv_cocoa.h @@ -291,7 +291,6 @@ extern int macdrv_set_display_mode(const struct macdrv_display* display, QUERY_EVENT_NO_PREEMPT_WAIT, REASSERT_WINDOW_POSITION, RELEASE_CAPTURE, - SENT_TEXT_INPUT, STATUS_ITEM_MOUSE_BUTTON, STATUS_ITEM_MOUSE_MOVE, WINDOW_BROUGHT_FORWARD, @@ -378,11 +377,6 @@ extern int macdrv_set_display_mode(const struct macdrv_display* display, struct { struct macdrv_query *query; } query_event; - struct { - int handled; - int *done; - void *ime_done_event; - } sent_text_input; struct { macdrv_status_item item; int button; @@ -546,8 +540,7 @@ extern void macdrv_order_cocoa_window(macdrv_window w, macdrv_window prev, extern int macdrv_get_view_backing_size(macdrv_view v, int backing_size[2]); extern void macdrv_set_view_backing_size(macdrv_view v, const int backing_size[2]); extern uint32_t macdrv_window_background_color(void); -extern void macdrv_ime_process_key(int keyc, unsigned int flags, int repeat, void *data, - int *done, void *ime_done_event); +extern int macdrv_ime_process_key(int keyc, unsigned int flags, int repeat, void *data); extern int macdrv_is_any_wine_window_visible(void); diff --git a/dlls/winemac.drv/macdrv_main.c b/dlls/winemac.drv/macdrv_main.c index f5e2b5e08d2..20295d1c6ee 100644 --- a/dlls/winemac.drv/macdrv_main.c +++ b/dlls/winemac.drv/macdrv_main.c @@ -464,8 +464,6 @@ void macdrv_ThreadDetach(void) macdrv_destroy_event_queue(data->queue); if (data->keyboard_layout_uchr) CFRelease(data->keyboard_layout_uchr); - if (data->ime_done_event) - NtClose(data->ime_done_event); free(data); /* clear data in case we get re-entered from user32 before the thread is truly dead */ NtUserGetThreadInfo()->driver_data = 0; @@ -525,8 +523,6 @@ struct macdrv_thread_data *macdrv_init_thread_data(void) NtTerminateProcess(0, 1); } - data->ime_done_event = NULL; - macdrv_get_input_source_info(&data->keyboard_layout_uchr, &data->keyboard_type, &data->iso_keyboard, &input_source); data->active_keyboard_layout = macdrv_get_hkl_from_source(input_source); CFRelease(input_source); -- GitLab https://gitlab.winehq.org/wine/wine/-/merge_requests/9260
I looked inside the OnMainThread, and perhaps there might be an error in my reply. I'll describe the problematic situation:
Should be fixed with the latest version. It is indeed as you already found an issue with the order of operations with the `IM_SET_TEXT` call. I added `macdrv_ProcessEvents(QS_POSTMESSAGE | QS_SENDMESSAGE)`, which makes this MR equivalent to the previous version, but still feels noticeably better. Regarding Japanese IME, there are many issues with it currently unfortunately, but these are probably best tackled in different MRs...
In addition to this MR, I tested whether it's possible to drop the IM_SET_TEXT code. It's a proof-of-concept patch and needs cleanup. It seems to work well, including the issue posted above.
Thanks for this, I don't think changing the signature of `ImeProcessKey()` is something we can do, but getting rid of some of these events if possible would be nicer indeed (or just making macdrv event handling much faster in general). -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_119669
On Sat Oct 25 13:09:04 2025 +0000, Marc-Aurel Zent wrote:
I looked inside the OnMainThread, and perhaps there might be an error in my reply. I'll describe the problematic situation: Should be fixed with the latest version. It is indeed as you already found an issue with the order of operations with the `IM_SET_TEXT` call. I added `macdrv_ProcessEvents(QS_POSTMESSAGE | QS_SENDMESSAGE)`, which makes this MR equivalent to the previous version, but still feels noticeably better. Regarding Japanese IME, there are many issues with it currently unfortunately, but these are probably best tackled in different MRs... In addition to this MR, I tested whether it's possible to drop the IM_SET_TEXT code. It's a proof-of-concept patch and needs cleanup. It seems to work well, including the issue posted above. Thanks for this, I don't think changing the signature of `ImeProcessKey()` is something we can do, but getting rid of some of these events if possible would be nicer indeed (or just making macdrv event handling much faster in general). After examining macdrv_ProcessEvents and NtUserMsgWaitForMultipleObjectsEx, I finally understand the meaning of your comment in !9246. Thank you.
Since NSTextInputContext handleEvent operates synchronously, IM_SET_TEXT can also be removed. The provided POC patch is poorly written in many places. I'm embarrassed. If implemented, I believe we could receive the hwnd via macdrv_ime_process_key. Something like ‘struct ime_result’. But it's not strictly necessary at this point. Patch v2 appears to work fine with the Korean keyboard. In my opinion, the Japanese keyboard issue should also be resolved in this MR to avoid regression. The current state is that Japanese sentence input is very slow. This happens frequently because the ‘RETURN’ key is used as the commit key. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_119680
On Sat Oct 25 21:26:27 2025 +0000, Byeongsik Jeon wrote:
After examining macdrv_ProcessEvents and NtUserMsgWaitForMultipleObjectsEx, I finally understand the meaning of your comment in !9246. Thank you. Since NSTextInputContext handleEvent operates synchronously, IM_SET_TEXT can also be removed. The provided POC patch is poorly written in many places. I'm embarrassed. If implemented, I believe we could receive the hwnd via macdrv_ime_process_key. Something like ‘struct ime_result’. But it's not strictly necessary at this point. Patch v2 appears to work fine with the Korean keyboard. In my opinion, the Japanese keyboard issue should also be resolved in this MR to avoid regression. The current state is that Japanese keyboard is very slow. This happens frequently because the ‘RETURN’ key is used as the commit key. Reverting !9246 and replacing NtUserMsgWaitForMultipleObjectEx with macdrv_ProcessEvents resolves the issue. I had introduced the event handle because I misunderstood the meaning of ‘QS_POSTMESSAGE | QS_SENDMESSAGE’.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_119681
On Sun Oct 26 14:59:09 2025 +0000, Byeongsik Jeon wrote:
Reverting !9246 and replacing NtUserMsgWaitForMultipleObjectsEx with macdrv_ProcessEvents resolves the issue. I had introduced the event handle because I misunderstood the meaning of ‘QS_POSTMESSAGE | QS_SENDMESSAGE’. Update: This is essentially equivalent to “while(!done) NtUserMsgWaitForMultipleObjectsEx(0, NULL, 0, 0, 0);”, so it is meaningless. The Japanese keyboard issue is caused by the timeout:0.3 in firstRectForCharacterRange:. This causes a 0.3-second delay. It appears to be related to QUERY_EVENT within OnMainThread.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_119695
On Sun Oct 26 15:32:03 2025 +0000, Byeongsik Jeon wrote:
The Japanese keyboard issue is caused by the timeout:0.3 in firstRectForCharacterRange:. This causes a 0.3s delay. ~~It appears to be related to QUERY_EVENT within OnMainThread.~~ Update: I haven't quite grasped the code path yet, but anyway, I think starting here should be fine. Since macdrv_ime_process_key uses OnThreadMain, the WineQueryNoPreemptWait flag is no longer needed. This was introduced in ! 33610da.
Unlike Korean keyboard, Japanese keyboard IME processing calls firstRectForCharacterRange earlier. When the Japanese keyboard response is delayed, it appears to additionally call the firstRectForCharacterRange method, which can occur after macdrv_ProcessEvents. Due to delayed query event processing, a timeout occurs. The number of firstRectForCharacterRange/query_ime_char_rect pairs differs. Like other query events, this event should also be handled in OnMainThread. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_119733
On Mon Oct 27 08:04:14 2025 +0000, Byeongsik Jeon wrote:
Since macdrv_ime_process_key uses OnThreadMain, the WineQueryNoPreemptWait flag is no longer needed. This was introduced in 33610da. ~~Unlike Korean keyboard, Japanese keyboard IME processing calls firstRectForCharacterRange earlier. When the Japanese keyboard response is delayed, it appears to additionally call the firstRectForCharacterRange method, which can occur after macdrv_ProcessEvents. Due to delayed query event processing, a timeout occurs. The number of firstRectForCharacterRange/query_ime_char_rect pairs differs.~~ Like other query events, this event should also be handled in OnMainThread. **Update:** Hmm... Sorry for posting that crappy interpretation. Looking more closely at the macdrv source, it seems there was an issue with the interpretation. Something must have gone wrong with the tests. There's a timing problem, but the number of firstRectForCharacterRange/query_ime_char_rect pairs matches. The conclusion seems the same, but the interpretation needs further examination. The waitUntilQueryDone method waits for the execution of query_ime_char_rect and eventually reaches a timeout.
By OnMainThread: ``` -[WineContentView firstRectForCharacterRange:actualRange:] -[WineApplicationController waitUntilQueryDone:timeout:processEvents:] *done=0 -[WineContentView setMarkedText:selectedRange:replacementRange:] -[WineContentView firstRectForCharacterRange:actualRange:] -[WineApplicationController waitUntilQueryDone:timeout:processEvents:] *done=0 ``` By macdrv_ProcessEvents: ``` 0024:trace:ime:query_ime_char_rect win 0x10118/0x7fcbe6144260 himc 0x1011e range 0-0 0024:trace:ime:query_ime_char_rect -> range 0-0 rect (134,442)-(137,481) 0024:trace:ime:macdrv_im_set_text win 0x10118/0x7fcbe6144260 himc 0x1011e text L"n" complete 0 0024:trace:ime:query_ime_char_rect win 0x10118/0x7fcbe6144260 himc 0x1011e range 0-1 0024:trace:ime:query_ime_char_rect -> range 0-1 rect (134,442)-(137,481) ``` Without the WineQueryNoPreemptWait flag, it executes as follows: ``` -[WineContentView firstRectForCharacterRange:actualRange:] 0024:trace:ime:query_ime_char_rect win 0x10118/0x7fcbe6144260 himc 0x1011e range 0-0 0024:trace:ime:query_ime_char_rect -> range 0-0 rect (134,442)-(137,481) -[WineApplicationController waitUntilQueryDone:timeout:processEvents:] *done=1 ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_119741
On Mon Oct 27 09:38:46 2025 +0000, Byeongsik Jeon wrote:
The waitUntilQueryDone method waits for the execution of query_ime_char_rect and eventually reaches a timeout. By OnMainThread: ``` -[WineContentView firstRectForCharacterRange:actualRange:] -[WineApplicationController waitUntilQueryDone:timeout:processEvents:] *done=0 -[WineContentView setMarkedText:selectedRange:replacementRange:] -[WineContentView firstRectForCharacterRange:actualRange:] -[WineApplicationController waitUntilQueryDone:timeout:processEvents:] *done=0 ``` By macdrv_ProcessEvents: ``` 0024:trace:ime:query_ime_char_rect win 0x10118/0x7fcbe6144260 himc 0x1011e range 0-0 0024:trace:ime:query_ime_char_rect -> range 0-0 rect (134,442)-(137,481) 0024:trace:ime:macdrv_im_set_text win 0x10118/0x7fcbe6144260 himc 0x1011e text L"n" complete 0 0024:trace:ime:query_ime_char_rect win 0x10118/0x7fcbe6144260 himc 0x1011e range 0-1 0024:trace:ime:query_ime_char_rect -> range 0-1 rect (134,442)-(137,481) ``` Without the WineQueryNoPreemptWait flag, it executes as follows: ``` -[WineContentView firstRectForCharacterRange:actualRange:] 0024:trace:ime:query_ime_char_rect win 0x10118/0x7fcbe6144260 himc 0x1011e range 0-0 0024:trace:ime:query_ime_char_rect -> range 0-0 rect (134,442)-(137,481) -[WineApplicationController waitUntilQueryDone:timeout:processEvents:] *done=1 ``` Interesting, I still believe that this sometimes deadlock (with a luckily .3s timeout) with the Japanese IME, is not a regression caused by this commit per se, just exasperated by making `macdrv_ImeProcessKey` faster.
I think we could get rid of the original race described in https://gitlab.winehq.org/wine/wine/-/commit/33610da6b4f5d9ad052173f005fdb73... by getting rid of IM_SET_TEXT and then effectively reverting that commit, removing this deadlock. I'll poke at this again later today; thanks for the review, testing and insight so far! -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_119742
This merge request was approved by Rémi Bernon. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260
Rémi Bernon (@rbernon) commented about dlls/winemac.drv/keyboard.c:
TRACE("flags 0x%08x keyc 0x%04x\n", flags, keyc); - - if (!thread_data->ime_done_event) - { - NTSTATUS status; - status = NtCreateEvent(&thread_data->ime_done_event, EVENT_ALL_ACCESS, NULL, - SynchronizationEvent, FALSE); - if (status != STATUS_SUCCESS) ERR("NtCreateEvent call failed.\n"); - } - - macdrv_ime_process_key(keyc, flags, repeat, himc, &done, thread_data->ime_done_event); - NtUserMsgWaitForMultipleObjectsEx(1, &thread_data->ime_done_event, INFINITE, 0, 0); - - return done > 0; + ret = (UINT)macdrv_ime_process_key(keyc, flags, repeat, himc); + macdrv_ProcessEvents(QS_POSTMESSAGE | QS_SENDMESSAGE);
I don't think it's good to call `macdrv_ProcessEvents` directly. Why is it still necessary if the key processing is synchronous? -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_120439
On Mon Nov 3 08:07:50 2025 +0000, Rémi Bernon wrote:
I don't think it's good to call `macdrv_ProcessEvents` directly. Why is it still necessary if the key processing is synchronous? With the current design we need to handle `IM_SET_TEXT` (which is generated by the key processing itself) before `macdrv_ImeProcessKey()` returns, otherwise the issue @bsjeon described above happens.
Of course this is still not completely race-free (but the previous version also wasn't), since the thread running `- (void) completeText:(NSString*)text` and generating this event can be preempted at any time. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_120441
On Mon Nov 3 08:13:53 2025 +0000, Marc-Aurel Zent wrote:
With the current design we need to handle `IM_SET_TEXT` (which is generated by the key processing itself) before `macdrv_ImeProcessKey()` returns, otherwise the issue @bsjeon described above happens. Of course this is still not completely race-free (but the previous version also wasn't), since the thread running `- (void) completeText:(NSString*)text` and generating this event can be preempted at any time. In that case maybe going through a `NtUserMsgWaitForMultipleObjectsEx` call is the best option for now if we cannot get rid of the IM_SET_TEXT event.
-- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_120451
On Mon Nov 3 08:33:51 2025 +0000, Rémi Bernon wrote:
In that case maybe going through a `NtUserMsgWaitForMultipleObjectsEx` call is the best option for now if we cannot get rid of the IM_SET_TEXT event. Hm what about something like
``` while (macdrv_copy_event_from_queue(thread_data->queue, im_set_text_mask, &event)) { macdrv_handle_event(event); macdrv_release_event(event); } ``` -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_120456
On Mon Nov 3 08:59:03 2025 +0000, Marc-Aurel Zent wrote:
Hm what about something like ``` while (macdrv_copy_event_from_queue(thread_data->queue, im_set_text_mask, &event)) { macdrv_handle_event(event); macdrv_release_event(event); } ``` `NtUserMsgWaitForMultipleObjectsEx` is now more tightly coupled with `ProcessEvents` and wineserver message queue fd polling. There's side effects like notifying wineserver after all events have been processed that need to be taken into account and I don't think it's a good idea to try to extract only some of the logic out.
Either we can get rid of the IM_SET_TEXT event too in this case (which seems doable) or we need to call `NtUserMsgWaitForMultipleObjectsEx`. -- https://gitlab.winehq.org/wine/wine/-/merge_requests/9260#note_120461
participants (4)
-
Byeongsik Jeon (＠bsjeon) -
Marc-Aurel Zent
-
Marc-Aurel Zent (＠mzent)
-
Rémi Bernon