Re: ole32:fix CoWaitForMultipleHandles cause RPC hang

[Sebastian Lackner]

On 11.12.2014 15:07, Changhui Liu wrote:
> Because there's a RPC request need to be handled by OLE apartment window proc apartment_wndproc.
> So, after received a WM_QUIT, 
> if the GUI thread in calling CoWaitForMultipleHandles don't handle message loop any more ,
> the RPC will never be handled, the RPC dispatch thread will be hang.

There are several issues with this patch. You wrote in your last mail:
> so this patch can't pass test on testbot.winehq.org, it only pass test on wine.
> The test result is here, https://testbot.winehq.org/JobDetails.pl?Key=10727.

It is not clear what you mean. The test should of course also pass on testbot.winehq.org, if it doesn't, this means that Wine doesn't properly reproduce the expected behaviour. You need to create a test so that it passes on both wine and the testbot, otherwise nothing is tested at all.

Other issues in your patch:

* It seems very unlikely that this is how quit messages are processed. Immediately calling PostQuitMessage(...) but continuing to process messages means that effectively WM_QUIT is lost.

* Always add a "\n" at the end of ok(...) messages.

* Try to avoid huge sleeps like Sleep(5000). This will probably cause the tests time out.

Unfortunately I didn't have much time to look into this issue myself so far, but I guess I'll have more time at the weekend.

Regards,
Sebastian

ole32:fix CoWaitForMultipleHandles cause RPC hang

[Changhui Liu]

Hi, I modified my patch, it has passed test on 
https://testbot.winehq.org/JobDetails.pl?Key=10745&log_202=1#k202

My test aims to reproduce the bug.

------------------ Original ------------------
From:  "Sebastian Lackner";
Date:  Thu, Dec 11, 2014 11:06 PM
To:  "liuchanghui"; 
Cc:  "wine-devel"; 
Subject:  Re: ole32:fix CoWaitForMultipleHandles cause RPC hang

On 11.12.2014 15:07, Changhui Liu wrote:
> Because there's a RPC request need to be handled by OLE apartment window proc apartment_wndproc.
> So, after received a WM_QUIT, 
> if the GUI thread in calling CoWaitForMultipleHandles don't handle message loop any more ,
> the RPC will never be handled, the RPC dispatch thread will be hang.

There are several issues with this patch. You wrote in your last mail:
> so this patch can't pass test on testbot.winehq.org, it only pass test on wine.
> The test result is here, https://testbot.winehq.org/JobDetails.pl?Key=10727.

It is not clear what you mean. The test should of course also pass on testbot.winehq.org, if it doesn't, this means that Wine doesn't properly reproduce the expected behaviour. You need to create a test so that it passes on both wine and the testbot, otherwise nothing is tested at all.

Other issues in your patch:

* It seems very unlikely that this is how quit messages are processed. Immediately calling PostQuitMessage(...) but continuing to process messages means that effectively WM_QUIT is lost.

* Always add a "\n" at the end of ok(...) messages.

* Try to avoid huge sleeps like Sleep(5000). This will probably cause the tests time out.

Unfortunately I didn't have much time to look into this issue myself so far, but I guess I'll have more time at the weekend.

Regards,
Sebastian

------------------
Regards,
Changhui

ole32:fix CoWaitForMultipleHandles cause RPC hang

[Sebastian Lackner]

On 12.12.2014 04:36, Changhui Liu wrote:
> Hi, I modified my patch, it has passed test on 
> https://testbot.winehq.org/JobDetails.pl?Key=10745&log_202=1#k202
>
> My test aims to reproduce the bug.

Sorry, but this patch is still not sufficient, it doesn't cover all situations how WM_QUIT messages are handled. As you can easily test with my current work-in-progress patch
https://github.com/wine-compholio/wine-staging/blob/master/patches/ole32-CoW...
your suggested fix breaks other stuff (which was working fine before), and so is most likely not correct.

I am not even sure if the problem is really in ole32, it might also be possible that the real problem is in the user32 message handling or probably somewhere else - but the only way to find that out is to write additional tests. Just testing this specific use-case will not be enough to justify a change, which might break other apps. ;)

Regards,
Sebastian

ole32:fix CoWaitForMultipleHandles cause RPC hang

[Changhui Liu]

Hi Sebastian,

I tested your current work-in-progress patch:
https://github.com/wine-compholio/wine-staging/blob/master/patches/ole32-CoW...

I have two puzzles about these code:
>   index = 0xdeadbeef;
>   PostMessageA(hWnd, WM_DDE_FIRST, 0, 0);
>   PostQuitMessage(44);
>   thread = CreateThread(NULL, 0, post_message_thread, hWnd, 0, &tid);
>   ok(thread != NULL, "CreateThread failed, error %u\n", GetLastError());
>   hr = CoWaitForMultipleHandles(0, 100, 2, handles, &index);
>   ok(hr == RPC_S_CALLPENDING, "expected RPC_S_CALLPENDING, got 0x%08x\n", hr);
>   ok(index == 0 || broken(index == 0xdeadbeef) /* Win 8 */, "expected index 0, got %u\n", index);
>   success = PeekMessageA(&msg, hWnd, WM_DDE_FIRST, WM_DDE_FIRST, PM_REMOVE);
>   ok(success, "PeekMessageA failed, error %u\n", GetLastError());

The first, why use the PeekMessage to check if received a WM_DDE_FIRST message?
Is there an real application do like that way?

In fact CoWaitForMultipleHandles has a message loop, the WM_DDE_FIRST message has been
dispatched to the window's WNDPROC. So the PeekMessageA certainly failed.

I think the correct test should be this: 
1, Define a custom WNDPROC function named cowait_test_wnd_proc .
2, Define a global int variable named g_count_of_wm_dde_first.
3, Set the value of g_count_of_wm_dde_first to zero before call CoWaitForMultipleHandles.
4, Increase the value of g_count_of_wm_dde_first by one, once received a WM_DDE_FIRST message in cowait_test_wnd_proc.
5, Check if the value of g_count_of_wm_dde_first is equal to 2 after CoWaitForMultipleHandles returned.

The second  is why use todo_wine ?
        todo_wine
        ok(!success, "PeekMessageA succeeded\n");
why not just write?
        ok(!success, "PeekMessageA succeeded\n");

Thank you.

------------------
Regards.

------------------ Original ------------------
From:  "Sebastian Lackner";
Date:  Fri, Dec 12, 2014 11:57 AM
To:  "Changhui Liu"; 
Cc:  "wine-devel"; 
Subject:  Re: ole32:fix CoWaitForMultipleHandles cause RPC hang

On 12.12.2014 04:36, Changhui Liu wrote:
> Hi, I modified my patch, it has passed test on 
> https://testbot.winehq.org/JobDetails.pl?Key=10745&log_202=1#k202
>
> My test aims to reproduce the bug.

Sorry, but this patch is still not sufficient, it doesn't cover all situations how WM_QUIT messages are handled. As you can easily test with my current work-in-progress patch
https://github.com/wine-compholio/wine-staging/blob/master/patches/ole32-CoW...
your suggested fix breaks other stuff (which was working fine before), and so is most likely not correct.

I am not even sure if the problem is really in ole32, it might also be possible that the real problem is in the user32 message handling or probably somewhere else - but the only way to find that out is to write additional tests. Just testing this specific use-case will not be enough to justify a change, which might break other apps. ;)

Regards,
Sebastian

ole32:fix CoWaitForMultipleHandles cause RPC hang

[Sebastian Lackner]

Hi,

On 13.12.2014 16:15, Changhui Liu wrote:
> Hi Sebastian,
>
> I tested your current work-in-progress patch:
> https://github.com/wine-compholio/wine-staging/blob/master/patches/ole32-CoW...
>
> I have two puzzles about these code:
> >   index = 0xdeadbeef;
> >   PostMessageA(hWnd, WM_DDE_FIRST, 0, 0);
> >   PostQuitMessage(44);
> >   thread = CreateThread(NULL, 0, post_message_thread, hWnd, 0, &tid);
> >   ok(thread != NULL, "CreateThread failed, error %u\n", GetLastError());
> >   hr = CoWaitForMultipleHandles(0, 100, 2, handles, &index);
> >   ok(hr == RPC_S_CALLPENDING, "expected RPC_S_CALLPENDING, got 0x%08x\n", hr);
> >   ok(index == 0 || broken(index == 0xdeadbeef) /* Win 8 */, "expected index 0, got %u\n", index);
> >   success = PeekMessageA(&msg, hWnd, WM_DDE_FIRST, WM_DDE_FIRST, PM_REMOVE);
> >   ok(success, "PeekMessageA failed, error %u\n", GetLastError());
>
> The first, why use the PeekMessage to check if received a WM_DDE_FIRST message?
> Is there an real application do like that way?

Of course there are multiple ways to test for window messages in the message queue, I just picked PeekMessageA because it doesn't require setting up a Wndproc.
> In fact CoWaitForMultipleHandles has a message loop, the WM_DDE_FIRST message has been
> dispatched to the window's WNDPROC. So the PeekMessageA certainly failed.

No, it doesn't. Feel free to run the tests yourself on a Windows machine, I have tested them with the Wine testbot and all tests pass on 2000/XP/Vista/8/2008/... - which means that Windows does not process all WM_DDE_FIRST messages.

This is also why I am unsure about how to fix it. When changing the behaviour in order to fix one of the tests which fails in Wine (marked with todo_wine), it breaks other tests which were working well before.
> I think the correct test should be this: 
> 1, Define a custom WNDPROC function named cowait_test_wnd_proc .
> 2, Define a global int variable named g_count_of_wm_dde_first.
> 3, Set the value of g_count_of_wm_dde_first to zero before call CoWaitForMultipleHandles.
> 4, Increase the value of g_count_of_wm_dde_first by one, once received a WM_DDE_FIRST message in cowait_test_wnd_proc.
> 5, Check if the value of g_count_of_wm_dde_first is equal to 2 after CoWaitForMultipleHandles returned.

This testing method would also be fine, but you would get 1 instead of 2. My patch doesn't show any test failures on Windows, which means that one of the WM_DDE_FIRST messages is still in the queue.
> The second  is why use todo_wine ?
>         todo_wine
>         ok(!success, "PeekMessageA succeeded\n");
> why not just write?
>         ok(!success, "PeekMessageA succeeded\n");

The Wine coding guideline does not allow to add new test failures, instead all tests which do not pass yet have to be marked with "todo_wine". This makes easily visible which tests need further work just by looking at the source code, moreover it avoids introducing regressions (breaking tests which were working well before). Take a look at some of the other Wine tests, this is not the only place where its used.
> Thank you.

Regards,
Sebastian