Skip to content

Commit

Permalink
Make sure we fully clean up state when closing a peasant (#11217)
Browse files Browse the repository at this point in the history
Fix infinite loop when trying to summon a window after close.

In #10972 code was added to try to clean up state manually when a window
was closed instead of waiting for it to be detected as a dead peasant.
Unfortunately I didn't know any better and missed cleaning up
`_mruPeasants` as well. The result is there  would be an infinite loop
in `_getMostRecentPeasant` since `_getPeasant` will only clean up ids if
it finds a peasant, not if it doesn't find anything. This is the minimal
change to get this working, but it might be a good idea to make
`_getPeasant` be more thorough about cleanup.

## Validation Steps Performed
Tested that before the change we infinitely loop, and after the change
we summon correctly.

Closes #11215
  • Loading branch information
Rosefield authored Sep 14, 2021
1 parent 3d7480e commit b4a40ff
Show file tree
Hide file tree
Showing 2 changed files with 142 additions and 0 deletions.
1 change: 1 addition & 0 deletions src/cascadia/Remoting/Monarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -147,6 +147,7 @@ namespace winrt::Microsoft::Terminal::Remoting::implementation
// - <none>
void Monarch::SignalClose(const uint64_t peasantId)
{
_clearOldMruEntries(peasantId);
_peasants.erase(peasantId);
_WindowClosedHandlers(nullptr, nullptr);
}
Expand Down
141 changes: 141 additions & 0 deletions src/cascadia/UnitTests_Remoting/RemotingTests.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -140,11 +140,16 @@ namespace RemotingUnitTests

TEST_METHOD(TestSummonMostRecentIsQuake);

TEST_METHOD(TestSummonAfterWindowClose);

TEST_CLASS_SETUP(ClassSetup)
{
return true;
}

static void _closePeasant(const com_ptr<Remoting::implementation::Monarch>& m,
const uint64_t peasantID);

static void _killPeasant(const com_ptr<Remoting::implementation::Monarch>& m,
const uint64_t peasantID);

Expand All @@ -164,6 +169,19 @@ namespace RemotingUnitTests
}
};

// Helper to tell the monarch that a peasant is closing, this emulates when
// a peasant is closed normally instead of when it crashes.
void RemotingTests::_closePeasant(const com_ptr<Remoting::implementation::Monarch>& m,
const uint64_t peasantID)
{
if (peasantID <= 0)
{
return;
}

m->SignalClose(peasantID);
}

// Helper to replace the specified peasant in a monarch with a
// "DeadPeasant", which will emulate what happens when the peasant process
// dies.
Expand Down Expand Up @@ -2378,4 +2396,127 @@ namespace RemotingUnitTests
}
}

void RemotingTests::TestSummonAfterWindowClose()
{
Log::Comment(L"Test that we can summon a window on the current desktop,"
L" when the MRU window on that desktop closes normally.");

const winrt::guid guid1{ Utils::GuidFromString(L"{11111111-1111-1111-1111-111111111111}") };
const winrt::guid guid2{ Utils::GuidFromString(L"{22222222-2222-2222-2222-222222222222}") };

constexpr auto monarch0PID = 12345u;
constexpr auto peasant1PID = 23456u;
constexpr auto peasant2PID = 34567u;
constexpr auto peasant3PID = 45678u;

auto m0 = make_private<Remoting::implementation::Monarch>(monarch0PID);
auto p1 = make_private<Remoting::implementation::Peasant>(peasant1PID);
auto p2 = make_private<Remoting::implementation::Peasant>(peasant2PID);
auto p3 = make_private<Remoting::implementation::Peasant>(peasant3PID);

p1->WindowName(L"one");
p2->WindowName(L"two");
p3->WindowName(L"three");

VERIFY_ARE_EQUAL(0, p1->GetID());
VERIFY_ARE_EQUAL(0, p2->GetID());
VERIFY_ARE_EQUAL(0, p3->GetID());

m0->AddPeasant(*p1);
m0->AddPeasant(*p2);
m0->AddPeasant(*p3);

VERIFY_ARE_EQUAL(1, p1->GetID());
VERIFY_ARE_EQUAL(2, p2->GetID());
VERIFY_ARE_EQUAL(3, p3->GetID());

VERIFY_ARE_EQUAL(3u, m0->_peasants.size());

bool p1ExpectedToBeSummoned = false;
bool p2ExpectedToBeSummoned = false;
bool p3ExpectedToBeSummoned = false;

p1->SummonRequested([&](auto&&, auto&&) {
Log::Comment(L"p1 summoned");
VERIFY_IS_TRUE(p1ExpectedToBeSummoned);
});
p2->SummonRequested([&](auto&&, auto&&) {
Log::Comment(L"p2 summoned");
VERIFY_IS_TRUE(p2ExpectedToBeSummoned);
});
p3->SummonRequested([&](auto&&, auto&&) {
Log::Comment(L"p3 summoned");
VERIFY_IS_TRUE(p3ExpectedToBeSummoned);
});

{
Log::Comment(L"Activate the first peasant, first desktop");
Remoting::WindowActivatedArgs activatedArgs{ p1->GetID(),
p1->GetPID(), // USE PID as HWND, because these values don't _really_ matter
guid1,
winrt::clock().now() };
p1->ActivateWindow(activatedArgs);
}
{
Log::Comment(L"Activate the second peasant, second desktop");
Remoting::WindowActivatedArgs activatedArgs{ p2->GetID(),
p2->GetPID(), // USE PID as HWND, because these values don't _really_ matter
guid2,
winrt::clock().now() };
p2->ActivateWindow(activatedArgs);
}
{
Log::Comment(L"Activate the third peasant, first desktop");
Remoting::WindowActivatedArgs activatedArgs{ p3->GetID(),
p3->GetPID(), // USE PID as HWND, because these values don't _really_ matter
guid1,
winrt::clock().now() };
p3->ActivateWindow(activatedArgs);
}

Log::Comment(L"Create a mock IVirtualDesktopManager to handle checking if a window is on a given desktop");
auto manager = winrt::make_self<MockDesktopManager>();
m0->_desktopManager = manager.try_as<IVirtualDesktopManager>();

auto firstCallback = [&](HWND h, BOOL* result) -> HRESULT {
Log::Comment(L"firstCallback: Checking if window is on desktop 1");

const uint64_t hwnd = reinterpret_cast<uint64_t>(h);
if (hwnd == peasant1PID || hwnd == peasant3PID)
{
*result = true;
}
else if (hwnd == peasant2PID)
{
*result = false;
}
else
{
VERIFY_IS_TRUE(false, L"IsWindowOnCurrentVirtualDesktop called with unexpected value");
}
return S_OK;
};
manager->pfnIsWindowOnCurrentVirtualDesktop = firstCallback;

Remoting::SummonWindowSelectionArgs args;

Log::Comment(L"Summon window three - it is the MRU on desktop 1");
p3ExpectedToBeSummoned = true;
args.OnCurrentDesktop(true);
m0->SummonWindow(args);
VERIFY_IS_TRUE(args.FoundMatch());

Log::Comment(L"Close window 3. Window 1 is now the MRU on desktop 1.");
RemotingTests::_closePeasant(m0, p3->GetID());

Log::Comment(L"Summon window three - it is the MRU on desktop 1");
p1ExpectedToBeSummoned = true;
p2ExpectedToBeSummoned = false;
p3ExpectedToBeSummoned = false;
args.FoundMatch(false);
args.OnCurrentDesktop(true);
m0->SummonWindow(args);
VERIFY_IS_TRUE(args.FoundMatch());
}

}

0 comments on commit b4a40ff

Please sign in to comment.