Skip to content

Commit

Permalink
Add missing lock guards on Terminal access (microsoft#15894)
Browse files Browse the repository at this point in the history
`Terminal` is used concurrently by at least 4 threads. The table
below lists the class members and the threads that access them
to the best of my knowledge. Where:
* UI: UI Thread
* BG: Background worker threads (`winrt::resume_background`)
* RD: Render thread
* VT: VT connection thread

|                                    | UI | BG | RD | VT |
|------------------------------------|----|----|----|----|
| `_pfnWriteInput`                   | x  | x  |    | x  |
| `_pfnWarningBell`                  |    |    |    | x  |
| `_pfnTitleChanged`                 |    |    |    | x  |
| `_pfnCopyToClipboard`              |    |    |    | x  |
| `_pfnScrollPositionChanged`        | x  | x  |    | x  |
| `_pfnCursorPositionChanged`        |    |    |    | x  |
| `_pfnTaskbarProgressChanged`       |    |    |    | x  |
| `_pfnShowWindowChanged`            |    |    |    | x  |
| `_pfnPlayMidiNote`                 |    |    |    | x  |
| `_pfnCompletionsChanged`           |    |    |    | x  |
| `_renderSettings`                  | x  |    | x  | x  |
| `_stateMachine`                    | x  |    |    | x  |
| `_terminalInput`                   | x  |    |    | x  |
| `_title`                           | x  |    | x  | x  |
| `_startingTitle`                   | x  |    | x  |    |
| `_startingTabColor`                | x  |    |    |    |
| `_defaultCursorShape`              | x  |    |    | x  |
| `_systemMode`                      |    | x  | x  | x  |
| `_snapOnInput`                     | x  | x  |    |    |
| `_altGrAliasing`                   | x  |    |    |    |
| `_suppressApplicationTitle`        | x  |    |    | x  |
| `_trimBlockSelection`              | x  |    |    |    |
| `_autoMarkPrompts`                 | x  |    |    |    |
| `_taskbarState`                    | x  |    |    | x  |
| `_taskbarProgress`                 | x  |    |    | x  |
| `_workingDirectory`                | x  |    |    | x  |
| `_fontInfo`                        | x  |    | x  |    |
| `_selection`                       | x  | x  | x  | x  |
| `_blockSelection`                  | x  | x  | x  |    |
| `_wordDelimiters`                  | x  | x  |    |    |
| `_multiClickSelectionMode`         | x  | x  | x  |    |
| `_selectionMode`                   | x  | x  | x  |    |
| `_selectionIsTargetingUrl`         | x  | x  | x  |    |
| `_selectionEndpoint`               | x  | x  | x  |    |
| `_anchorInactiveSelectionEndpoint` | x  | x  | x  |    |
| `_mainBuffer`                      | x  | x  | x  | x  |
| `_altBuffer`                       | x  | x  | x  | x  |
| `_mutableViewport`                 | x  |    | x  | x  |
| `_scrollbackLines`                 | x  |    |    |    |
| `_detectURLs`                      | x  |    |    |    |
| `_altBufferSize`                   | x  | x  | x  | x  |
| `_deferredResize`                  | x  |    |    | x  |
| `_scrollOffset`                    | x  | x  | x  | x  |
| `_patternIntervalTree`             | x  | x  | x  | x  |
| `_lastKeyEventCodes`               | x  |    |    |    |
| `_currentPromptState`              | x  |    |    | x  |

Only 7 members are specific to one thread and don't require locking.
All other members require some for of locking to be safe for use.

To address the issue this changeset adds `LockForReading/LockForWriting`
calls everywhere `_terminal` is accessed in `ControlCore/HwndTerminal`.
Additionally, to ensure these issues don't pop up anymore, it adds to
all `Terminal` functions a debug assertion that the lock is being held.

Finally, because this changeset started off rather modest, it contains
changes that I initially made without being aware about the extent of
the issue. It simplifies the access around `_patternIntervalTree` by
making `_InvalidatePatternTree()` directly use that member.
Furthermore, it simplifies `_terminal->SetCursorOn(!IsCursorOn())` to
`BlinkCursor()`, allowing the code to be shared with `HwndTerminal`.

Ideally `Terminal` should not be that much of a class so that we don't
need such coarse locking. Splitting out selection and rendering state
should allow deduplicating code with conhost and use finer locking.

Closes microsoft#9617

## Validation Steps Performed
I tried to use as many Windows Terminal features as I could and fixed
every occurrence of `_assertLocked()` failures.
  • Loading branch information
lhecker authored Sep 19, 2023
1 parent f494d68 commit 4370da9
Show file tree
Hide file tree
Showing 9 changed files with 436 additions and 262 deletions.
1 change: 1 addition & 0 deletions .github/actions/spelling/allow/apis.txt
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ CYICON
Dacl
dataobject
dcomp
debugbreak
delayimp
DERR
dlldata
Expand Down
Loading

0 comments on commit 4370da9

Please sign in to comment.