Use a mutex to lock Win32 message handling #5304
Conversation
cpp/iedriver/IECommandExecutor.cpp
Outdated
| @@ -438,6 +439,8 @@ unsigned int WINAPI IECommandExecutor::ThreadProc(LPVOID lpParameter) { | |||
| LOG(DEBUG) << "Returned from DestroyWindow()"; | |||
| break; | |||
| } else { | |||
| static std::mutex messageLock; | |||
| std::lock_guard<std::mutex> lock(messageLock); | |||
There was a problem hiding this comment.
I'm not a Win32 hacker, so I'm not sure how this fixes the stability issue. The docs for GetMessage, TranslateMessage, and DispatchMessage suggest they only operate on the current thread. Is this something to do with multiple threads attempting to post messages to the same HWND?
There was a problem hiding this comment.
It appears to me that this code is indeed called from multiple threads. In a typical win32 program these go on what I call the "UI thread" but that's not the context they're being used in here. Perhaps I'm wrong and this is only called in one thread, in which case this PR is useless, but I believe there are multiple threads using this.
There was a problem hiding this comment.
Huh, so although the docs suggest that these methods are thread safe, they're not really because the normal use case is that they're only used on the UI thread? Interesting.
Your observed increase in stability is a Good Thing, and I'm happy to trust your reasoning :)
cpp/iedriver/IECommandExecutor.cpp
Outdated
| @@ -438,6 +439,8 @@ unsigned int WINAPI IECommandExecutor::ThreadProc(LPVOID lpParameter) { | |||
| LOG(DEBUG) << "Returned from DestroyWindow()"; | |||
| break; | |||
| } else { | |||
| static std::mutex messageLock; | |||
There was a problem hiding this comment.
Can we have a comment explaining why the mutex is necessary?
There was a problem hiding this comment.
I'll keep on eye on this PR for that :) Thank you!
|
Anything that improves stability is a Good Thing. Thank you for putting this PR up again. |
|
@Xaeroxe, I'm just waiting for that comment before I merge this --- I'm not sure I understand how the patch fixes the problem, and I'd hate to put something misleading into the tree :) If you reply to the comment above, I can add it, or another diff would work too. Looking forward to this landing! |
|
Comment added, sorry for the delay. Let me know if you feel the explanation is satisfactory. |
|
@Xaeroxe Thank you! I've merged the PR and it'll be in the next release. |
Xin the preceding checkbox, I verify that I have signed the Contributor License AgreementI use Selenium at work and for our test bed we were having many problems with the IEDriverServer crashing due to a Memory Access Violation. It got to the point where it was impeding our work, so I cloned the project and ran it in debug mode to see if I could identify the problem.
Our test bed was causing the driver to crash specifically on line 441 of IECommandExecutor.cpp, the inner message handling loop for Win32. I believe the crash was occurring due to concurrent use of the Win32 APIs by multiple threads, which do not appear to be thread safe. So I introduced a static mutex shared between threads and had each thread acquire a lock to this mutex before processing messages. This has worked wonderfully for us, our tests are now much more stable. I'd like to contribute this fix back to Selenium.
I believe this may have also been the cause for #1973 and similar issues.