Skip to content

Commit

Permalink
Hopefully fix the HandleCommandlineArgs crashes
Browse files Browse the repository at this point in the history
  This is an experiment, as discussed in #11790 (comment). 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.
  • Loading branch information
zadjii-msft committed Jul 26, 2022
1 parent 6c2316d commit a53dc65
Showing 1 changed file with 42 additions and 60 deletions.
102 changes: 42 additions & 60 deletions src/cascadia/Remoting/WindowManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
}

Expand Down

1 comment on commit a53dc65

@github-actions

This comment was marked as outdated.

Please sign in to comment.