From 6657d2c3e543006aadd16758d5df83c9566b0748 Mon Sep 17 00:00:00 2001
From: Carlos Zamora <carlos.zamora@microsoft.com>
Date: Thu, 30 Sep 2021 15:15:20 -0700
Subject: [PATCH] [deadlock fix] Remove lock for SIUP::GetSelectionRange()
 (#11386)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

## Summary of the Pull Request
The deadlock was caused by `ScreenInfoUiaProviderBase::GetSelection()` calling `TermControlUiaProvider::GetSelectionRange` (both of which attempted to lock the console). This PR removes the lock and initialization check from `TermControlUiaProvider`. It is no longer necessary because the only one that calls it is `SIUPB::GetSelection()`.

Additionally, this adds some code that was useful in debugging this race condition. That should help us figure out any locking issues that may come up in the future.

## References
#11312
Closes #11385

## Validation Steps Performed
✅ Repro steps don't cause hang
---
 src/cascadia/TerminalCore/Terminal.cpp           | 12 ++++++++++++
 src/cascadia/TerminalCore/Terminal.hpp           |  3 +++
 src/cascadia/TerminalCore/terminalrenderdata.cpp |  3 +++
 src/types/TermControlUiaProvider.cpp             |  6 ------
 4 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/src/cascadia/TerminalCore/Terminal.cpp b/src/cascadia/TerminalCore/Terminal.cpp
index aba4d68de..1ecf6158c 100644
--- a/src/cascadia/TerminalCore/Terminal.cpp
+++ b/src/cascadia/TerminalCore/Terminal.cpp
@@ -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
     return std::unique_lock{ _readWriteLock };
+#else
+    auto lock = std::unique_lock{ _readWriteLock };
+    _lastLocker = GetCurrentThreadId();
+    return lock;
+#endif
 }
 
 // Method Description:
@@ -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
diff --git a/src/cascadia/TerminalCore/Terminal.hpp b/src/cascadia/TerminalCore/Terminal.hpp
index 5eb4db473..0e105b1e1 100644
--- a/src/cascadia/TerminalCore/Terminal.hpp
+++ b/src/cascadia/TerminalCore/Terminal.hpp
@@ -269,6 +269,9 @@ private:
     // 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;
diff --git a/src/cascadia/TerminalCore/terminalrenderdata.cpp b/src/cascadia/TerminalCore/terminalrenderdata.cpp
index 1ab2b5af8..fe1fee627 100644
--- a/src/cascadia/TerminalCore/terminalrenderdata.cpp
+++ b/src/cascadia/TerminalCore/terminalrenderdata.cpp
@@ -233,6 +233,9 @@ catch (...)
 void Terminal::LockConsole() noexcept
 {
     _readWriteLock.lock();
+#ifndef NDEBUG
+    _lastLocker = GetCurrentThreadId();
+#endif
 }
 
 // Method Description:
diff --git a/src/types/TermControlUiaProvider.cpp b/src/types/TermControlUiaProvider.cpp
index 3ced07561..edcee5a15 100644
--- a/src/types/TermControlUiaProvider.cpp
+++ b/src/types/TermControlUiaProvider.cpp
@@ -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