Re: [v3 PATCH] kernel32: Fix RegisterWaitForSingleObject for console handles

[Sebastian Lackner]

On 17.08.2016 21:02, Keno Fischer wrote:
> Minimal change from v2, just an extra Sleep(100) before UnregisterWait
> to hopefully fix the failure identified by the testbot.
>
> 0001-kernel32-Fix-RegisterWaitForSingleObject-for-console.patch
>
> From 50347e64eedb1c07f1c9aedf253ec78eac4d3476 Mon Sep 17 00:00:00 2001
> From: Keno Fischer 
> Date: Tue, 16 Aug 2016 14:35:27 -0400
> Subject: [PATCH] kernel32: Fix RegisterWaitForSingleObject for console handles
>
> WaitForSingleObject contains a special case to wait on the correct
> handle if the passed in handle is a console object. However,
> RegisterWaitForSingleObject was missing this special case. This
> functionality was used e.g., by the libuv I/O library, causing
> any users thereof to not receive input under wine,
> e.g. fixes https://github.com/JuliaLang/julia/issues/18006.
> Signed-off-by: Keno Fischer
>
> dlls/kernel32/sync.c          | 37 +++++++++++++++--------------
>  dlls/kernel32/tests/console.c | 54 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 74 insertions(+), 17 deletions(-)

Thanks for all your effort to get this bug fixed. :)
Unfortunately you'll have to send another iteration, C++ - style comments ("//")
are not allowed in Wine source code, only C-style comments ("/* ... */").
If you don't mind, I also have some other suggestions how to improve the code a bit.
> diff --git a/dlls/kernel32/sync.c b/dlls/kernel32/sync.c
> index c10fd01..2d6625b 100644
> --- a/dlls/kernel32/sync.c
> +++ b/dlls/kernel32/sync.c
> @@ -148,6 +148,23 @@ DWORD WINAPI WaitForMultipleObjects( DWORD count, const HANDLE *handles,
>      return WaitForMultipleObjectsEx( count, handles, wait_all, timeout, FALSE );
>  }
>
> +static HANDLE NormalizeHandleIfConsole(HANDLE handle)

Usually lowercase names (with underscores) are preferred for Wine internal
functions, otherwise it looks like a WINAPI function. ;)
> +{
> - if ((handle == (HANDLE)STD_INPUT_HANDLE) ||
> -
>    (handle == (HANDLE)STD_OUTPUT_HANDLE) ||
> -
>    (handle == (HANDLE)STD_ERROR_HANDLE))
> -
>    handle = GetStdHandle( HandleToULong(handle) );
> -
> - /* yes, even screen buffer console handles are waitable, and are
> -
> * handled as a handle to the console itself !!
> -
> */
> - if (is_console_handle(handle))
> - {
> -
>    if (VerifyConsoleIoHandle(handle))
> -
>        handle = GetConsoleInputWaitHandle();
> - }
> - return handle;
>
> +}
>
> /***********************************************************************
> -
>       WaitForMultipleObjectsEx   (KERNEL32.@)
>
> @@ -167,23 +184,7 @@ DWORD WINAPI WaitForMultipleObjectsEx( DWORD count, const HANDLE *handles,
>          return WAIT_FAILED;
>      }
>      for (i = 0; i < count; i++)
> - {
> -
>    if ((handles[i] == (HANDLE)STD_INPUT_HANDLE) ||
> -
>        (handles[i] == (HANDLE)STD_OUTPUT_HANDLE) ||
> -
>        (handles[i] == (HANDLE)STD_ERROR_HANDLE))
> -
>        hloc[i] = GetStdHandle( HandleToULong(handles[i]) );
> -
>    else
> -
>        hloc[i] = handles[i];
> -
> -
>    /* yes, even screen buffer console handles are waitable, and are
> -
>     * handled as a handle to the console itself !!
> -
>     */
> -
>    if (is_console_handle(hloc[i]))
> -
>    {
> -
>        if (VerifyConsoleIoHandle(hloc[i]))
> -
>            hloc[i] = GetConsoleInputWaitHandle();
> -
>    }
> - }
> -
>    hloc[i] = NormalizeHandleIfConsole(handles[i]);
>
> status = NtWaitForMultipleObjects( count, hloc, !wait_all, alertable,
>                                       get_nt_timeout( &time, timeout ) );
>
> @@ -209,6 +210,7 @@ BOOL WINAPI RegisterWaitForSingleObject(PHANDLE phNewWaitObject, HANDLE hObject,
>      TRACE("%p %p %p %p %d %d\n",
>            phNewWaitObject,hObject,Callback,Context,dwMilliseconds,dwFlags);
> - hObject = NormalizeHandleIfConsole(hObject);
> status = RtlRegisterWait( phNewWaitObject, hObject, Callback, Context, dwMilliseconds, dwFlags );
> if (status != STATUS_SUCCESS)
> {
>
> @@ -231,6 +233,7 @@ HANDLE WINAPI RegisterWaitForSingleObjectEx( HANDLE hObject,
>      TRACE("%p %p %p %d %d\n",
>            hObject,Callback,Context,dwMilliseconds,dwFlags);
> - hObject = NormalizeHandleIfConsole(hObject);
> status = RtlRegisterWait( &hNewWaitObject, hObject, Callback, Context, dwMilliseconds, dwFlags );
> if (status != STATUS_SUCCESS)
> {
>
> diff --git a/dlls/kernel32/tests/console.c b/dlls/kernel32/tests/console.c
> index 5b66d9f..88ef2b8 100644
> --- a/dlls/kernel32/tests/console.c
> +++ b/dlls/kernel32/tests/console.c
> @@ -908,6 +908,58 @@ static void testScreenBuffer(HANDLE hConOut)
>      SetConsoleOutputCP(oldcp);
>  }
>
> +static void CALLBACK signaled_function(void *p, BOOLEAN timeout)
> +{
> - HANDLE event = p;
> - SetEvent(event);
> - ok(!timeout, "wait shouldn't have timed out\n");
>
> +}
> -
>
> +static void testWaitForConsoleInput(HANDLE input_handle)
> +{
> - HANDLE wait_handle;
> - HANDLE complete_event;
> - INPUT_RECORD record;
> - DWORD events_written;
> - DWORD wait_ret;
> - BOOL ret;
> -
> - complete_event = CreateEventW(NULL, FALSE, FALSE, NULL);
> -
> - // Test success case
> - ret = RegisterWaitForSingleObject(&wait_handle, input_handle, signaled_function, complete_event, INFINITE, WT_EXECUTEONLYONCE);
> - ok(ret == TRUE, "Expected RegisterWaitForSingleObject to return TRUE, got %d\n", ret);
> - /* give worker thread a chance to start up */
> - Sleep(100);
> - record.EventType = KEY_EVENT;
> - record.Event.KeyEvent.bKeyDown = 1;
> - record.Event.KeyEvent.wRepeatCount = 1;
> - record.Event.KeyEvent.wVirtualKeyCode = VK_RETURN;
> - record.Event.KeyEvent.wVirtualScanCode = VK_RETURN;
> - record.Event.KeyEvent.uChar.UnicodeChar = L'\r';

The L prefix should be omitted here. Wine can't use the Compilers wchar_t
features because they might have a different size than WCHAR on Windows.
> - record.Event.KeyEvent.dwControlKeyState = 0;
> - ret = WriteConsoleInputW(input_handle, &record, 1, &events_written);
> - ok(ret == TRUE, "Expected WriteConsoleInputW to return TRUE, got %d\n", ret);
> - wait_ret = WaitForSingleObject(complete_event, INFINITE);
> - ok(wait_ret == WAIT_OBJECT_0, "Expected the handle to be signaled\n");
> - /* give worker thread chance to complete */
> - Sleep(100);
> - ret = UnregisterWait(wait_handle);
> - ok(ret, "UnregisterWait failed with error %d\n", GetLastError());

According to MSDN "If any callback functions associated with the timer have not
completed when UnregisterWait is called, UnregisterWait unregisters the wait on
the callback functions and fails with the ERROR_IO_PENDING error code. The error
code does not indicate that the function has failed, and the function does not
need to be called again."

Since tests should always run as quickly as possible I would suggest to omit the
Sleep and remove the return value check here (or if you prefer, explicitly treat
ERROR_IO_PENDING as success).
> -
> - // Test timeout case
> - FlushConsoleInputBuffer(input_handle);
> - ret = RegisterWaitForSingleObject(&wait_handle, input_handle, signaled_function, complete_event, INFINITE, WT_EXECUTEONLYONCE);
> - wait_ret = WaitForSingleObject(complete_event, 100);
> - ok(wait_ret == WAIT_TIMEOUT, "Expected the wait to time out\n");
> - ret = UnregisterWait(wait_handle);
> - ok(ret, "UnregisterWait failed with error %d\n", GetLastError());
> -
> - // Clean up
> - ret = CloseHandle(complete_event);
> - ok(!!ret, "Failed to close event handle, last error %#x.\n", GetLastError());

CloseHandle returns a BOOL, so there should be no need to use "!!". Also I would
suggest to use a consistent way of tracing GetLastError() within the patch, above
you used "%d".
> +}
> -
>
> static void test_GetSetConsoleInputExeName(void)
>  {
>      BOOL ret;
> @@ -3044,6 +3096,8 @@ START_TEST(console)
>      testScroll(hConOut, sbi.dwSize);
>      /* will test sb creation / modification / codepage handling */
>      testScreenBuffer(hConOut);
> -
>
> /* Test waiting for a console handle */
> -
>
> testWaitForConsoleInput(hConIn);
>
> /* clear duplicated console font table */
> CloseHandle(hConIn);
>
> -- 2.7.4