Skip to content

Commit

Permalink
Try to silently fall back to a local monarch (#12825)
Browse files Browse the repository at this point in the history
This is a crazy idea Dustin and I had.

> we can't repro this at will. But we kinda have an idea of where the deref is. We don't know if the small patch (throw, and try again) will fix it. We're sure that the "just fall back to an isolated monarch" will work. I'd almost rather take a build testing the small patch first, to see if that works

> This might seem crazy
> in 1.12, isolated monarch. In 1.13, "small patch". In 1.14, we can wait and see

I can write more details in the morning. It's 5pm here so if we want this today, here it is.

@DHowett double check my velocity flag logic here. Should be always true for Release, and off for Dev, Preview. 

* [x] closes #12774
  • Loading branch information
zadjii-msft authored Apr 4, 2022
1 parent 8405c7a commit 446f280
Show file tree
Hide file tree
Showing 3 changed files with 58 additions and 1 deletion.
2 changes: 1 addition & 1 deletion src/cascadia/Remoting/Monarch.h
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ constexpr GUID Monarch_clsid
0x7eb1,
0x4f3e,
{
0x85, 0xf5, 0x8b, 0xdd, 0x73, 0x86, 0xcc, 0xe3
0x85, 0xf5, 0x8b, 0xdd, 0x73, 0x86, 0xcc, 0xe4
}
};

Expand Down
46 changes: 46 additions & 0 deletions src/cascadia/Remoting/WindowManager.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -347,6 +347,52 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
void WindowManager::_createMonarchAndCallbacks()
{
_createMonarch();

if (_monarch == nullptr)
{
// See MSFT:38540483, GH#12774 for details.
TraceLoggingWrite(g_hRemotingProvider,
"WindowManager_NullMonarchTryAgain",
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
TraceLoggingKeyword(TIL_KEYWORD_TRACE));
// Here we're gonna just give it a quick second try.Probably not
// definitive, but might help.
_createMonarch();
}

if (_monarch == nullptr)
{
// See MSFT:38540483, GH#12774 for details.
if constexpr (Feature_IsolatedMonarchMode::IsEnabled())
{
// Fall back to having a in proc monarch. Were now isolated from
// other windows. This is a pretty torn state, but at least we
// didn't just explode.
TraceLoggingWrite(g_hRemotingProvider,
"WindowManager_NullMonarchIsolateMode",
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
TraceLoggingKeyword(TIL_KEYWORD_TRACE));

_monarch = winrt::make<winrt::Microsoft::Terminal::Remoting::implementation::Monarch>();
}
else
{
// The monarch is null. We're hoping that we can find another,
// hopefully us. We're gonna go back around the loop again and
// see what happens. If this is really an infinite loop (where
// the OS won't even give us back US as the monarch), then I
// suppose we'll find out soon enough.
TraceLoggingWrite(g_hRemotingProvider,
"WindowManager_NullMonarchTryAgain",
TraceLoggingLevel(WINEVENT_LEVEL_VERBOSE),
TraceLoggingKeyword(TIL_KEYWORD_TRACE));

winrt::hresult_error(E_UNEXPECTED, L"Did not expect the Monarch to ever be null");
}
}

// We're pretty confident that we have a Monarch here.

// Save the result of checking if we're the king. We want to avoid
// unnecessary calls back and forth if we can.
_isKing = _areWeTheKing();
Expand Down
11 changes: 11 additions & 0 deletions src/features.xml
Original file line number Diff line number Diff line change
Expand Up @@ -95,4 +95,15 @@
<brandingToken>Preview</brandingToken>
</alwaysEnabledBrandingTokens>
</feature>

<feature>
<name>Feature_IsolatedMonarchMode</name>
<description>Enables a test flag for MSFT:38540483. When enabled, if we ever create a null Monarch, we'll stealthily try to fall back to an in-proc monarch instance.</description>
<stage>AlwaysEnabled</stage>
<!-- Did it this way instead of "release tokens" to ensure it will go into Windows Inbox -->
<alwaysDisabledBrandingTokens>
<brandingToken>Dev</brandingToken>
<brandingToken>Preview</brandingToken>
</alwaysDisabledBrandingTokens>
</feature>
</featureStaging>

0 comments on commit 446f280

Please sign in to comment.