Add mutex to keyEvents in TermControlAutomationPeer #14694
Conversation
ghost
added
Area-TerminalControl
| @@ -112,7 +112,7 @@ namespace winrt::Microsoft::Terminal::Control::implementation | |||
| { | |||
| if (const auto keyEventChar{ gsl::narrow_cast<wchar_t>(charCode) }; IsReadable({ &keyEventChar, 1 })) | |||
| { | |||
| _keyEvents.emplace_back(keyEventChar); | |||
| _keyEvents.lock()->emplace_back(keyEventChar); | |||
There was a problem hiding this comment.
@lhecker this is an acceptable use of lock(), right?
There was a problem hiding this comment.
Yeah the guard should get destroyed at ; (= after the insertion finished).
| _keyEvents.pop_front(); | ||
| } | ||
| else | ||
| auto keyEvents = _keyEvents.lock(); |
There was a problem hiding this comment.
you might need to make this auto& or even (C++14) decltype(auto). I think that auto keyEvents will result in a copy of keyEvents (thanks to how auto works).
See how the first one compiles down to escape(9) even though we just set the value to 7 through auto m!
There was a problem hiding this comment.
There was a problem hiding this comment.
Oh, but this returns a proxy object... so really it doesn't matter. You're actually using a guard<deque>, which is fine. Ignore me! You have not been bitten by C++; in fact, I have!
| _keyEvents.pop_front(); | ||
| } | ||
| else | ||
| auto keyEvents = _keyEvents.lock(); |
There was a problem hiding this comment.
ghost
added
the
Needs-Author-Feedback
DHowett
added
the
AutoMerge
|
Hello @DHowett! Because this pull request has the Do note that I've been instructed to only help merge pull requests of this repository that have been opened for at least 8 hours, a condition that will be fulfilled in about 7 hours 43 minutes. No worries though, I will be back when the time is right! 😉 p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (
|
ghost
merged commit Some investigation revealed that `_keyEvents` would get a `NULL_POINTER_READ` error. On the main thread, `RecordKeyEvent()` would be called, which mainly updates `_keyEvents`. On the renderer thread, `NotifyNewOutput()` would be called, which starts by iterating through `_keyEvents` and slowly clearing it out. On occasion, these two threads are modifying `_keyEvents` simultaneously, causing a crash. The fix is to add a mutex on this variable. Closes #14592 (cherry picked from commit 8e04169) Service-Card-Id: 87649541 Service-Version: 1.16
|
🎉 Handy links: |
ghost
mentioned this pull request
|
🎉 Handy links: |
Some investigation revealed that
_keyEventswould get aNULL_POINTER_READerror. On the main thread,RecordKeyEvent()would be called, which mainly updates_keyEvents. On the renderer thread,NotifyNewOutput()would be called, which starts by iterating through_keyEventsand slowly clearing it out. On occasion, these two threads are modifying_keyEventssimultaneously, causing a crash.The fix is to add a mutex on this variable.
Closes #14592