The conhost v2 rewrite from a decade ago introduced a race condition:
Previously, we would acquire and hold the global console lock while
servicing
a console API call. If the call cannot be completed a wait task is
enqueued,
while the lock is held. The v2 rewrite then split the project up into a
"server" and "host" component (which remain to this day). The "host"
would
hold the console lock, while the "server" was responsible for enqueueing
wait
tasks _outside of the console lock_. Without any form of
synchronization,
any operations on the waiter list would then of course introduce a race
condition. In conhost this primarily meant keyboard/mouse input, because
that
runs on the separate Win32 window thread. For Windows Terminal it
primarily
meant the VT input thread.
I do not know why this issue is so extremely noticeable specifically
when we
respond to DSC CPR requests, but I'm also not surprised: I suspect that
the
overall performance issues that conhost had for a long time, meant that
most
things it did were slower than allocating the wait task.
Now that both conhost and Windows Terminal became orders of magnitudes
faster
over the last few years, it probably just so happens that the DSC CPR
request
takes almost exactly as many cycles to complete as allocating the wait
task
does, hence perfectly reproducing the race condition.
There's also a slight chance that this is actually a regression from my
ConPTY
rewrite #17510, but I fail to see what that would be. Regardless of
that,
I'm 100% certain though, that this is a bug that has existed in v0.1.
Closes#18117Closes#18800
## Validation Steps Performed
* See repro in #18800. In other words:
* Continuously emit DSC CPR sequences
* ...read the response from stdin
* ...and print the response to stdout
* Doesn't deadlock randomly anymore ✅
* Feature & Unit tests ✅
Roughly 4 years ago we gave Windows Terminal the ability to
differentiate between black/white and the default colors.
One of the victims was PowerShell and most importantly PSReadLine,
which emit SRG 37 & 40 when what they really want is 38 & 48.
We fixed this on our side by adding a shim.
Since the addition of VT passthrough in #17510 we now intentionally
lost the ability to translate VT sequences from one thing to another.
This meant we also lost the ability to do this shim and as such
this PR removes it. Luckily Windows 11 now ships PSReadLine 2.0.0,
which contains a proper fix for this.
Unfortunately, this is not the case for Windows 10, which ships
PSReadLine 2.0.0-beta2. Users affected by this will have to install
a newer version of PSReadLine or use the default black/white theme.
See 1bf4c082b4586dd056a9e67e363b5ab22197b09f
Closes#13037
Subjectively speaking, this commit makes 3 improvements:
* Most importantly, it now would work with arbitrary Unicode text.
(No more `IsGlyphFullWidth` or DBCS handling during reflow.)
* Due to the simpler implementation it hopefully makes review of
future changes and maintenance simpler. (~3x less LOC.)
* It improves perf. by 1-2 orders of magnitude.
(At 120x9001 with a full buffer I get 60ms -> 2ms.)
Unfortunately, I'm not confident that the new code replicates the old
code exactly, because I failed to understand it. During development
I simply tried to match its behavior with what I think reflow should do.
Closes#797Closes#3088Closes#4968Closes#6546Closes#6901Closes#15964
Closes MSFT:19446208
Related to #5800 and #8000
## Validation Steps Performed
* Unit tests ✅
* Feature tests ✅
* Reflow with a scrollback ✅
* Reflowing the cursor cell causes a forced line-wrap ✅
(Even at the end of the buffer. ✅)
* `color 8f` and reflowing retains the background color ✅
* Enter alt buffer, Resize window, Exit alt buffer ✅
This is a rather trivial changeset. Now that these two are present in the
`std` namespace there's no reason for us to continue using the `gsl` ones.
Additionally this ensures future compatibility with other 3rd party libraries.
This is a follow-up of #13025 to make the members of `til::point/size/rect`
uniform and consistent without the use of `unions`. The only file that has
any changes is `src/host/getset.cpp` where an if condition was simplified.
## Validation Steps Performed
* Host unit tests ✅
* Host feature tests ✅
* ControlCore feature tests ✅
Previously this project used a great variety of types to present text buffer
coordinates: `short`, `unsigned short`, `int`, `unsigned int`, `size_t`,
`ptrdiff_t`, `COORD`/`SMALL_RECT` (aka `short`), and more.
This massive commit migrates almost all use of those types over to the
centralized types `til::point`/`size`/`rect`/`inclusive_rect` and their
underlying type `til::CoordType` (aka `int32_t`).
Due to the size of the changeset and statistics I expect it to contain bugs.
The biggest risk I see is that some code potentially, maybe implicitly, expected
arithmetic to be mod 2^16 and that this code now allows it to be mod 2^32.
Any narrowing into `short` later on would then throw exceptions.
## PR Checklist
* [x] Closes#4015
* [x] I work here
* [x] Tests added/passed
## Validation Steps Performed
Casual usage of OpenConsole and Windows Terminal. ✅
#4015 requires sweeping changes in order to allow a migration of our buffer
coordinates from `int16_t` to `int32_t`. This commit reduces the size of
future commits by using type inference wherever possible, dropping the
need to manually adjust types throughout the project later.
As an added bonus this commit standardizes the alignment of cv qualifiers
to be always left of the type (e.g. `const T&` instead of `T const&`).
The migration to type inference with `auto` was mostly done
using JetBrains Resharper with some manual intervention and the
standardization of cv qualifier alignment using clang-format 14.
## References
This is preparation work for #4015.
## Validation Steps Performed
* Tests pass ✅
This is an attempt to simplify the `ConGetSet` interface down to the
smallest set of methods necessary to support the `AdaptDispatch` class.
The idea is that it should then be easier to implement that interface in
Windows Terminal, so we can replace the `TerminalDispatch` class with
`AdaptDispatch`.
This is a continuation of the refactoring started in #12247, and a
significant step towards #3849.
## Detailed Description of the Pull Request / Additional comments
The general idea was to give the `AdaptDispatch` class direct access to
the high-level structures on which it needs to operate. Some of these
structures are now passed in when the class is constructed (the
`Renderer`, `RenderSettings`, and `TerminalInput`), and some are exposed
as new methods in `ConGetSet` (`GetStateMachine`, `GetTextBuffer`, and
`GetViewport`).
Many of the existing `ConhostInternalGetSet` methods could easily then
be reimplemented in `AdaptDispatch`, since they were often simply
forwarding to methods in one of the above structures. Some were a little
more complicated, though, and require further explanation.
* `GetConsoleScreenBufferInfoEx`: What we were typically using this for
was to obtain the viewport, although what we really wanted was the
virtual viewport, which is now accessible via the `GetViewport`
method. This was also used to obtain the cursor position and buffer
width, which we can now get via the `GetTextBuffer` method.
* `SetConsoleScreenBufferInfoEx`: This was only really used for the
`AdaptDispatch::SetColumns` implementation (for `DECCOLM` mode), and
that could be replaced with `ResizeWindow`. This is a slight change in
behaviour (it sizes the window rather than the buffer), but neither is
technically correct for `DECCOLM`, so I think it's good enough for
now, and at least it's consistent with the other VT sizing operations.
* `SetCursorPosition`: This could mostly be replaced with direct
manipulation of the `Cursor` object (accessible via the text buffer),
although again this is a slight change in behavior. The original code
would also have made a call to `ConsoleImeResizeCompStrView` (which I
don't think is applicable to VT movement), and would potentially have
moved the viewport (not essential for now, but could later be
supported by `DECHCCM`). It also called `VtIo::SetCursorPosition` to
handle cursor inheritance, but that should only apply to
`InteractDispatch`, so I've moved that to the
`InteractDispatch::MoveCursor` method.
* `ScrollRegion`: This has been replaced by two simple helper methods in
`AdaptDispatch` which better meet the VT requirements -
`_ScrollRectVertically` and `_ScrollRectHorizontally`. Unlike the
original `ScrollRegion` implementation, these don't generate
`EVENT_CONSOLE_UPDATE_SCROLL` events (see #12656 for more details).
* `FillRegion`: This has been replaced by the `_FillRect` helper method
in `AdaptDispatch`. It differs from the original `FillRegion` in that
it takes a rect rather than a start position and length, which gives
us more flexibility for future operations.
* `ReverseLineFeed`: This has been replaced with a somewhat refactored
reimplementation in `AdaptDispatch`, mostly using the
`_ScrollRectVertically` helper described above.
* `EraseAll`: This was previously handled by
`SCREEN_INFORMATION::VtEraseAll`, but has now been entirely
reimplemented in the `AdaptDispatch::_EraseAll` method.
* `DeleteLines`/`InsertLines`/`_modifyLines`: These have been replaced
by the `_InsertDeleteLineHelper` method in `AdaptDispatch`, which
mostly relies on the `_ScrollRectVertically` helper described above.
Finally there were a few methods that weren't actually needed in the
`ConGetSet` interface:
* `MoveToBottom`: This was really just a hack to get the virtual
viewport from `GetConsoleScreenBufferInfoEx`. We may still want
something like in the future (e.g. to support `DECVCCM` or #8879), but
I don't think it's essential for now.
* `SuppressResizeRepaint`: This was only needed in `InteractDispatch`
and `PtySignalInputThread`, and they could easily access the `VtIo`
object to implement it themselves.
* `ClearBuffer`: This was only used in `PtySignalInputThread`, and that
could easily access the buffer directly via the global console
information.
* `WriteControlInput`: This was only used in `InteractDispatch`, and
that could easily be replaced with a direct call to
`HandleGenericKeyEvent`.
As part of these changes, I've also refactored some of the existing
`AdaptDispatch` code:
* `_InsertDeleteHelper` (renamed `_InsertDeleteCharacterHelper`) is now
just a straightforward call to the new `_ScrollRectHorizontally`
helper.
* `EraseInDisplay` and `EraseInLine` have been implemented as a series
of `_FillRect` calls, so `_EraseSingleLineHelper` is no longer
required.
* `_EraseScrollback` is a essentially a special form of scrolling
operation, which mostly depends on the `TextBuffer::ScrollRows`
method, and with the filling now provided by the new `_FillRect`
helper.
* There are quite a few operations now in `AdaptDispatch` that are
affected by the scrolling margins, so I've pulled out the common
margin setup into a new `_GetVerticalMargins` helper method. This also
fixes some edge cases where margins could end up out of range.
## Validation Steps Performed
There were a number of unit tests that needed to be updated to work
around functions that have now been removed, but these substitutions
were fairly straightforward for the most part.
The adapter tests were a different story, though. In that case we were
explicitly testing how operations were passed through to the `ConGetSet`
interface, but with more than half those methods now gone, a significant
rewrite was required.
I've tried to retain the crux of the original tests, but we now have to
validate the state changes on the underlying data structures, where
before that state would have been tracked in the `TestGetSet` mock. And
in some cases we were testing what happened when a method failed, but
since that scenario is no longer possible, I've simply removed those
tests.
I've also tried to manually test all the affected operations to confirm
that they're still working as expected, both in vttest as well as my own
test scripts.
Closes#12662
`WideCharToMultiByte` doesn't write a final null-byte by default.
`til::u16u8` avoids the problem.
## PR Checklist
* [x] I work here
* [x] Tests added/passed
## Validation Steps Performed
* Test passes in Debug builds ✅
[Git2Git] Merged PR 6303114: Prepare command history before COOKED_READ in tests
PR !6278637 introduced a dependency from COOKED_READ_DATA on the ability
to locate a command history for a process handle (here, `nullptr`).
The tests were blowing up because no such history had been allocated.
Closes MSFT-34812916
Closes MSFT-34813774
Closes MSFT-34815941
Closes MSFT-34817558
Closes MSFT-34817540 (Watson)
Retrieved from https://microsoft.visualstudio.com os.2020 OS official/rs_wdx_dxp_windev f7517e686447fc0469f6b83df19760dc3dafd577
When you use the size parameter to WideCharToMultiByte, it only
null-terminates the output string if the input string was
null-terminated within the specified range.
Burned in for 1k runs-
BEFORE
Summary: Total=1000, Passed=997, Failed=3
AFTER
Summary: Total=1000, Passed=1000, Failed=0
Fixes MSFT-34656993
## References
* See also #8617
## PR Checklist
* [x] Supports #3075
* [x] I work here.
* [x] Manual test.
## Detailed Description of the Pull Request / Additional comments
### Window Title Generation
Every time the renderer checks the title, it's doing two bad things that
I've fixed:
1. It's assembling the prefix to the full title doing a concatenation.
No one ever gets just the prefix ever after it is set besides the
concat. So instead of storing prefix and the title, I store the
assembled prefix + title and the bare title.
2. A copy must be made because it was returning `std::wstring` instead
of `std::wstring&`. Now it returns the ref.
### Dirty Area Return
Every time the renderer checks the dirty area, which is sometimes
multiple times per pass (regular text printing, again for selection,
etc.), a vector is created off the heap to return the rectangles. The
consumers only ever iterate this data. Now we return a span over a
rectangle or rectangles that the engine must store itself.
1. For some renderers, it's always a constant 1 element. They update
that 1 element when dirty is queried and return it in the span with a
span size of 1.
2. For other renderers with more complex behavior, they're already
holding a cached vector of rectangles. Now it's effectively giving
out the ref to those in the span for iteration.
### Bitmap Runs
The `til::bitmap` used a `std::optional<std::vector<til::rectangle>>`
inside itself to cache its runs and would clear the optional when the
runs became invalidated. Unfortunately doing `.reset()` to clear the
optional will destroy the underlying vector and have it release its
memory. We know it's about to get reallocated again, so we're just going
to make it a `std::pmr::vector` and give it a memory pool.
The alternative solution here was to use a `bool` and
`std::vector<til::rectangle>` and just flag when the vector was invalid,
but that was honestly more code changes and I love excuses to try out
PMR now.
Also, instead of returning the ref to the vector... I'm just returning a
span now. Everyone just iterates it anyway, may as well not share the
implementation detail.
### UTF-8 conversions
When testing with Terminal and looking at the `conhost.exe`'s PTY
renderer, it spends a TON of allocation time on converting all the
UTF-16 stuff inside to UTF-8 before it sends it out the PTY. This was
because `ConvertToA` was allocating a string inside itself and returning
it just to have it freed after printing and looping back around again...
as a PTY does.
The change here is to use `til::u16u8` that accepts a buffer out
parameter so the caller can just hold onto it.
## Validation Steps Performed
- [x] `big.txt` in conhost.exe (GDI renderer)
- [x] `big.txt` in Terminal (DX, PTY renderer)
- [x] Ensure WDDM and BGFX build under Razzle with this change.
Many include statements use forward slashes, while others use backwards
slashes. This is inconsistent formatting. For this reason, I changed the
backward slashes to forward slashes since that is the standard.
There is going to be a very long tail of applications that will
explicitly request VT SGR 40/37 when what they really want is to
SetConsoleTextAttribute() with a black background/white foreground.
Instead of making those applications look bad (and therefore making us
look bad, because we're releasing this as an update to something that
"looks good" already), we're introducing this compatibility quirk.
Before the color reckoning in #6698 + #6506, *every* color was subject
to being spontaneously and erroneously turned into the default color.
Now, only the 16-color palette value that matches the active console
background/foreground color will be destroyed, and only when received
from specific applications.
Removal will be tracked by #6807.
Michael and I discussed what layer this quirk really belonged in. I
originally believed it would be sufficient to detect a background color
that matched the legacy default background, but @j4james provided an
example of where that wouldn't work out (powershell setting the
foreground color to white/gray). In addition, it was too heavyhanded: it
re-broke black backgrounds for every application.
Michael thought that it should live in the server, as a small VT parser
that righted the wrongs coming directly out of the application. On
further investigation, however, I realized that we'd need to push more
information up into the server (so that it could make the decision about
which VT was wrong and which was right) than should be strictly
necessary.
The host knows which colors are right and wrong, and it gets final say
in what ends up in the buffer.
Because of that, I chose to push the quirk state down through
WriteConsole to DoWriteConsole and toggle state on the
SCREEN_INFORMATION that indicates whether the colors coming out of the
application are to be distrusted. This quirk _only applies to pwsh.exe
and powershell.exe._
NOTE: This doesn't work for PowerShell the .NET Global tool, because it
is run as an assembly through dotnet.exe. I have no opinion on how to
fix this, or whether it is worth fixing.
VALIDATION
----------
I configured my terminals to have an incredibly garish color scheme to
show exactly what's going to happen as a result of this. The _default
terminal background_ is purple or red, and the foreground green. I've
printed out a heap of test colors to see how black interacts with them.
Pull request #6810 contains the images generated from this test.
The only color lines that change are the ones where black as a
background or white as a foreground is selected out of the 16-color
palette explicitly. Reverse video still works fine (because black is in
the foreground!), and it's even possible to represent "black on default"
and reverse it into "default on black", despite the black in question
having been `40`.
Fixes#6767.
The `DECSTBM` margins are meant to define the range of lines within which
certain vertical scrolling operations take place. However, we were applying
these margin restrictions in the `ScrollRegion` function, which is also used in
a number of places that shouldn't be affected by `DECSTBM`.
This includes the `ICH` and `DCH` escape sequences (which are only affected by
the horizontal margins, which we don't yet support), the
`ScrollConsoleScreenBuffer` API (which is public Console API, not meant to be
affected by the VT terminal emulation), and the `CSI 3 J` erase scrollback
extension (which isn't really scrolling as such, but uses the `ScrollRegion`
function to manipulate the scrollback buffer).
This commit moves the margin clipping out of the `ScrollRegion` function, so it
can be applied exclusively in the places that need it.
With the margin clipping removed from the `ScrollRegion` function, it now had
to be applied manually in the places it was actually required. This included:
* The `DoSrvPrivateReverseLineFeed` function (for the `RI` control): This was
* just a matter of updating the bottom of the scroll rect to the bottom margin
* (at least when the margins were actually set), since the top of the scroll
* rect would always be the top of the viewport. The
* `DoSrvPrivateModifyLinesImpl` function (for the `IL` and `DL` commands):
* Again this was just a matter of updating the bottom of the scroll rect, since
* the cursor position would always determine the top of the scroll rect. The
* `AdaptDispatch::_ScrollMovement` method (for the `SU` and `SD` commands):
* This required updating both the top and bottom coordinates of the scroll
* rect, and also a simpler destination Y coordinate (the way the `ScrollRegion`
* function worked before, the caller was expected to take the margins into
* account when determining the destination).
On the plus side, there was now no longer a need to override the margins when
calling `ScrollRegion` in the `AdjustCursorPosition` function. In the first
case, the margins had needed to be cleared (_stream.cpp 143-145), but that is
now the default behaviour. In the second case, there had been a more
complicated adjustment of the margins (_stream.cpp 196-209), but that code was
never actually used so could be removed completely (to get to that point either
_fScrollUp_ was true, so _scrollDownAtTop_ couldn't also be true, or
_fScrollDown_ was true, but in that case there is a check to make sure
_scrollDownAtTop_ is false).
While testing, I also noticed that one of the `ScrollRegion` calls in the
`AdjustCursorPosition` function was not setting the horizontal range correctly
- the scrolling should always affect the full buffer width rather than just the
viewport width - so I've fixed that now as well.
## Validation Steps Performed
For commands like `RI`, `IL`, `DL`, etc. where we've changed the implementation
but not the behaviour, there were already unit tests that could confirm that
the new implementation was still producing the correct results.
Where there has been a change in behaviour - namely for the `ICH` and `DCH`
commands, and the `ScrollConsoleScreenBuffer` API - I've extended the existing
unit tests to check that they still function correctly even when the `DECSTBM`
margins are set (which would previously have caused them to fail).
I've also tested manually with the test cases in issues #2543 and #2659, and
confirmed that they now work as expected.
Closes#2543Closes#2659
* Removed using namespace directive from header files and put these in cpp files where they are used
* Fixed tabbing issues by replacing them with spaces.
Also regrouped the using directives.
* Update src/host/exemain.cpp
Co-Authored-By: Mike Griese <migrie@microsoft.com>
* Update src/interactivity/win32/find.cpp
Co-Authored-By: Mike Griese <migrie@microsoft.com>
