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:
Okay, so this isn't your run-of-the-mill failure. Here's what I've learned.

1. We are successfully getting a COM proxy object for `_monarch` -- therefore, it is *not null*
2. The null pointer AV is coming from (effectively) `nullptr->GetPID()`.
3. `_monarch` is holding an `IUnknown` that points at the proxy.
4. When we call `_monarch.GetPID()`, C++/WinRT does two things (effectively¹):
   1. `QueryInterface(IID_PPV_ARGS(&pMonarchAsIMonarch))`
   2. `pMonarch->GetPID()`
5. `pMonarch` is the `nullptr` from step 2

The proxy object is failing to QI for `IMonarch`. This really does suggest that this is an issue where the version of Terminal that is trying to launch (1) doesn't agree on what the IID for `IMonarch` is with the running version, or (2) has gone terribly wrong somewhere in COM land.

Checking whether `_monarch` itself is populated will never tell us if it's a live object. We *could* try to `try_as` `IMonarch` ourselves and check that instead. However, it looks like this crash usually happens in defterm and causes a fallback to conhost. 🤷🏻 

What I can't figure out is _what the heck the server is doing_ when it gets this call. We clearly have a handle to a remote object, but I have no idea who that object is or why it doesn't QI. We can trivially reproduce this crash by having a custom COM server simply camp out with a class factory object on the Monarch's CLSID.

---

¹ This comes in through an automatic `try_as`²
² Actually, this makes every `Monarch` call twice as expensive -- it QIs (over RPC) for `IMonarch` and then calls the function (over RPC again.)³
³ This is partially because _the default interface for the `Monarch` runtimeclass is actually `IMonarch2`_, which is an empty interface that does nothing⁴ but fill space/have a GUID.
⁴ See that `[default_interface]` on `runtimeclass Monarch`? If we omitted that, `IMonarch` _would be the default interface_. Because we specified it, MIDL3 generates an empty interface to be the default interface.

We can ameliorate footnote 4 by removing `default_interface`. We can address footnote 2 by using the `IMonarch` projection internally. Idly, I am wondering if doing so would move the failure up into `_createMonarch` where we could understand it, or work around it.
> We can ameliorate footnote 4 by removing `default_interface`. 

We should probably do that regardless. We don't really need that since this isn't an empty class.

> We can address footnote 2 by using the `IMonarch` projection internally

That's easy enough

<hr>

Of course, if this gets another week deeper into ask mode, we could always just yank `IMonarch` and the test that depends on it entirely. One fewer test is not the end of the world. 

Fix patch:
diff --git a/src/cascadia/Remoting/Monarch.idl b/src/cascadia/Remoting/Monarch.idl
index 08c7b63cbda..2ef09807080 100644
--- a/src/cascadia/Remoting/Monarch.idl
+++ b/src/cascadia/Remoting/Monarch.idl
@@ -68,7 +68,7 @@ namespace Microsoft.Terminal.Remoting
         event Windows.Foundation.TypedEventHandler<Object, QuitAllRequestedArgs> QuitAllRequested;
     };
 
-    [default_interface] runtimeclass Monarch : IMonarch
+    runtimeclass Monarch : [default] IMonarch
     {
         Monarch();
     };
diff --git a/src/cascadia/Remoting/WindowManager.cpp b/src/cascadia/Remoting/WindowManager.cpp
index 7f7ad363acf..4c8198c2741 100644
--- a/src/cascadia/Remoting/WindowManager.cpp
+++ b/src/cascadia/Remoting/WindowManager.cpp
@@ -341,8 +341,8 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
         //
         // * If we're running unpackaged: the .winmd must be a sibling of the .exe
         // * If we're running packaged: the .winmd must be in the package root
-        _monarch = create_instance<Remoting::Monarch>(Monarch_clsid,
-                                                      CLSCTX_LOCAL_SERVER);
+        _monarch = create_instance<Remoting::IMonarch>(Monarch_clsid,
+                                                       CLSCTX_LOCAL_SERVER);
     }
 
     // NOTE: This can throw! Callers include:
diff --git a/src/cascadia/Remoting/WindowManager.h b/src/cascadia/Remoting/WindowManager.h
index 5ea2c56eb6f..2eda5edcc87 100644
--- a/src/cascadia/Remoting/WindowManager.h
+++ b/src/cascadia/Remoting/WindowManager.h
@@ -64,7 +64,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
         bool _shouldCreateWindow{ false };
         bool _isKing{ false };
         DWORD _registrationHostClass{ 0 };
-        winrt::Microsoft::Terminal::Remoting::Monarch _monarch{ nullptr };
+        winrt::Microsoft::Terminal::Remoting::IMonarch _monarch{ nullptr };
         winrt::Microsoft::Terminal::Remoting::Peasant _peasant{ nullptr };
 
         wil::unique_event _monarchWaitInterrupt;


Test patch: