Repository: microsoft/terminal (language: cpp)

Issue:
wslsys in alt buffer toggles DISABLE_NEWLINE_AUTO_RETURN
### Windows Terminal version

1.16.2641.0

### Windows build number

10.0.19044.2364

### Other Software

_No response_

### Steps to reproduce

1. Open a WSL bash shell in conhost or Windows Terminal.
2. Switch to the alt buffer with `printf "\e[?1049h"`
3. Test the formfeed behavior with `printf "\e[10CX\b\fY\n"`
4. Run `wslsys`
5. Run the formfeed test again with `printf "\e[10CX\b\fY\n"`


### Expected Behavior

The test case above moves the cursor right 10 columns, outputs an `X`, a `BS` control (moving the cursor back a column), and a `FF` control (moving the cursor down a row), then finally outputs a `Y`.

In both cases the `Y` should be directly below the `X`.

![image](https://user-images.githubusercontent.com/4181424/212958617-a86e3902-56dd-4d17-9a5e-855fb5dea1a0.png)


### Actual Behavior

After `wslsys` has been run, the `DISABLE_NEWLINE_AUTO_RETURN` mode is turned off, so the `FF` control also triggers a carriage return, and the `Y` is output in the first column.

![image](https://user-images.githubusercontent.com/4181424/212957770-2925fe8e-c2f8-48ff-9d52-054a3f16ba4b.png)

The problem is that the auto-return mode is tracked in two separates places. It's one of the modes of the `OutputMode` field in the `SCREEN_INFORMATION` class, but it's also stored in the `_fAutoReturnOnNewline` flag in the global `Settings`. These two places have different scope!

By default `_fAutoReturnOnNewline` is true, and the `DISABLE_NEWLINE_AUTO_RETURN` mode isn't set. But WSL toggles that state on startup, so in WSL the mode is set, and `_fAutoReturnOnNewline` flag is false. When you switch to the alt buffer, though, you get a new `SCREEN_INFORMATION` instance with the defaults again, so the mode is again not set, but the global `_fAutoReturnOnNewline` is still false!

This isn't immediately a problem, because the line feed operations rely on the `_fAutoReturnOnNewline` flag which still has the correct value. But once you run `wslsys`, it makes a call set `SetConsoleMode` which forces the `_fAutoReturnOnNewline` flag to be refreshed. And although it doesn't change the mode, the mode was already wrong at this point, so that incorrect state is now transferred to the `_fAutoReturnOnNewline` flag.

From then, all the line feed controls will trigger a carriage return, which can be problematic for apps that weren't expecting that behavior. One case in particular is `tmux`, which relies on line feed controls to move down without a carriage return when redrawing the screen. If that isn't correct, the entire display gets messed up. This is at least one of the causes of #6987.


Discussion / hints:
_holy hell_

Like, how do you even find this kind of stuff. This is 👌 

So, seems like if we change it so that changing `_fAutoReturnOnNewline` also sets `OutputMode` to match, this should just work?
If we just want a quick fix, we could make the `SCREEN_INFORMATION` class initialise the `OutputMode` based on the state of the `_fAutoReturnOnNewline` flag. We already do this for the `VIRTUAL_TERMINAL_PROCESSING` mode, which also mirrors its state globally (in the `Settings::_dwVirtTermLevel` field).

This isn't really correct though. Because we can still have a situation where one buffer has the mode set, and another has it reset, but the only state we care about internally is the global flag. So at least some of the buffers will be reporting a mode that doesn't match the active behavior.

Ideally we'd just make the `OutputMode` a global field itself, and then drop all of those other global fields that are mirroring some of the flags. However, there may be legacy reasons why we need to retain at least some of the current behavior.

Just so you know how things currently stand, this is my understanding of the various modes:

The `ENABLE_LVB_GRID_WORLDWIDE`, `DISABLE_NEWLINE_AUTO_RETURN` modes are both essentially global, but they are also mirrored in the per-buffer `OutputMode` field. As a result, the reported mode won't necessarily reflect that active state. I don't think there's any reason why these shouldn't be made strictly global - that's really a bug fix.

The `ENABLE_VIRTUAL_TERMINAL_PROCESSING` is mostly treated as a per-buffer mode, but it's also tracked globally, and that global state determines the initial state of any new buffers. I find that confusing, and since this is a relatively new mode, I wouldn't have thought it essential to preserve the current behavior, but some may still prefer that.

The `ENABLE_PROCESSED_OUTPUT` and `ENABLE_WRAP_AT_EOL_OUTPUT` modes are both per-buffer options. From a VT point of view the latter mode should be global (it's equivalent of `DECAWM`). The former doesn't have an exact VT equivalent, but it's similar to `CRM` mode, which would typically be global. For legacy reasons, though, I suspect we might need to preserve both of these as per-buffer options.

In short: The first two should definitely be strictly global modes, the third I think could reasonably be made global, but the last two might need to remain as per-buffer modes for backwards compatibility.
I'd bet there's a wacky chance that there's an app out there that uses different screen buffers (not VT ones) and relies on different modes in each. That just feels like the kind of weird gap that's been abused in the last 40 years.

That being said, if there's anything to do to help mitigate #6987, I think we should try it. 

I think I'm cool with the breakdown you posted above. Let's just leave the last two per-buffer, but initialized by the global state. 
> That just feels like the kind of weird gap that's been abused in the last 40 years.

I think that this has been "officially supported" ever since the `SetConsoleMode` API took a `HANDLE` :smile:

We may want to draw a clear line between what is shared between console buffers allocated via `CreateConsoleScreenBuffer` ("real" console buffers implemented as multiple `SCREEN_INFORMATION` objects) and what is shared between alt buffers (where we used multiple `SCREEN_INFORMATION` objects only out of convenience¹).

¹ _[irony intensifies]_

Fix patch:
diff --git a/src/host/_stream.cpp b/src/host/_stream.cpp
index 0d4af59ee36..35f7f76d0e9 100644
--- a/src/host/_stream.cpp
+++ b/src/host/_stream.cpp
@@ -805,7 +805,7 @@ using Microsoft::Console::VirtualTerminal::StateMachine;
             // move cursor to the next line.
             pwchBuffer++;
 
-            if (gci.IsReturnOnNewlineAutomatic())
+            if (WI_IsFlagClear(screenInfo.OutputMode, DISABLE_NEWLINE_AUTO_RETURN))
             {
                 // Traditionally, we reset the X position to 0 with a newline automatically.
                 // Some things might not want this automatic "ONLCR line discipline" (for example, things that are expecting a *NIX behavior.)
diff --git a/src/host/ft_host/API_ModeTests.cpp b/src/host/ft_host/API_ModeTests.cpp
index 02a1577f870..9487b1773e5 100644
--- a/src/host/ft_host/API_ModeTests.cpp
+++ b/src/host/ft_host/API_ModeTests.cpp
@@ -21,6 +21,7 @@ class ModeTests
 
     TEST_METHOD(TestConsoleModeInputScenario);
     TEST_METHOD(TestConsoleModeScreenBufferScenario);
+    TEST_METHOD(TestConsoleModeAcrossMultipleBuffers);
 
     TEST_METHOD(TestGetConsoleDisplayMode);
 
@@ -94,6 +95,55 @@ void ModeTests::TestConsoleModeScreenBufferScenario()
     VERIFY_ARE_EQUAL(dwOutputMode, (DWORD)0, L"Verify able to set zero output flags");
 }
 
+void ModeTests::TestConsoleModeAcrossMultipleBuffers()
+{
+    auto dwInitialMode = (DWORD)-1;
+    VERIFY_WIN32_BOOL_SUCCEEDED(GetConsoleMode(Common::_hConsole, &dwInitialMode),
+                                L"Get initial output flags");
+
+    Log::Comment(L"Verify initial flags match the expected defaults");
+    VERIFY_IS_TRUE(WI_IsFlagSet(dwInitialMode, ENABLE_PROCESSED_OUTPUT));
+    VERIFY_IS_TRUE(WI_IsFlagSet(dwInitialMode, ENABLE_WRAP_AT_EOL_OUTPUT));
+    VERIFY_IS_TRUE(WI_IsFlagClear(dwInitialMode, DISABLE_NEWLINE_AUTO_RETURN));
+    VERIFY_IS_TRUE(WI_IsFlagClear(dwInitialMode, ENABLE_LVB_GRID_WORLDWIDE));
+
+    // The initial VT flag may vary, dependent on the VirtualTerminalLevel registry entry.
+    const auto defaultVtFlag = WI_IsFlagSet(dwInitialMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING);
+
+    auto dwUpdatedMode = dwInitialMode;
+    WI_ClearFlag(dwUpdatedMode, ENABLE_PROCESSED_OUTPUT);
+    WI_ClearFlag(dwUpdatedMode, ENABLE_WRAP_AT_EOL_OUTPUT);
+    WI_SetFlag(dwUpdatedMode, DISABLE_NEWLINE_AUTO_RETURN);
+    WI_SetFlag(dwUpdatedMode, ENABLE_LVB_GRID_WORLDWIDE);
+    WI_UpdateFlag(dwUpdatedMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING, !defaultVtFlag);
+    VERIFY_WIN32_BOOL_SUCCEEDED(SetConsoleMode(Common::_hConsole, dwUpdatedMode),
+                                L"Update flags to the opposite of their initial values");
+
+    auto hSecondBuffer = CreateConsoleScreenBuffer(GENERIC_READ | GENERIC_WRITE,
+                                                   0 /*dwShareMode*/,
+                                                   nullptr /*lpSecurityAttributes*/,
+                                                   CONSOLE_TEXTMODE_BUFFER,
+                                                   nullptr /*lpReserved*/);
+    VERIFY_ARE_NOT_EQUAL(INVALID_HANDLE_VALUE, hSecondBuffer, L"Create a second screen buffer");
+
+    auto dwSecondBufferMode = (DWORD)-1;
+    VERIFY_WIN32_BOOL_SUCCEEDED(GetConsoleMode(hSecondBuffer, &dwSecondBufferMode),
+                                L"Get initial flags for second buffer");
+
+    VERIFY_ARE_EQUAL(dwInitialMode, dwSecondBufferMode, L"Verify second buffer initialized with defaults");
+
+    VERIFY_WIN32_BOOL_SUCCEEDED(SetConsoleMode(hSecondBuffer, dwSecondBufferMode),
+                                L"Reapply mode to test if it affects the main buffer");
+
+    VERIFY_WIN32_BOOL_SUCCEEDED(CloseHandle(hSecondBuffer), L"Close the second buffer");
+
+    auto dwFinalMode = (DWORD)-1;
+    VERIFY_WIN32_BOOL_SUCCEEDED(GetConsoleMode(Common::_hConsole, &dwFinalMode),
+                                L"Get flags from the main buffer again");
+
+    VERIFY_ARE_EQUAL(dwUpdatedMode, dwFinalMode, L"Verify main buffer flags haven't changed");
+}
+
 void ModeTests::TestGetConsoleDisplayMode()
 {
     DWORD dwMode = 0;
diff --git a/src/host/getset.cpp b/src/host/getset.cpp
index 4a581253343..fa6df67b8d5 100644
--- a/src/host/getset.cpp
+++ b/src/host/getset.cpp
@@ -436,10 +436,6 @@ void ApiRoutines::GetNumberOfConsoleMouseButtonsImpl(ULONG& buttons) noexcept
             screenInfo.GetStateMachine().ResetState();
         }
 
-        gci.SetVirtTermLevel(WI_IsFlagSet(dwNewMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING) ? 1 : 0);
-        gci.SetAutomaticReturnOnNewline(WI_IsFlagSet(screenInfo.OutputMode, DISABLE_NEWLINE_AUTO_RETURN) ? false : true);
-        gci.SetGridRenderingAllowedWorldwide(WI_IsFlagSet(screenInfo.OutputMode, ENABLE_LVB_GRID_WORLDWIDE));
-
         // if we changed rendering modes then redraw the output buffer,
         // but only do this if we're not in conpty mode.
         if (!gci.IsInVtIoMode() &&
diff --git a/src/host/outputStream.cpp b/src/host/outputStream.cpp
index 9f382cab2af..dbe8f16857e 100644
--- a/src/host/outputStream.cpp
+++ b/src/host/outputStream.cpp
@@ -166,8 +166,8 @@ void ConhostInternalGetSet::SetScrollingRegion(const til::inclusive_rect& scroll
 // - true if a line feed also produces a carriage return. false otherwise.
 bool ConhostInternalGetSet::GetLineFeedMode() const
 {
-    const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
-    return gci.IsReturnOnNewlineAutomatic();
+    auto& screenInfo = _io.GetActiveOutputBuffer();
+    return WI_IsFlagClear(screenInfo.OutputMode, DISABLE_NEWLINE_AUTO_RETURN);
 }
 
 // Routine Description:
diff --git a/src/host/renderData.cpp b/src/host/renderData.cpp
index 4bac2b2da76..80fbf9c8eba 100644
--- a/src/host/renderData.cpp
+++ b/src/host/renderData.cpp
@@ -261,26 +261,20 @@ bool RenderData::IsCursorDoubleWidth() const noexcept
 const bool RenderData::IsGridLineDrawingAllowed() noexcept
 {
     const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
-    // If virtual terminal output is set, grid line drawing is a must. It is always allowed.
-    if (WI_IsFlagSet(gci.GetActiveOutputBuffer().OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING))
+    const auto outputMode = gci.GetActiveOutputBuffer().OutputMode;
+    // If virtual terminal output is set, grid line drawing is a must. It is also enabled
+    // if someone explicitly asked for worldwide line drawing.
+    if (WI_IsAnyFlagSet(outputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING | ENABLE_LVB_GRID_WORLDWIDE))
     {
         return true;
     }
     else
     {
-        // If someone explicitly asked for worldwide line drawing, enable it.
-        if (gci.IsGridRenderingAllowedWorldwide())
-        {
-            return true;
-        }
-        else
-        {
-            // Otherwise, for compatibility reasons with legacy applications that used the additional CHAR_INFO bits by accident or for their own purposes,
-            // we must enable grid line drawing only in a DBCS output codepage. (Line drawing historically only worked in DBCS codepages.)
-            // The only known instance of this is Image for Windows by TeraByte, Inc. (TeraByte Unlimited) which used the bits accidentally and for no purpose
-            //   (according to the app developer) in conjunction with the Borland Turbo C cgscrn library.
-            return !!IsAvailableEastAsianCodePage(gci.OutputCP);
-        }
+        // Otherwise, for compatibility reasons with legacy applications that used the additional CHAR_INFO bits by accident or for their own purposes,
+        // we must enable grid line drawing only in a DBCS output codepage. (Line drawing historically only worked in DBCS codepages.)
+        // The only known instance of this is Image for Windows by TeraByte, Inc. (TeraByte Unlimited) which used the bits accidentally and for no purpose
+        //   (according to the app developer) in conjunction with the Borland Turbo C cgscrn library.
+        return !!IsAvailableEastAsianCodePage(gci.OutputCP);
     }
 }
 
diff --git a/src/host/screenInfo.cpp b/src/host/screenInfo.cpp
index c6249b0459d..f3925234937 100644
--- a/src/host/screenInfo.cpp
+++ b/src/host/screenInfo.cpp
@@ -58,10 +58,11 @@ SCREEN_INFORMATION::SCREEN_INFORMATION(
     _desiredFont{ fontInfo },
     _ignoreLegacyEquivalentVTAttributes{ false }
 {
-    // Check if VT mode is enabled. Note that this can be true w/o calling
-    // SetConsoleMode, if VirtualTerminalLevel is set to !=0 in the registry.
+    // Check if VT mode should be enabled by default. This can be true if
+    // VirtualTerminalLevel is set to !=0 in the registry, or when conhost
+    // is started in conpty mode.
     const auto& gci = ServiceLocator::LocateGlobals().getConsoleInformation();
-    if (gci.GetVirtTermLevel() != 0)
+    if (gci.GetDefaultVirtTermLevel() != 0)
     {
         OutputMode |= ENABLE_VIRTUAL_TERMINAL_PROCESSING;
     }
@@ -1913,6 +1914,8 @@ const SCREEN_INFORMATION& SCREEN_INFORMATION::GetMainBuffer() const
         auto altCursorPos = myCursor.GetPosition();
         altCursorPos.y -= GetVirtualViewport().Top();
         altCursor.SetPosition(altCursorPos);
+        // The alt buffer's output mode should match the main buffer.
+        createdBuffer->OutputMode = OutputMode;
 
         s_InsertScreenBuffer(createdBuffer);
 
@@ -2078,6 +2081,9 @@ void SCREEN_INFORMATION::UseMainScreenBuffer()
         mainCursor.SetIsVisible(altCursor.IsVisible());
         mainCursor.SetBlinkingAllowed(altCursor.IsBlinkingAllowed());
 
+        // Copy the alt buffer's output mode back to the main buffer.
+        psiMain->OutputMode = psiAlt->OutputMode;
+
         s_RemoveScreenBuffer(psiAlt); // this will also delete the alt buffer
         // deleting the alt buffer will give the GetSet back to its main
 
diff --git a/src/host/settings.cpp b/src/host/settings.cpp
index 973e3bcce72..a9673658bb6 100644
--- a/src/host/settings.cpp
+++ b/src/host/settings.cpp
@@ -51,8 +51,6 @@ Settings::Settings() :
     _fAllowAltF4Close(true),
     _dwVirtTermLevel(0),
     _fUseWindowSizePixels(false),
-    _fAutoReturnOnNewline(true), // the historic Windows behavior defaults this to on.
-    _fRenderGridWorldwide(false), // historically grid lines were only rendered in DBCS codepages, so this is false by default unless otherwise specified.
     // window size pixels initialized below
     _fInterceptCopyPaste(0),
     _fUseDx(UseDx::Disabled),
@@ -358,11 +356,11 @@ void Settings::Validate()
     FAIL_FAST_IF(!(_dwScreenBufferSize.Y > 0));
 }
 
-DWORD Settings::GetVirtTermLevel() const
+DWORD Settings::GetDefaultVirtTermLevel() const
 {
     return _dwVirtTermLevel;
 }
-void Settings::SetVirtTermLevel(const DWORD dwVirtTermLevel)
+void Settings::SetDefaultVirtTermLevel(const DWORD dwVirtTermLevel)
 {
     _dwVirtTermLevel = dwVirtTermLevel;
 }
@@ -376,33 +374,6 @@ void Settings::SetAltF4CloseAllowed(const bool fAllowAltF4Close)
     _fAllowAltF4Close = fAllowAltF4Close;
 }
 
-bool Settings::IsReturnOnNewlineAutomatic() const
-{
-    return _fAutoReturnOnNewline;
-}
-void Settings::SetAutomaticReturnOnNewline(const bool fAutoReturnOnNewline)
-{
-    _fAutoReturnOnNewline = fAutoReturnOnNewline;
-}
-
-bool Settings::IsGridRenderingAllowedWorldwide() const
-{
-    return _fRenderGridWorldwide;
-}
-void Settings::SetGridRenderingAllowedWorldwide(const bool fGridRenderingAllowed)
-{
-    // Only trigger a notification and update the status if something has changed.
-    if (_fRenderGridWorldwide != fGridRenderingAllowed)
-    {
-        _fRenderGridWorldwide = fGridRenderingAllowed;
-
-        if (ServiceLocator::LocateGlobals().pRender != nullptr)
-        {
-            ServiceLocator::LocateGlobals().pRender->TriggerRedrawAll();
-        }
-    }
-}
-
 bool Settings::GetFilterOnPaste() const
 {
     return _fFilterOnPaste;
diff --git a/src/host/settings.hpp b/src/host/settings.hpp
index bb79bfcf951..03eff335367 100644
--- a/src/host/settings.hpp
+++ b/src/host/settings.hpp
@@ -50,18 +50,12 @@ class Settings
     RenderSettings& GetRenderSettings() noexcept { return _renderSettings; };
     const RenderSettings& GetRenderSettings() const noexcept { return _renderSettings; };
 
-    DWORD GetVirtTermLevel() const;
-    void SetVirtTermLevel(const DWORD dwVirtTermLevel);
+    DWORD GetDefaultVirtTermLevel() const;
+    void SetDefaultVirtTermLevel(const DWORD dwVirtTermLevel);
 
     bool IsAltF4CloseAllowed() const;
     void SetAltF4CloseAllowed(const bool fAllowAltF4Close);
 
-    bool IsReturnOnNewlineAutomatic() const;
-    void SetAutomaticReturnOnNewline(const bool fAutoReturnOnNewline);
-
-    bool IsGridRenderingAllowedWorldwide() const;
-    void SetGridRenderingAllowedWorldwide(const bool fGridRenderingAllowed);
-
     bool GetFilterOnPaste() const;
     void SetFilterOnPaste(const bool fFilterOnPaste);
 
@@ -225,8 +219,6 @@ class Settings
     std::wstring _LaunchFaceName;
     bool _fAllowAltF4Close;
     DWORD _dwVirtTermLevel;
-    bool _fAutoReturnOnNewline;
-    bool _fRenderGridWorldwide;
     UseDx _fUseDx;
     bool _fCopyColor;
 
diff --git a/src/host/srvinit.cpp b/src/host/srvinit.cpp
index 5706718113a..0f97f62cebd 100644
--- a/src/host/srvinit.cpp
+++ b/src/host/srvinit.cpp
@@ -198,7 +198,7 @@ static bool s_IsOnDesktop()
         // We want everyone to be using VT by default anyways, so this is a
         // strong nudge in that direction. If an application _doesn't_ want VT
         // processing, it's free to disable this setting, even in conpty mode.
-        settings.SetVirtTermLevel(1);
+        settings.SetDefaultVirtTermLevel(1);
 
         // GH#9458 - In the case of a DefTerm handoff, the OriginalTitle might
         // be stashed in the lnk. We want to crack that lnk open, so we can get
diff --git a/src/host/telemetry.cpp b/src/host/telemetry.cpp
index ce2848c9f41..ac37440653b 100644
--- a/src/host/telemetry.cpp
+++ b/src/host/telemetry.cpp
@@ -429,7 +429,7 @@ void Telemetry::WriteFinalTraceLog()
                                     TraceLoggingValue(gci.GetScreenBufferSize().width, "ScreenBufferSizeX"),
                                     TraceLoggingValue(gci.GetScreenBufferSize().height, "ScreenBufferSizeY"),
                                     TraceLoggingValue(gci.GetStartupFlags(), "StartupFlags"),
-                                    TraceLoggingValue(gci.GetVirtTermLevel(), "VirtualTerminalLevel"),
+                                    TraceLoggingValue(gci.GetDefaultVirtTermLevel(), "VirtualTerminalLevel"),
                                     TraceLoggingValue(gci.GetWindowSize().width, "WindowSizeX"),
                                     TraceLoggingValue(gci.GetWindowSize().height, "WindowSizeY"),
                                     TraceLoggingValue(gci.GetWindowOrigin().width, "WindowOriginX"),
diff --git a/src/host/ut_host/TextBufferTests.cpp b/src/host/ut_host/TextBufferTests.cpp
index 1d94330e2fc..041526b480d 100644
--- a/src/host/ut_host/TextBufferTests.cpp
+++ b/src/host/ut_host/TextBufferTests.cpp
@@ -1537,7 +1537,6 @@ void TextBufferTests::TestBackspaceStringsAPI()
     const auto& tbi = si.GetTextBuffer();
     const auto& cursor = tbi.GetCursor();
 
-    gci.SetVirtTermLevel(0);
     WI_ClearFlag(si.OutputMode, ENABLE_VIRTUAL_TERMINAL_PROCESSING);
 
     const auto x0 = cursor.GetPosition().x;


Test patch: