Fix a pair of crashes, likely related to defterm (#12666)

This fixes a pair of inbox bugs, hopefully.

* MSFT:35731327
  * There's a small window where a peasant is being created when a monarch is exiting. When that happens, the new peasant will try to tell itself (the new monarch) when the peasant was last activated, but because the window hasn't actually finished instantiating, the peasant doesn't yet have a LastActivatedArgs to tell the monarch about.
* MSFT:32518679 (ARM version) / MSFT:32279047 (AMD64 version)
  * This one's tricky. Not totally sure this is the fix, bug assuming my hypothesis is correct, this should fix it. Regardless, this does fix a bug that was in the code.
  * If the king dies right as another window is starting, right while the new window is starting to ProposeCommandline to the monarch, the monarch could die. If it does, the new window just explodes too. Not what you want.

Vaguely tested the second bug manually, by setting breakpoints in the monarch, starting a defterm, then exiting the monarch while the handoff was in process. That now creates a new window, so that's at least something. `RemotingTests::TestProposeCommandlineWithDeadMonarch` was the closest I could get to testing that.

The first bug only got an eye check. Not sure how to repro, but I figured yeet and hopefully we get it.

* [x] Closes #12624

(cherry picked from commit f507d9f)

.github/actions/spelling/allow/apis.txt

@@ -64,6 +64,7 @@ IFACEMETHOD
 IFile
 IInheritable
 IMap
+IMonarch
 IObject
 iosfwd
 IPackage

src/cascadia/Remoting/Monarch.cpp

@@ -353,6 +353,14 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
     // - <none>
     void Monarch::HandleActivatePeasant(const Remoting::WindowActivatedArgs& args)
     {
+        if (args == nullptr)
+        {
+            // MSFT:35731327, GH #12624. There's a chance that the way the
+            // window gets set up for defterm, the ActivatedArgs haven't been
+            // created for this window yet. Check here and just ignore them if
+            // they're null. They'll come back with real args soon
+            return;
+        }
         // Start by making a local copy of these args. It's easier for us if our
         // tracking of these args is all in-proc. That way, the only thing that
         // could fail due to the peasant dying is _this first copy_.
@@ -418,6 +426,9 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
     // - <none>
     void Monarch::_doHandleActivatePeasant(const winrt::com_ptr<implementation::WindowActivatedArgs>& localArgs)
     {
+        // We're sure that localArgs isn't null here, we checked before in our
+        // one caller (in Monarch::HandleActivatePeasant)
+
         const auto newLastActiveTime = localArgs->ActivatedTime().time_since_epoch().count();
 
         // * Check all the current lists to look for this peasant.

src/cascadia/Remoting/Monarch.idl

@@ -44,8 +44,8 @@ namespace Microsoft.Terminal.Remoting
         String TabTitle;
     };
 
-    [default_interface] runtimeclass Monarch {
-        Monarch();
+    interface IMonarch
+    {
 
         UInt64 GetPID();
         UInt64 AddPeasant(IPeasant peasant);
@@ -67,4 +67,9 @@ namespace Microsoft.Terminal.Remoting
         event Windows.Foundation.TypedEventHandler<Object, Object> WindowClosed;
         event Windows.Foundation.TypedEventHandler<Object, QuitAllRequestedArgs> QuitAllRequested;
     };
+
+    [default_interface] runtimeclass Monarch : IMonarch
+    {
+        Monarch();
+    };
 }

src/cascadia/Remoting/WindowManager.cpp

@@ -77,57 +77,161 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
         }
     }
 
-    void WindowManager::ProposeCommandline(const Remoting::CommandlineArgs& args)
+    void WindowManager::_proposeToMonarch(const Remoting::CommandlineArgs& args,
+                                          std::optional<uint64_t>& givenID,
+                                          winrt::hstring& givenName)
     {
-        // If we're the king, we _definitely_ want to process the arguments, we were
-        // launched with them!
-        //
-        // Otherwise, the King will tell us if we should make a new window
-        _shouldCreateWindow = _isKing;
-        std::optional<uint64_t> givenID;
-        winrt::hstring givenName{};
-        if (!_isKing)
+        // these two errors are Win32 errors, convert them to HRESULTS so we can actually compare below.
+        static constexpr HRESULT RPC_SERVER_UNAVAILABLE_HR = HRESULT_FROM_WIN32(RPC_S_SERVER_UNAVAILABLE);
+        static constexpr HRESULT RPC_CALL_FAILED_HR = HRESULT_FROM_WIN32(RPC_S_CALL_FAILED);
+
+        // The monarch may respond back "you should be a new
+        // window, with ID,name of (id, name)". Really the responses are:
+        // * You should not create a new window
+        // * Create a new window (but without a given ID or name). The
+        //   Monarch will assign your ID/name later
+        // * Create a new window, and you'll have this ID or name
+        //   - This is the case where the user provides `wt -w 1`, and
+        //     there's no existing window 1
+
+        // You can emulate the monarch dying by: starting a terminal, sticking a
+        // breakpoint in
+        // TerminalApp!winrt::TerminalApp::implementation::AppLogic::_doFindTargetWindow,
+        // starting a defterm, and when that BP gets hit, kill the original
+        // monarch, and see what happens here.
+
+        bool proposedCommandline = false;
+        Remoting::ProposeCommandlineResult result{ nullptr };
+        while (!proposedCommandline)
         {
-            // The monarch may respond back "you should be a new
-            // window, with ID,name of (id, name)". Really the responses are:
-            // * You should not create a new window
-            // * Create a new window (but without a given ID or name). The
-            //   Monarch will assign your ID/name later
-            // * Create a new window, and you'll have this ID or name
-            //   - This is the case where the user provides `wt -w 1`, and
-            //     there's no existing window 1
-
-            const auto result = _monarch.ProposeCommandline(args);
-            _shouldCreateWindow = result.ShouldCreateWindow();
-            if (result.Id())
+            try
             {
-                givenID = result.Id().Value();
+                result = _monarch.ProposeCommandline(args);
+                proposedCommandline = true;
             }
-            givenName = result.WindowName();
-            // TraceLogging doesn't have a good solution for logging an
-            // optional. So we have to repeat the calls here:
-            if (givenID)
+            catch (const winrt::hresult_error& e)
             {
-                TraceLoggingWrite(g_hRemotingProvider,
-                                  "WindowManager_ProposeCommandline",
-                                  TraceLoggingBoolean(_shouldCreateWindow, "CreateWindow", "true iff we should create a new window"),
-                                  TraceLoggingUInt64(givenID.value(), "Id", "The ID we should assign our peasant"),
-                                  TraceLoggingWideString(givenName.c_str(), "Name", "The name we should assign this window"),
-                                  TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
-                                  TraceLoggingKeyword(TIL_KEYWORD_TRACE));
+                // We did not successfully ask the king what to do. They
+                // hopefully just died here. That's okay, let's just go ask the
+                // next in the line of succession. At the very worst, we'll find
+                // _us_, (likely last in the line).
+                //
+                // If the king returned some _other_ error here, than lets
+                // bubble that up because that's a real issue.
+                //
+                // I'm checking both these here. I had previously got a
+                // RPC_S_CALL_FAILED about here once.
+                if (e.code() == RPC_SERVER_UNAVAILABLE_HR || e.code() == RPC_CALL_FAILED_HR)
+                {
+                    TraceLoggingWrite(g_hRemotingProvider,
+                                      "WindowManager_proposeToMonarch_kingDied",
+                                      TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
+                                      TraceLoggingKeyword(TIL_KEYWORD_TRACE));
+
+                    // We failed to ask the monarch. It must have died. Try and
+                    // find the real monarch. Don't perform an election, that
+                    // assumes we have a peasant, which we don't yet.
+                    _createMonarchAndCallbacks();
+                    // _createMonarchAndCallbacks will initialize _isKing
+                    if (_isKing)
+                    {
+                        // We became the king. We don't need to ProposeCommandline to ourself, we're just
+                        // going to do it.
+                        //
+                        // Return early, because there's nothing else for us to do here.
+                        TraceLoggingWrite(g_hRemotingProvider,
+                                          "WindowManager_proposeToMonarch_becameKing",
+                                          TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
+                                          TraceLoggingKeyword(TIL_KEYWORD_TRACE));
+
+                        // In WindowManager::ProposeCommandline, had we been the
+                        // king originally, we would have started by setting
+                        // this to true. We became the monarch here, so set it
+                        // here as well.
+                        _shouldCreateWindow = true;
+                        return;
+                    }
+
+                    // Here, we created the new monarch, it wasn't us, so we're
+                    // gonna go through the while loop again and ask the new
+                    // king.
+                    TraceLoggingWrite(g_hRemotingProvider,
+                                      "WindowManager_proposeToMonarch_tryAgain",
+                                      TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
+                                      TraceLoggingKeyword(TIL_KEYWORD_TRACE));
+                }
+                else
+                {
+                    TraceLoggingWrite(g_hRemotingProvider,
+                                      "WindowManager_proposeToMonarch_unexpectedResultFromKing",
+                                      TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
+                                      TraceLoggingKeyword(TIL_KEYWORD_TRACE));
+                    LOG_CAUGHT_EXCEPTION();
+                    throw;
+                }
             }
-            else
+            catch (...)
             {
+                // If the monarch (maybe us) failed for _any other reason_ than
+                // them dying. This IS quite unexpected. Let this bubble out.
                 TraceLoggingWrite(g_hRemotingProvider,
-                                  "WindowManager_ProposeCommandline",
-                                  TraceLoggingBoolean(_shouldCreateWindow, "CreateWindow", "true iff we should create a new window"),
-                                  TraceLoggingPointer(nullptr, "Id", "No ID provided"),
-                                  TraceLoggingWideString(givenName.c_str(), "Name", "The name we should assign this window"),
+                                  "WindowManager_proposeToMonarch_unexpectedExceptionFromKing",
                                   TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
                                   TraceLoggingKeyword(TIL_KEYWORD_TRACE));
+
+                LOG_CAUGHT_EXCEPTION();
+                throw;
             }
         }
+
+        // Here, the monarch (not us) has replied to the message. Get the
+        // valuables out of the response:
+        _shouldCreateWindow = result.ShouldCreateWindow();
+        if (result.Id())
+        {
+            givenID = result.Id().Value();
+        }
+        givenName = result.WindowName();
+
+        // TraceLogging doesn't have a good solution for logging an
+        // optional. So we have to repeat the calls here:
+        if (givenID)
+        {
+            TraceLoggingWrite(g_hRemotingProvider,
+                              "WindowManager_ProposeCommandline",
+                              TraceLoggingBoolean(_shouldCreateWindow, "CreateWindow", "true iff we should create a new window"),
+                              TraceLoggingUInt64(givenID.value(), "Id", "The ID we should assign our peasant"),
+                              TraceLoggingWideString(givenName.c_str(), "Name", "The name we should assign this window"),
+                              TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
+                              TraceLoggingKeyword(TIL_KEYWORD_TRACE));
+        }
         else
+        {
+            TraceLoggingWrite(g_hRemotingProvider,
+                              "WindowManager_ProposeCommandline",
+                              TraceLoggingBoolean(_shouldCreateWindow, "CreateWindow", "true iff we should create a new window"),
+                              TraceLoggingPointer(nullptr, "Id", "No ID provided"),
+                              TraceLoggingWideString(givenName.c_str(), "Name", "The name we should assign this window"),
+                              TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
+                              TraceLoggingKeyword(TIL_KEYWORD_TRACE));
+        }
+    }
+    void WindowManager::ProposeCommandline(const Remoting::CommandlineArgs& args)
+    {
+        // If we're the king, we _definitely_ want to process the arguments, we were
+        // launched with them!
+        //
+        // Otherwise, the King will tell us if we should make a new window
+        _shouldCreateWindow = _isKing;
+        std::optional<uint64_t> givenID;
+        winrt::hstring givenName{};
+        if (!_isKing)
+        {
+            _proposeToMonarch(args, givenID, givenName);
+        }
+
+        // During _proposeToMonarch, it's possible that we found that the king was dead, and we're the new king. Cool! Do this now.
+        if (_isKing)
         {
             // We're the monarch, we don't need to propose anything. We're just
             // going to do it.
@@ -195,6 +299,10 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
                 _createPeasantThread();
             }
 
+            // This right here will just tell us to stash the args away for the
+            // future. The AppHost hasnt yet set up the callbacks, and the rest
+            // of the app hasn't started at all. We'll note them and come back
+            // later.
             _peasant.ExecuteCommandline(args);
         }
         // Otherwise, we'll do _nothing_.

src/cascadia/Remoting/WindowManager.h

@@ -83,6 +83,10 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
         void _waitOnMonarchThread();
         void _raiseFindTargetWindowRequested(const winrt::Windows::Foundation::IInspectable& sender,
                                              const winrt::Microsoft::Terminal::Remoting::FindTargetWindowArgs& args);
+
+        void _proposeToMonarch(const Remoting::CommandlineArgs& args,
+                               std::optional<uint64_t>& givenID,
+                               winrt::hstring& givenName);
     };
 }