Dmitry Timoshkov dmitry@baikal.ru writes:
@@ -2931,7 +2931,10 @@ static BOOL peek_message( MSG *msg, HWND hwnd, UINT first, UINT last, UINT flags else peek_message( msg, info.msg.hwnd, info.msg.message, info.msg.message, flags | PM_REMOVE, changed_mask );
continue;
/* give a chance to real message to arrive */HeapFree( GetProcessHeap(), 0, buffer );return FALSE;
Why do you need to leave the function instead of checking for further messages? What is the issue you are seeing?
Alexandre Julliard julliard@winehq.org wrote:
@@ -2931,7 +2931,10 @@ static BOOL peek_message( MSG *msg, HWND hwnd, UINT first, UINT last, UINT flags else peek_message( msg, info.msg.hwnd, info.msg.message, info.msg.message, flags | PM_REMOVE, changed_mask );
continue;
/* give a chance to real message to arrive */HeapFree( GetProcessHeap(), 0, buffer );return FALSE;Why do you need to leave the function instead of checking for further messages? What is the issue you are seeing?
If you mean the only quoted piece of code (MSG_POSTED case) then only reason of the change is to make it consistent with the sent message case.
Dmitry Timoshkov dmitry@baikal.ru writes:
Alexandre Julliard julliard@winehq.org wrote:
@@ -2931,7 +2931,10 @@ static BOOL peek_message( MSG *msg, HWND hwnd, UINT first, UINT last, UINT flags else peek_message( msg, info.msg.hwnd, info.msg.message, info.msg.message, flags | PM_REMOVE, changed_mask );
continue;
/* give a chance to real message to arrive */HeapFree( GetProcessHeap(), 0, buffer );return FALSE;Why do you need to leave the function instead of checking for further messages? What is the issue you are seeing?
If you mean the only quoted piece of code (MSG_POSTED case) then only reason of the change is to make it consistent with the sent message case.
I don't really see any reason to make it consistent, but I'd like to know why you need the other one too.
Alexandre Julliard julliard@winehq.org wrote:
@@ -2931,7 +2931,10 @@ static BOOL peek_message( MSG *msg, HWND hwnd, UINT first, UINT last, UINT flags else peek_message( msg, info.msg.hwnd, info.msg.message, info.msg.message, flags | PM_REMOVE, changed_mask );
continue;
/* give a chance to real message to arrive */HeapFree( GetProcessHeap(), 0, buffer );return FALSE;Why do you need to leave the function instead of checking for further messages? What is the issue you are seeing?
If you mean the only quoted piece of code (MSG_POSTED case) then only reason of the change is to make it consistent with the sent message case.
I don't really see any reason to make it consistent, but I'd like to know why you need the other one too.
The other one is needed to make the test pass for the sent message case. If you have a suggestion how fix it differently please let me know.
Dmitry Timoshkov dmitry@baikal.ru writes:
Alexandre Julliard julliard@winehq.org wrote:
@@ -2931,7 +2931,10 @@ static BOOL peek_message( MSG *msg, HWND hwnd, UINT first, UINT last, UINT flags else peek_message( msg, info.msg.hwnd, info.msg.message, info.msg.message, flags | PM_REMOVE, changed_mask );
continue;
/* give a chance to real message to arrive */HeapFree( GetProcessHeap(), 0, buffer );return FALSE;Why do you need to leave the function instead of checking for further messages? What is the issue you are seeing?
If you mean the only quoted piece of code (MSG_POSTED case) then only reason of the change is to make it consistent with the sent message case.
I don't really see any reason to make it consistent, but I'd like to know why you need the other one too.
The other one is needed to make the test pass for the sent message case. If you have a suggestion how fix it differently please let me know.
I may have one, once I understand what you are trying to fix. What is the actual problem that prompted you to write the tests and the fix?
Alexandre Julliard julliard@winehq.org wrote:
Why do you need to leave the function instead of checking for further messages? What is the issue you are seeing?
If you mean the only quoted piece of code (MSG_POSTED case) then only reason of the change is to make it consistent with the sent message case.
I don't really see any reason to make it consistent, but I'd like to know why you need the other one too.
The other one is needed to make the test pass for the sent message case. If you have a suggestion how fix it differently please let me know.
I may have one, once I understand what you are trying to fix. What is the actual problem that prompted you to write the tests and the fix?
The problem is described in the first test for sending inter-thread messages: https://www.winehq.org/pipermail/wine-patches/2015-April/138750.html My original guess was that the problem is caused by internal message handling in Wine, and the tests I created confirmed that there is a problem in that area. The patch we are discussing fixes the problem discovered by these tests.
Another guess was that it's a race between EndDialog and DestroyWindow leading to a crash. According to my tests under Windows EndDialog called from another thread never returns.
At the moment I'm inclined to another theory about the source of the problem: it's the implementation of SetWindowPos called for another thread window, and the tests+patches I sent today confirm that. Moreover it looks like the real fix for the problem.
There are other discovered problems caused by DestroyWindow from window's thread+SendMessage from another thread, but let's leave them aside for the time being.
Dmitry Timoshkov dmitry@baikal.ru writes:
Alexandre Julliard julliard@winehq.org wrote:
I may have one, once I understand what you are trying to fix. What is the actual problem that prompted you to write the tests and the fix?
The problem is described in the first test for sending inter-thread messages: https://www.winehq.org/pipermail/wine-patches/2015-April/138750.html My original guess was that the problem is caused by internal message handling in Wine, and the tests I created confirmed that there is a problem in that area. The patch we are discussing fixes the problem discovered by these tests.
I still don't understand why internal messages would be different. Is that because you get only one inter-thread message instead of several?
I think it would be better to try to write a test that replicates the problem you are seeing. In particular, testing GetQueueStatus instead of message sequences is not very convincing to me, since it doesn't properly show what's going on.
Alexandre Julliard julliard@winehq.org wrote:
I may have one, once I understand what you are trying to fix. What is the actual problem that prompted you to write the tests and the fix?
The problem is described in the first test for sending inter-thread messages: https://www.winehq.org/pipermail/wine-patches/2015-April/138750.html My original guess was that the problem is caused by internal message handling in Wine, and the tests I created confirmed that there is a problem in that area. The patch we are discussing fixes the problem discovered by these tests.
I still don't understand why internal messages would be different. Is that because you get only one inter-thread message instead of several?
The problem is that once Get/PeekMessage handles an internal message it doesn't yield control to another thread and checks the message queue immediately, this misses a sent message from another thread and instead returns a previously posted one. I admit that this is a pure artificial thing that I stumbled upon, but it reveals a real bug in Wine with internal message handling.
I think it would be better to try to write a test that replicates the problem you are seeing.
I tried to simplify the tests since calling EndDialog+GetMessage+DestroyMessage from different threads may create pretty complicated scenarios, and most obvious (to me) things to test are SendMessage+Get/PeekMessage and SetWindowPos+ Get/PeekMessage from different threads are two cases that should cover things that I think may lead to the observed bug(s).
I can sertainly add more tests, but only when already sent ones are accepted to avoid creating conflicts and resending them again.
In particular, testing GetQueueStatus instead of message sequences is not very convincing to me, since it doesn't properly show what's going on.
I tried to test both as much as possible, did I miss some cases where one of GetQueueStatus or a message sequence is not tested?
Dmitry Timoshkov dmitry@baikal.ru writes:
The problem is that once Get/PeekMessage handles an internal message it doesn't yield control to another thread and checks the message queue immediately, this misses a sent message from another thread and instead returns a previously posted one. I admit that this is a pure artificial thing that I stumbled upon, but it reveals a real bug in Wine with internal message handling.
This sounds like a simple race condition. Slowing down message processing may make it less likely to happen, but it doesn't fix anything.
Alexandre Julliard julliard@winehq.org wrote:
The problem is that once Get/PeekMessage handles an internal message it doesn't yield control to another thread and checks the message queue immediately, this misses a sent message from another thread and instead returns a previously posted one. I admit that this is a pure artificial thing that I stumbled upon, but it reveals a real bug in Wine with internal message handling.
This sounds like a simple race condition. Slowing down message processing may make it less likely to happen, but it doesn't fix anything.
Yes, it's a race, and it's caused by using internal messages for synchronization across the threads. Another way to fix this is by avoiding sending an internal message at all, and probably a test will show that Windows sends WM_ENABLE message from the source thread. Would you like to see such a test?
Dmitry Timoshkov dmitry@baikal.ru writes:
Alexandre Julliard julliard@winehq.org wrote:
The problem is that once Get/PeekMessage handles an internal message it doesn't yield control to another thread and checks the message queue immediately, this misses a sent message from another thread and instead returns a previously posted one. I admit that this is a pure artificial thing that I stumbled upon, but it reveals a real bug in Wine with internal message handling.
This sounds like a simple race condition. Slowing down message processing may make it less likely to happen, but it doesn't fix anything.
Yes, it's a race, and it's caused by using internal messages for synchronization across the threads. Another way to fix this is by avoiding sending an internal message at all, and probably a test will show that Windows sends WM_ENABLE message from the source thread. Would you like to see such a test?
If you can avoid internal messages that's certainly good, but yes, I'd like to see more tests. In particular, for SetWindowPos you'd want to test using separate thread inputs, and how that work with changing the active window etc. Also SWP_ASYNCWINDOWPOS should be tested, from the docs it sounds very much like it needs an internal message.
Alexandre Julliard julliard@winehq.org wrote:
If you can avoid internal messages that's certainly good, but yes, I'd like to see more tests. In particular, for SetWindowPos you'd want to test using separate thread inputs, and how that work with changing the active window etc. Also SWP_ASYNCWINDOWPOS should be tested, from the docs it sounds very much like it needs an internal message.
It would be helpful to clarify status of the already sent patches first, after that I'll see waht could be done next.
Dmitry Timoshkov dmitry@baikal.ru writes:
Alexandre Julliard julliard@winehq.org wrote:
If you can avoid internal messages that's certainly good, but yes, I'd like to see more tests. In particular, for SetWindowPos you'd want to test using separate thread inputs, and how that work with changing the active window etc. Also SWP_ASYNCWINDOWPOS should be tested, from the docs it sounds very much like it needs an internal message.
It would be helpful to clarify status of the already sent patches first, after that I'll see waht could be done next.
I'm hoping you can consolidate this into a single series of tests that focuses on the problematic area.
Alexandre Julliard julliard@winehq.org wrote:
If you can avoid internal messages that's certainly good, but yes, I'd like to see more tests. In particular, for SetWindowPos you'd want to test using separate thread inputs, and how that work with changing the active window etc. Also SWP_ASYNCWINDOWPOS should be tested, from the docs it sounds very much like it needs an internal message.
It would be helpful to clarify status of the already sent patches first, after that I'll see waht could be done next.
I'm hoping you can consolidate this into a single series of tests that focuses on the problematic area.
Already sent tests already do test the cases which I consider as a most likely source of the problem. There is no other theories on my part so far, there is nothing to consolidate.
Dmitry Timoshkov dmitry@baikal.ru writes:
Alexandre Julliard julliard@winehq.org wrote:
If you can avoid internal messages that's certainly good, but yes, I'd like to see more tests. In particular, for SetWindowPos you'd want to test using separate thread inputs, and how that work with changing the active window etc. Also SWP_ASYNCWINDOWPOS should be tested, from the docs it sounds very much like it needs an internal message.
It would be helpful to clarify status of the already sent patches first, after that I'll see waht could be done next.
I'm hoping you can consolidate this into a single series of tests that focuses on the problematic area.
Already sent tests already do test the cases which I consider as a most likely source of the problem. There is no other theories on my part so far, there is nothing to consolidate.
They may be enough to show that there's a problem, but they are not sufficient to demonstrate that your proposed solution is correct. In order to do that, you'll need to test the things I mentioned above.
Alexandre Julliard julliard@winehq.org wrote:
If you can avoid internal messages that's certainly good, but yes, I'd like to see more tests. In particular, for SetWindowPos you'd want to test using separate thread inputs, and how that work with changing the active window etc. Also SWP_ASYNCWINDOWPOS should be tested, from the docs it sounds very much like it needs an internal message.
It would be helpful to clarify status of the already sent patches first, after that I'll see waht could be done next.
I'm hoping you can consolidate this into a single series of tests that focuses on the problematic area.
Already sent tests already do test the cases which I consider as a most likely source of the problem. There is no other theories on my part so far, there is nothing to consolidate.
They may be enough to show that there's a problem, but they are not sufficient to demonstrate that your proposed solution is correct. In order to do that, you'll need to test the things I mentioned above.
I understand that the patch that removes WM_WINE_SETWINDOWPOS message probably needs more tests, it's already in the pending state, I'm asking about the tests that are waiting in the queue. First thing I'm trying to do is understand where exactly the source of the problem is, and the tests I sent exersize the suspected things one by one. There is no point in testing something that clearly doesn't happen with the application I have here (like SWP_ASYNCWINDOWPOS, or separate thread inputs), and spending my time writing tests for that things won't happen until there is a clear understanding what is going on and that the proposed solution needs to cope with unrelated things.
Dmitry Timoshkov dmitry@baikal.ru writes:
I understand that the patch that removes WM_WINE_SETWINDOWPOS message probably needs more tests, it's already in the pending state, I'm asking about the tests that are waiting in the queue. First thing I'm trying to do is understand where exactly the source of the problem is, and the tests I sent exersize the suspected things one by one.
Sure, that's a good thing to do, but it doesn't mean that all these intermediate tests need to end up in the tree, or that they need to be committed before you can make progress. Just take your time to understand that thing properly, then write tests demonstrating the actual problem, and submit them along with the solution.
Alexandre Julliard julliard@winehq.org wrote:
I understand that the patch that removes WM_WINE_SETWINDOWPOS message probably needs more tests, it's already in the pending state, I'm asking about the tests that are waiting in the queue. First thing I'm trying to do is understand where exactly the source of the problem is, and the tests I sent exersize the suspected things one by one.
Sure, that's a good thing to do, but it doesn't mean that all these intermediate tests need to end up in the tree, or that they need to be committed before you can make progress. Just take your time to understand that thing properly, then write tests demonstrating the actual problem, and submit them along with the solution.
That's a usual thing with hard to diagnose problems like race conditions which is the case here: find something suspicious -> write a test -> try a fix -> find that the fix still not enough -> go to start. In the end there might be a fix somewhere in a different place, but in the way there's a good chance that other problems could be found and fixed, that wouldn't happen otherwise. If you believe that the tests by itself prove nothing or don't deserve to be accepted on their own that's highly discouraging. Is that how Wine development works these days? Probably I should add a local hack and be done with it then.
-
Alexandre Julliard -
Dmitry Timoshkov