Repository: microsoft/terminal
File: src/cascadia/Remoting/WindowManager.cpp (C++)

Code before:
        //   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)
        {
            try
            {
                result = _monarch.ProposeCommandline(args);
                proposedCommandline = true;
            }
            catch (const winrt::hresult_error& e)
            {
                // 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,

Review comment (bug):
Copying the `outOfProcResult` still involves the exact same call to `ShouldCreateWindow` which might fail, right? So isn't the underlying bug, that we didn't protect the call to `ShouldCreateWindow` down below with a try/catch?

If I'm right, I don't like this fix _that_ much because it hides a potential source of exceptions inside a constructor call, which is a bit "opaque".

IMO we should consider moving this block from down below:
```cpp
_shouldCreateWindow = result.ShouldCreateWindow();
if (result.Id())
{
    givenID = result.Id().Value();
}
givenName = result.WindowName();
```

...up here, which would fix the bug the exact same way.
Afterwards we can move the "dangerous" `result` variable inside this block, preventing any further misuse.

Code after:
        // * 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)
        {
            try
            {
                // MSFT:38542548 _We believe_ that this is the source of the
                // crash here. After we get the result, stash it's values into a
                // local copy, so that we can check them later. If the Monarch
                // dies between now and the inspection of
                // `result.ShouldCreateWindow` below, we don't want to explode
                // (since _proposeToMonarch is not try/caught).
                Remoting::ProposeCommandlineResult outOfProcResult = _monarch.ProposeCommandline(args);
                result = winrt::make<implementation::ProposeCommandlineResult>(outOfProcResult);

                proposedCommandline = true;
            }
            catch (const winrt::hresult_error& e)
            {
                // 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.