Repository: microsoft/terminal (language: cpp)

Issue:
Crash in `Monarch_::GetPID`
This one's wild, because it don't make NO sense.

MSFT:38540483


<details>
<summary>Some verbatim Team notes, because searching teams is impossible and I won't be able to find this again</summary>

> okay so second dump from bottom
> 
> ```
> 0:000> lmDvmWindowsTerminal
> Browse full module list
> start             end                 module name
> 00007ff6`75e00000 00007ff6`75e75000   WindowsTerminal   (no symbols)           
>     Loaded symbol image file: WindowsTerminal.exe
>     Mapped memory image file: C:\ProgramData\Dbg\sym\WindowsTerminal.exe\622F96A975000\WindowsTerminal.exe
>     Image path: C:\Program Files\WindowsApps\Microsoft.WindowsTerminal_1.12.10733.0_x64__8wekyb3d8bbwe\WindowsTerminal.exe
>     Image name: WindowsTerminal.exe
>     Browse all global symbols  functions  data
>     Timestamp:        Mon Mar 14 14:25:29 2022 (622F96A9)
>     CheckSum:         00071A4E
>     ImageSize:        00075000
>     File version:     1.12.2203.14003
>     Product version:  1.12.2203.14003
>     File flags:       0 (Mask 3F)
>     File OS:          4 Unknown Win32
>     File type:        1.0 App
>     File date:        00000000.00000000
>     Translations:     0000.04b0
>     Information from resource tables:
> 0:000> lmDvmMicrosoft_Terminal_Remoting
> Browse full module list
> start             end                 module name
> 00007ffe`ece80000 00007ffe`ecec3000   Microsoft_Terminal_Remoting   (private pdb symbols)  C:\ProgramData\Dbg\sym\Microsoft.Terminal.Remoting.pdb\A3DE2F1865ED46C490034B3D23F05EAE1\Microsoft.Terminal.Remoting.pdb
>     Loaded symbol image file: Microsoft.Terminal.Remoting.dll
>     Mapped memory image file: C:\ProgramData\Dbg\sym\Microsoft.Terminal.Remoting.dll\622F940D43000\Microsoft.Terminal.Remoting.dll
>     Image path: C:\Program Files\WindowsApps\Microsoft.WindowsTerminal_1.12.10733.0_x64__8wekyb3d8bbwe\Microsoft.Terminal.Remoting.dll
>     Image name: Microsoft.Terminal.Remoting.dll
>     Browse all global symbols  functions  data
>     Timestamp:        Mon Mar 14 14:14:21 2022 (622F940D)
>     CheckSum:         00047923
>     ImageSize:        00043000
>     File version:     1.12.2203.14003
>     Product version:  1.12.2203.14003
>     File flags:       0 (Mask 3F)
>     File OS:          4 Unknown Win32
>     File type:        2.0 Dll
>     File date:        00000000.00000000
>     Translations:     0000.04b0
>     Information from resource tables:
> ```
> 
> in `WindowManager::_createMonarchAndCallbacks`, `_monarch` isn't null in `WindowManager::WindowManager`, there's a `try/catch(...)` around `_createMonarchAndCallbacks()`. So even if that did A/V in `GetPID`, it should be caught (then try again)
> 
> modules look like the same version

> hmmmm
> 
> but you can't catch a null pointer dereference
> 
> i was wondering if it was a torn version thing insofar as the monarch is running one version and the new executable is a different one
> 
> and it tried to create a monarch by clsid, got the old one, failed to talk to it

> but that wouldn't be a A/V, would it?
> 
> would it


> thats where i get scared. here's why
> 
> In one dump, i went up a few stack frames?
> 
> `_monarch->m_ptr` was NOT null

> yea i don't see any null monarchs

> and then i got to wondering
> 
> if cppwinrt-Monarch internally QIs for IMonarch and gets null, will it blow up if it hits that line
> 
> the autogenerated code doesn't show up in the debugger

> ```
> #define WINRT_IMPL_SHIM(...) (*(abi_t<__VA_ARGS__>**)&static_cast<__VA_ARGS__ const&>(static_cast<D const&>(*this)))
> ```
> 
> ```
> check_hresult(WINRT_IMPL_SHIM(winrt::Microsoft::Terminal::Remoting::IMonarch)->GetPID(&result));(*(abi_t<winrt::Microsoft::Terminal::Remoting::IMonarch>**)&static_cast<winrt::Microsoft::Terminal::Remoting::IMonarch const&>(static_cast<D const&>(*this)))D is winrt::Microsoft::Terminal::Remoting::Monarchso
> ```
> 
> ```
> (*(abi_t<winrt::Microsoft::Terminal::Remoting::IMonarch>**)&static_cast<winrt::Microsoft::Terminal::Remoting::IMonarch const&>(static_cast<winrt::Microsoft::Terminal::Remoting::Monarch const&>(*this)))
> ```
> 
> ```
> (
>     *(abi_t<winrt::Microsoft::Terminal::Remoting::IMonarch>**)
>     &static_cast<winrt::Microsoft::Terminal::Remoting::IMonarch const&>(
>         static_cast<winrt::Microsoft::Terminal::Remoting::Monarch const&>(*this)
>     )
> )
> ```
> 
> so maybe the old one wasn't a IMonarch
> 
> but still this would go away once folks are settled
> 
> from a cold start this shouldn't be hittable, only in the middle of an update, if that's really the truth of the matter
> 
> could we... test this?


> that's my understanding
> 
> i don't know how to get it to partially stage an update
> 
> that's my big issue
> 
> that would help us test the FONT thing, the DLL thing, and now this
> 
> OH WAIT
> 
> OH OH WAIT


> launch a dev build of the Terminal that doesn't have the IMonarch change, then swap out Remoting.dll/winmd and do it again?


> or just
> 
> does unpackaged glom with packaged?
> 
> does unpackaged glom with unpackaged?

> .....................................

> if so, yeah, just unpack 10732 and 10532

> unpackaged gloms to, well
> 
> hm
> 
> gloms to packaged?
> 
> so an unpackaged 10532 talking with a packaged 10732

> other way perhaps
> 
> since 10732 talks IMonarchese

> right
> 
> okay I can test that (in the morning)

> I had 5 minutes spare
> 
> so I tested it
> 
> In the torn version state, it's much worse than I expected! It logs E_NOINTERFACE thousands and thousands of time in a tight infinite loop



</details>


Discussion / hints:

Fix patch:
diff --git a/src/cascadia/Remoting/Monarch.h b/src/cascadia/Remoting/Monarch.h
index ae85396b29a..a9eac206daf 100644
--- a/src/cascadia/Remoting/Monarch.h
+++ b/src/cascadia/Remoting/Monarch.h
@@ -27,7 +27,7 @@ constexpr GUID Monarch_clsid
         0x7eb1,
         0x4f3e,
     {
-        0x85, 0xf5, 0x8b, 0xdd, 0x73, 0x86, 0xcc, 0xe3
+        0x85, 0xf5, 0x8b, 0xdd, 0x73, 0x86, 0xcc, 0xe4
     }
 };
 
diff --git a/src/cascadia/Remoting/WindowManager.cpp b/src/cascadia/Remoting/WindowManager.cpp
index 89f337830bf..d9d42100c8e 100644
--- a/src/cascadia/Remoting/WindowManager.cpp
+++ b/src/cascadia/Remoting/WindowManager.cpp
@@ -347,6 +347,52 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
     void WindowManager::_createMonarchAndCallbacks()
     {
         _createMonarch();
+
+        if (_monarch == nullptr)
+        {
+            // See MSFT:38540483, GH#12774 for details.
+            TraceLoggingWrite(g_hRemotingProvider,
+                              "WindowManager_NullMonarchTryAgain",
+                              TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
+                              TraceLoggingKeyword(TIL_KEYWORD_TRACE));
+            // Here we're gonna just give it a quick second try.Probably not
+            // definitive, but might help.
+            _createMonarch();
+        }
+
+        if (_monarch == nullptr)
+        {
+            // See MSFT:38540483, GH#12774 for details.
+            if constexpr (Feature_IsolatedMonarchMode::IsEnabled())
+            {
+                // Fall back to having a in proc monarch. Were now isolated from
+                // other windows. This is a pretty torn state, but at least we
+                // didn't just explode.
+                TraceLoggingWrite(g_hRemotingProvider,
+                                  "WindowManager_NullMonarchIsolateMode",
+                                  TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
+                                  TraceLoggingKeyword(TIL_KEYWORD_TRACE));
+
+                _monarch = winrt::make<winrt::Microsoft::Terminal::Remoting::implementation::Monarch>();
+            }
+            else
+            {
+                // The monarch is null. We're hoping that we can find another,
+                // hopefully us. We're gonna go back around the loop again and
+                // see what happens. If this is really an infinite loop (where
+                // the OS won't even give us back US as the monarch), then I
+                // suppose we'll find out soon enough.
+                TraceLoggingWrite(g_hRemotingProvider,
+                                  "WindowManager_NullMonarchTryAgain",
+                                  TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
+                                  TraceLoggingKeyword(TIL_KEYWORD_TRACE));
+
+                winrt::hresult_error(E_UNEXPECTED, L"Did not expect the Monarch to ever be null");
+            }
+        }
+
+        // We're pretty confident that we have a Monarch here.
+
         // Save the result of checking if we're the king. We want to avoid
         // unnecessary calls back and forth if we can.
         _isKing = _areWeTheKing();
diff --git a/src/features.xml b/src/features.xml
index c2fde9d2765..d08b97a9026 100644
--- a/src/features.xml
+++ b/src/features.xml
@@ -95,4 +95,15 @@
             <brandingToken>Preview</brandingToken>
         </alwaysEnabledBrandingTokens>
     </feature>
+
+    <feature>
+        <name>Feature_IsolatedMonarchMode</name>
+        <description>Enables a test flag for MSFT:38540483. When enabled, if we ever create a null Monarch, we'll stealthily try to fall back to an in-proc monarch instance.</description>
+        <stage>AlwaysEnabled</stage>
+        <!-- Did it this way instead of "release tokens" to ensure it will go into Windows Inbox -->
+        <alwaysDisabledBrandingTokens>
+            <brandingToken>Dev</brandingToken>
+            <brandingToken>Preview</brandingToken>
+        </alwaysDisabledBrandingTokens>
+    </feature>
 </featureStaging>


Test patch: