From a53dc659c536e48ca523992a9dbc2f25a7d40b8f Mon Sep 17 00:00:00 2001 From: Mike Griese Date: Tue, 26 Jul 2022 12:59:55 -0500 Subject: [PATCH] Hopefully fix the HandleCommandlineArgs crashes This is an experiment, as discussed in https://github.com/microsoft/terminal/issues/11790#issuecomment-1179143049. We don't know what for sure causes these crashes, but it seems that blindly throwing, so that it gets picked up by Watson, is probably not the move. Instead, we're just gonna do our fallback, REGARDLESS of what the exception was. --- src/cascadia/Remoting/WindowManager.cpp | 102 ++++++++++-------------- 1 file changed, 42 insertions(+), 60 deletions(-) diff --git a/src/cascadia/Remoting/WindowManager.cpp b/src/cascadia/Remoting/WindowManager.cpp index cd8cbae5d58..8838edd0454 100644 --- a/src/cascadia/Remoting/WindowManager.cpp +++ b/src/cascadia/Remoting/WindowManager.cpp @@ -118,78 +118,60 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation proposedCommandline = true; } - catch (const winrt::hresult_error& e) + catch (...) { - // 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. + // We did not successfully ask the king what to do. This could + // be for many reasons. Most commonly, the monarch died as we + // were talking to it. That could be a RPC_SERVER_UNAVAILABLE_HR + // or RPC_CALL_FAILED_HR (GH#12666). We also saw a + // RPC_S_CALL_FAILED_DNE in GH#11790. Ultimately, if this is + // gonna fail, we want to just try again, regardless of the + // cause. That's why we're no longer checking what the exception + // was, we're just always gonna try again regardless. // - // 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)); + // 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). - // 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; - } + // 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_proposeToMonarch_unexpectedExceptionFromKing", + TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), + TraceLoggingKeyword(TIL_KEYWORD_TRACE)); - // 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 + LOG_CAUGHT_EXCEPTION(); + // 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_unexpectedResultFromKing", + "WindowManager_proposeToMonarch_becameKing", TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), TraceLoggingKeyword(TIL_KEYWORD_TRACE)); - LOG_CAUGHT_EXCEPTION(); - throw; + + // 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; } - } - catch (...) - { - // If the monarch (maybe us) failed for _any other reason_ than - // them dying. This IS quite unexpected. Let this bubble out. + + // 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_unexpectedExceptionFromKing", + "WindowManager_proposeToMonarch_tryAgain", TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE), TraceLoggingKeyword(TIL_KEYWORD_TRACE)); - - LOG_CAUGHT_EXCEPTION(); - throw; } }