Skip to content

[deadlock fix] Remove lock for SIUP::GetSelectionRange() #11386

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
1 commit merged into from
Sep 30, 2021
+18 −6
Merged

[deadlock fix] Remove lock for SIUP::GetSelectionRange() #11386

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 12 additions & 0 deletions src/cascadia/TerminalCore/Terminal.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -849,7 +849,13 @@ WORD Terminal::_TakeVirtualKeyFromLastKeyEvent(const WORD scanCode) noexcept
// will release this lock when it's destructed.
[[nodiscard]] std::unique_lock<til::ticket_lock> Terminal::LockForReading()
{
#ifdef NDEBUG

@miniksa Michael Niksa (miniksa) Sep 30, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've always done ifdef DBG and done it the other way. Leonard Hecker (@lhecker) would know if this is fine.

return std::unique_lock{ _readWriteLock };
#else
auto lock = std::unique_lock{ _readWriteLock };
_lastLocker = GetCurrentThreadId();
return lock;
#endif
}

// Method Description:
Expand All @@ -859,7 +865,13 @@ WORD Terminal::_TakeVirtualKeyFromLastKeyEvent(const WORD scanCode) noexcept
// will release this lock when it's destructed.
[[nodiscard]] std::unique_lock<til::ticket_lock> Terminal::LockForWriting()
{
#ifdef NDEBUG
return std::unique_lock{ _readWriteLock };
#else
auto lock = std::unique_lock{ _readWriteLock };
_lastLocker = GetCurrentThreadId();
return lock;
#endif
}

Viewport Terminal::_GetMutableViewport() const noexcept
Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalCore/Terminal.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -269,6 +269,9 @@ class Microsoft::Terminal::Core::Terminal final :
// But we can abuse the fact that the surrounding members rarely change and are huge
// (std::function is like 64 bytes) to create some natural padding without wasting space.
til::ticket_lock _readWriteLock;
#ifndef NDEBUG
DWORD _lastLocker;
#endif

std::function<void(const int, const int, const int)> _pfnScrollPositionChanged;
std::function<void(const til::color)> _pfnBackgroundColorChanged;
Expand Down
3 changes: 3 additions & 0 deletions src/cascadia/TerminalCore/terminalrenderdata.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -233,6 +233,9 @@ catch (...)
void Terminal::LockConsole() noexcept
{
_readWriteLock.lock();
#ifndef NDEBUG
_lastLocker = GetCurrentThreadId();
#endif
}

// Method Description:
Expand Down
6 changes: 0 additions & 6 deletions src/types/TermControlUiaProvider.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -114,12 +114,6 @@ HRESULT TermControlUiaProvider::GetSelectionRange(_In_ IRawElementProviderSimple
RETURN_HR_IF_NULL(E_INVALIDARG, ppUtr);
*ppUtr = nullptr;

_pData->LockConsole();
auto Unlock = wil::scope_exit([&]() noexcept {
_pData->UnlockConsole();
});
RETURN_HR_IF(E_FAIL, !_pData->IsUiaDataInitialized() || !_pData->IsSelectionActive());

const auto start = _pData->GetSelectionAnchor();

// we need to make end exclusive
Expand Down