wip

This change prevents `throttled_func` from reading uninitialized memory
under some yet-unkown circumstances. The tl;dr is:
This simply moves the callback invocation into the storage.
That way we can centrally avoid invoking the callback accidentally.

CONTRIBUTING.md

@@ -158,7 +158,7 @@ Once you've discussed your proposed feature/fix/etc. with a team member, and you
 
 ### Testing
 
-Testing is a key component in the development workflow. Both Windows Terminal and Windows Console use TAEF(the Test Authoring and Execution Framework) as the main framework for testing.
+Testing is a key component in the development workflow. Both Windows Terminal and Windows Console use TAEF (the Test Authoring and Execution Framework) as the main framework for testing.
 
 If your changes affect existing test cases, or you're working on brand new features and also the accompanying test cases, see [TAEF](./doc/TAEF.md) for more information about how to validate your work locally.
 

OpenConsole.sln

@@ -2304,10 +2304,14 @@ Global
 		{6515F03F-E56D-4DB4-B23D-AC4FB80DB36F}.Release|x64.Build.0 = Release|x64
 		{6515F03F-E56D-4DB4-B23D-AC4FB80DB36F}.Release|x86.ActiveCfg = Release|Win32
 		{6515F03F-E56D-4DB4-B23D-AC4FB80DB36F}.Release|x86.Build.0 = Release|Win32
-        {7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|Any CPU.ActiveCfg = Debug|Win32
-        {7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|ARM64.ActiveCfg = Release|ARM64
-        {7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x64.ActiveCfg = Release|x64
-        {7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x86.ActiveCfg = Release|Win32
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|Any CPU.ActiveCfg = AuditMode|x64
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|Any CPU.Build.0 = AuditMode|x64
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|ARM64.ActiveCfg = AuditMode|ARM64
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|ARM64.Build.0 = AuditMode|ARM64
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x64.ActiveCfg = AuditMode|x64
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x64.Build.0 = AuditMode|x64
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x86.ActiveCfg = AuditMode|Win32
+		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.AuditMode|x86.Build.0 = AuditMode|Win32
 		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.Debug|Any CPU.ActiveCfg = Debug|x64
 		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.Debug|ARM64.ActiveCfg = Debug|ARM64
 		{7615F03F-E56D-4DB4-B23D-BD4FB80DB36F}.Debug|ARM64.Build.0 = Debug|ARM64

src/cascadia/TerminalControl/TermControl.cpp

@@ -66,6 +66,8 @@ namespace winrt::Microsoft::Terminal::Control::implementation
             /* Cygwin */ L"/cygdrive/",
             /* MSYS2 */ L"/",
         };
+        static constexpr wil::zwstring_view sSingleQuoteEscape = L"'\\''";
+        static constexpr auto cchSingleQuoteEscape = sSingleQuoteEscape.size();
 
         if (translationStyle == PathTranslationStyle::None)
         {
@@ -75,6 +77,16 @@ namespace winrt::Microsoft::Terminal::Control::implementation
         // All of the other path translation modes current result in /-delimited paths
         std::replace(fullPath.begin(), fullPath.end(), L'\\', L'/');
 
+        // Escape single quotes, assuming translated paths are always quoted by a pair of single quotes.
+        size_t pos = 0;
+        while ((pos = fullPath.find(L'\'', pos)) != std::wstring::npos)
+        {
+            // ' -> '\'' (for POSIX shell)
+            fullPath.replace(pos, 1, sSingleQuoteEscape);
+            // Arithmetic overflow cannot occur here.
+            pos += cchSingleQuoteEscape;
+        }
+
         if (fullPath.size() >= 2 && fullPath.at(1) == L':')
         {
             // C:/foo/bar -> Cc/foo/bar

src/cascadia/UIMarkdown/CodeBlock.h

@@ -30,7 +30,7 @@ namespace winrt::Microsoft::Terminal::UI::Markdown::implementation
 
     struct RequestRunCommandsArgs : RequestRunCommandsArgsT<RequestRunCommandsArgs>
     {
-        RequestRunCommandsArgs(const winrt::hstring& commandlines) :
+        RequestRunCommandsArgs(const winrt::hstring& commandlines) noexcept :
             Commandlines{ commandlines } {};
 
         til::property<winrt::hstring> Commandlines;

src/cascadia/UIMarkdown/MarkdownToXaml.cpp

@@ -92,7 +92,7 @@ WUX::Documents::Paragraph MarkdownToXaml::_CurrentParagraph()
             {
                 _lastParagraph.TextIndent(-WidthOfBulletPoint);
             }
-            _lastParagraph.Margin(WUX::ThicknessHelper::FromLengths(IndentWidth * _indent, 0, 0, 0));
+            _lastParagraph.Margin(WUX::ThicknessHelper::FromLengths(static_cast<double>(IndentWidth) * _indent, 0, 0, 0));
         }
         _root.Blocks().Append(_lastParagraph);
     }
@@ -137,16 +137,16 @@ WUX::Documents::Run MarkdownToXaml::_NewRun()
     }
     return _lastRun;
 }
-void MarkdownToXaml::_EndRun()
+void MarkdownToXaml::_EndRun() noexcept
 {
     _lastRun = nullptr;
 }
-void MarkdownToXaml::_EndSpan()
+void MarkdownToXaml::_EndSpan() noexcept
 {
     _EndRun();
     _lastSpan = nullptr;
 }
-void MarkdownToXaml::_EndParagraph()
+void MarkdownToXaml::_EndParagraph() noexcept
 {
     _EndSpan();
     _lastParagraph = nullptr;
@@ -162,7 +162,7 @@ WUX::Controls::TextBlock MarkdownToXaml::_makeDefaultTextBlock()
 
 void MarkdownToXaml::_RenderNode(cmark_node* node, cmark_event_type ev_type)
 {
-    bool entering = (ev_type == CMARK_EVENT_ENTER);
+    const bool entering = (ev_type == CMARK_EVENT_ENTER);
 
     switch (cmark_node_get_type(node))
     {
@@ -221,7 +221,7 @@ void MarkdownToXaml::_RenderNode(cmark_node* node, cmark_event_type ev_type)
         if (entering)
         {
             _EndParagraph();
-            _NewRun().Text(bullets[std::clamp(_indent - _blockQuoteDepth - 1, 0, 2)]);
+            _NewRun().Text(gsl::at(bullets, std::clamp(_indent - _blockQuoteDepth - 1, 0, 2)));
         }
         break;
 
@@ -251,7 +251,7 @@ void MarkdownToXaml::_RenderNode(cmark_node* node, cmark_event_type ev_type)
         const auto codeHstring{ winrt::to_hstring(cmark_node_get_literal(node)) };
         // The literal for a code node always includes the trailing newline.
         // Trim that off.
-        std::wstring_view codeView{ codeHstring.c_str(), codeHstring.size() - 1 };
+        const std::wstring_view codeView{ codeHstring.c_str(), codeHstring.size() - 1 };
 
         auto codeBlock = winrt::make<winrt::Microsoft::Terminal::UI::Markdown::implementation::CodeBlock>(winrt::hstring{ codeView });
         WUX::Documents::InlineUIContainer codeContainer{};
@@ -282,7 +282,7 @@ void MarkdownToXaml::_RenderNode(cmark_node* node, cmark_event_type ev_type)
         cmark_node* parent = cmark_node_parent(node);
         cmark_node* grandparent = cmark_node_parent(parent);
 
-        if (grandparent != NULL && cmark_node_get_type(grandparent))
+        if (grandparent != nullptr && cmark_node_get_type(grandparent))
         {
             tight = cmark_node_get_list_tight(grandparent);
         }

src/cascadia/UIMarkdown/MarkdownToXaml.h

@@ -27,9 +27,9 @@ private:
     winrt::Windows::UI::Xaml::Documents::Run _CurrentRun();
     winrt::Windows::UI::Xaml::Documents::Span _CurrentSpan();
     winrt::Windows::UI::Xaml::Documents::Run _NewRun();
-    void _EndRun();
-    void _EndSpan();
-    void _EndParagraph();
+    void _EndRun() noexcept;
+    void _EndSpan() noexcept;
+    void _EndParagraph() noexcept;
 
     winrt::Windows::UI::Xaml::Controls::TextBlock _makeDefaultTextBlock();
 

src/cascadia/WinRTUtils/inc/ThrottledFunc.h

@@ -113,7 +113,7 @@ private:
                 {
                     try
                     {
-                        std::apply(self->_func, self->_storage.take());
+                        self->_storage.apply(self->_func);
                     }
                     CATCH_LOG();
                 }

src/cascadia/inc/cppwinrt_utils.h

@@ -208,7 +208,7 @@ public:
                                                                                           \
 protected:                                                                                \
     type _##name{ __VA_ARGS__ };                                                          \
-    void _set##name(const type& value)                                                    \
+    void _set##name(const type& value) noexcept(noexcept(_##name = value))                \
     {                                                                                     \
         _##name = value;                                                                  \
     };

src/common.build.pre.props

@@ -275,6 +275,9 @@
     <VcpkgOSTarget>windows</VcpkgOSTarget>
     <VcpkgUseStatic>true</VcpkgUseStatic>
 
+    <!-- BODGY - work around DD#2302065. vcpkg now validates target triplets, and is *case sensitive* -->
+    <VcpkgPlatformTarget Condition="'$(Platform)'=='ARM64'">arm64</VcpkgPlatformTarget>
+
     <VcpkgAdditionalInstallOptions>--x-feature=terminal</VcpkgAdditionalInstallOptions>
     <!--
       Since we link everything statically, we don't need to copy anything.

src/host/input.cpp

@@ -321,28 +321,35 @@ void ProcessCtrlEvents()
         return;
     }
 
-    auto Status = STATUS_SUCCESS;
+    const auto ctrl = ServiceLocator::LocateConsoleControl();
+
     for (const auto& r : termRecords)
     {
-        /*
-         * Status will be non-successful if a process attached to this console
-         * vetoes shutdown. In that case, we don't want to try to kill any more
-         * processes, but we do need to make sure we continue looping so we
-         * can close any remaining process handles. The exception is if the
-         * process is inaccessible, such that we can't even open a handle for
-         * query. In this case, use best effort to send the close event but
-         * ignore any errors.
-         */
-        if (SUCCEEDED_NTSTATUS(Status))
-        {
-            Status = ServiceLocator::LocateConsoleControl()
-                         ->EndTask(r.dwProcessID,
-                                   EventType,
-                                   CtrlFlags);
-            if (!r.hProcess)
-            {
-                Status = STATUS_SUCCESS;
-            }
-        }
+        // Older versions of Windows would do various things if the EndTask() call failed:
+        // * XP: Pops up a "Windows can't end this program" dialog for every already-dead process.
+        // * Vista - Win 7: Simply skips over already-dead processes.
+        // * Win 8 - Win 11 26100: Aborts on an already-dead process (you have to WM_CLOSE conhost multiple times).
+        //
+        // That last period had the following comment:
+        //   Status will be non-successful if a process attached to this console
+        //   vetoes shutdown. In that case, we don't want to try to kill any more
+        //   processes, but we do need to make sure we continue looping so we
+        //   can close any remaining process handles. The exception is if the
+        //   process is inaccessible, such that we can't even open a handle for
+        //   query. In this case, use best effort to send the close event but
+        //   ignore any errors.
+        //
+        // The corresponding logic worked like this:
+        //   if (FAILED(EndTask(...)) && r.hProcess) {
+        //       break;
+        //   }
+        //
+        // That logic was removed around the Windows 11 26100 time frame, because CSRSS
+        // (which handles EndTask) now waits 5s and then force-kills the process for us.
+        // Going back to the Win 7 behavior then should make shutdown a lot more robust.
+        // The bad news is that EndTask() returns STATUS_UNSUCCESSFUL no matter whether
+        // the process was already dead, or if the request actually failed for some reason.
+        // Hopefully there aren't any regressions, but we can't know without trying.
+        LOG_IF_NTSTATUS_FAILED(ctrl->EndTask(r.dwProcessID, EventType, CtrlFlags));
     }
 }

src/host/settings.cpp

@@ -36,7 +36,7 @@ Settings::Settings() :
     _bAutoPosition(true),
     _uHistoryBufferSize(DEFAULT_NUMBER_OF_COMMANDS),
     _uNumberOfHistoryBuffers(DEFAULT_NUMBER_OF_BUFFERS),
-    _bHistoryNoDup(true),
+    _bHistoryNoDup(false),
     // ColorTable initialized below
     _uCodePage(ServiceLocator::LocateGlobals().uiOEMCP),
     _uScrollScale(1),
@@ -110,7 +110,7 @@ void Settings::ApplyDesktopSpecificDefaults()
     _bQuickEdit = TRUE;
     _uHistoryBufferSize = 50;
     _uNumberOfHistoryBuffers = 4;
-    _bHistoryNoDup = true;
+    _bHistoryNoDup = FALSE;
 
     _renderSettings.ResetColorTable();
 

src/inc/til/throttled_func.h

@@ -30,12 +30,22 @@ namespace til
                 }
             }
 
-            std::tuple<Args...> take()
+            void apply(const auto& func)
             {
-                std::unique_lock guard{ _lock };
-                auto pendingRunArgs = std::move(*_pendingRunArgs);
-                _pendingRunArgs.reset();
-                return pendingRunArgs;
+                decltype(_pendingRunArgs) args;
+                {
+                    std::unique_lock guard{ _lock };
+                    args = std::exchange(_pendingRunArgs, std::nullopt);
+                }
+                // Theoretically it should always have a value, because the throttled_func
+                // should not call the callback without there being a reason.
+                // But in practice a failure here was observed at least once.
+                // It's unknown to me what caused it, so the best we can do is avoid a crash.
+                assert(args.has_value());
+                if (args)
+                {
+                    std::apply(func, *args);
+                }
             }
 
             explicit operator bool() const
@@ -60,10 +70,12 @@ namespace til
                 return _isPending.exchange(true, std::memory_order_relaxed);
             }
 
-            std::tuple<> take()
+            void apply(const auto& func)
             {
-                reset();
-                return {};
+                if (_isPending.exchange(false, std::memory_order_relaxed))
+                {
+                    func();
+                }
             }
 
             void reset()
@@ -171,31 +183,24 @@ namespace til
         void flush()
         {
             WaitForThreadpoolTimerCallbacks(_timer.get(), true);
-            if (_storage)
-            {
-                _trailing_edge();
-            }
+            _timer_callback(nullptr, this, nullptr);
         }
 
     private:
         static void __stdcall _timer_callback(PTP_CALLBACK_INSTANCE /*instance*/, PVOID context, PTP_TIMER /*timer*/) noexcept
         try
         {
-            static_cast<throttled_func*>(context)->_trailing_edge();
-        }
-        CATCH_LOG()
-
-        void _trailing_edge()
-        {
+            const auto self = static_cast<throttled_func*>(context);
             if constexpr (Leading)
             {
-                _storage.reset();
+                self->_storage.reset();
             }
             else
             {
-                std::apply(_func, _storage.take());
+                self->_storage.apply(self->_func);
             }
         }
+        CATCH_LOG()
 
         wil::unique_threadpool_timer _createTimer()
         {

src/inc/til/winrt.h

@@ -8,12 +8,12 @@ namespace til // Terminal Implementation Library. Also: "Today I Learned"
     template<typename T>
     struct property
     {
-        explicit constexpr property(auto&&... args) :
+        explicit constexpr property(auto&&... args) noexcept(std::is_nothrow_constructible_v<T, decltype(args)...>) :
             _value{ std::forward<decltype(args)>(args)... } {}
 
         property& operator=(const property& other) = default;
 
-        T operator()() const noexcept
+        T operator()() const noexcept(std::is_nothrow_copy_constructible<T>::value)
         {
             return _value;
         }

src/internal/stubs.cpp

@@ -3,6 +3,9 @@
 
 #include "precomp.h"
 
+#include <dwmapi.h>
+#include <uxtheme.h>
+
 #include "../inc/conint.h"
 
 using namespace Microsoft::Console::Internal;
@@ -21,7 +24,27 @@ using namespace Microsoft::Console::Internal;
     return S_OK;
 }
 
-[[nodiscard]] HRESULT Theming::TrySetDarkMode(HWND /*hwnd*/) noexcept
+[[nodiscard]] HRESULT Theming::TrySetDarkMode(HWND hwnd) noexcept
 {
-    return S_FALSE;
+    static constexpr auto subKey = LR"(SOFTWARE\Microsoft\Windows\CurrentVersion\Themes\Personalize)";
+    static constexpr auto get = [](HKEY key, const wchar_t* value, DWORD& data) {
+        DWORD dataType = 0;
+        DWORD size = sizeof(data);
+        return RegGetValueW(key, subKey, value, RRF_RT_REG_DWORD, &dataType, &data, &size);
+    };
+
+    // This is the approach that WinUI3 used in its initial source code release at the time of writing.
+    DWORD useLightTheme = 0;
+    if (get(HKEY_CURRENT_USER, L"AppsUseLightTheme", useLightTheme) != ERROR_SUCCESS)
+    {
+        if (get(HKEY_LOCAL_MACHINE, L"SystemUsesLightTheme", useLightTheme) != ERROR_SUCCESS)
+        {
+            useLightTheme = 0;
+        }
+    }
+
+    const BOOL useDarkMode = useLightTheme == 0;
+    LOG_IF_FAILED(SetWindowTheme(hwnd, useLightTheme ? L"" : L"DarkMode_Explorer", nullptr));
+    LOG_IF_FAILED(DwmSetWindowAttribute(hwnd, DWMWA_USE_IMMERSIVE_DARK_MODE, &useDarkMode, sizeof(useDarkMode)));
+    return S_OK;
 }

src/propsheet/registry.cpp

@@ -79,7 +79,7 @@ VOID InitRegistryValues(
     pStateInfo->CursorSize = 25;
     pStateInfo->HistoryBufferSize = 25;
     pStateInfo->NumberOfHistoryBuffers = 4;
-    pStateInfo->HistoryNoDup = 1;
+    pStateInfo->HistoryNoDup = 0;
 
     // clang-format off
     if (pStateInfo->fIsV2Console)

src/server/ProcessList.cpp

@@ -211,9 +211,15 @@ ConsoleProcessHandle* ConsoleProcessList::GetOldestProcess() const
     {
         termRecords.clear();
 
+        // The caller (ProcessCtrlEvents) expects them in newest-to-oldest order,
+        // because that's how Windows has historically always dispatched these events.
+        auto it = _processes.crbegin();
+        const auto end = _processes.crend();
+
         // Dig through known processes looking for a match
-        for (const auto& p : _processes)
+        for (; it != end; ++it)
         {
+            const auto p = *it;
             // If no limit was specified OR if we have a match, generate a new termination record.
             if (!dwLimitingProcessId ||
                 p->_ulProcessGroupId == dwLimitingProcessId)

src/types/ThemeUtils.cpp

@@ -1,19 +0,0 @@
-#include "precomp.h"
-#include "inc/ThemeUtils.h"
-
-namespace Microsoft::Console::ThemeUtils
-{
-    // Routine Description:
-    // - Attempts to enable/disable the dark mode on the frame of a window.
-    // Arguments:
-    // - hwnd: handle to the window to change
-    // - enabled: whether to enable or not the dark mode on the window's frame
-    // Return Value:
-    // - S_OK or suitable HRESULT from DWM engines.
-    [[nodiscard]] HRESULT SetWindowFrameDarkMode(HWND /* hwnd */, bool /* enabled */) noexcept
-    {
-        // TODO:GH #3425 implement the new DWM API and change
-        //  src/interactivity/win32/windowtheme.cpp to use it.
-        return S_OK;
-    }
-}

src/types/inc/ThemeUtils.h

@@ -1,8 +0,0 @@
-#pragma once
-
-#include <Windows.h>
-
-namespace Microsoft::Console::ThemeUtils
-{
-    [[nodiscard]] HRESULT SetWindowFrameDarkMode(HWND hwnd, bool enabled) noexcept;
-}

src/types/lib/types.vcxproj

@@ -18,7 +18,6 @@
     <ClCompile Include="..\GlyphWidth.cpp" />
     <ClCompile Include="..\ScreenInfoUiaProviderBase.cpp" />
     <ClCompile Include="..\sgrStack.cpp" />
-    <ClCompile Include="..\ThemeUtils.cpp" />
     <ClCompile Include="..\UiaTextRangeBase.cpp" />
     <ClCompile Include="..\UiaTracing.cpp" />
     <ClCompile Include="..\TermControlUiaTextRange.cpp" />
@@ -38,7 +37,6 @@
     <ClInclude Include="..\inc\GlyphWidth.hpp" />
     <ClInclude Include="..\inc\IInputEvent.hpp" />
     <ClInclude Include="..\inc\sgrStack.hpp" />
-    <ClInclude Include="..\inc\ThemeUtils.h" />
     <ClInclude Include="..\inc\utils.hpp" />
     <ClInclude Include="..\inc\Viewport.hpp" />
     <ClInclude Include="..\IUiaEventDispatcher.h" />

src/types/lib/types.vcxproj.filters

@@ -42,9 +42,6 @@
     <ClCompile Include="..\UiaTextRangeBase.cpp">
       <Filter>Source Files</Filter>
     </ClCompile>
-    <ClCompile Include="..\ThemeUtils.cpp">
-      <Filter>Source Files</Filter>
-    </ClCompile>
     <ClCompile Include="..\sgrStack.cpp">
       <Filter>Source Files</Filter>
     </ClCompile>
@@ -107,9 +104,6 @@
     <ClInclude Include="..\inc\utils.hpp">
       <Filter>Header Files</Filter>
     </ClInclude>
-    <ClInclude Include="..\inc\ThemeUtils.h">
-      <Filter>Header Files</Filter>
-    </ClInclude>
     <ClInclude Include="..\inc\sgrStack.hpp">
       <Filter>Header Files</Filter>
     </ClInclude>

src/types/sources.inc

@@ -35,7 +35,6 @@ SOURCES= \
     ..\convert.cpp \
     ..\colorTable.cpp \
     ..\utils.cpp \
-    ..\ThemeUtils.cpp \
     ..\ScreenInfoUiaProviderBase.cpp \
     ..\sgrStack.cpp \
     ..\UiaTextRangeBase.cpp \