Repository navigation
Mysterious hang in Monarch::_getMostRecentPeasantID #11215
Description
version 1.12.2521.0.
Unsure how I got into this state. Probably
- open windows 1, 2, 3. 1 is not the MRU window.
- Close 2 and 3.
- Try
globalSummonthe window. Terminal is in an infinite loop now insideMonarch::_getMostRecentPeasantID
but my repo's in the middle of a weird merge ATM so I can't verify on main
The symbols for this build don't include accurate source info, or at least my windbg is fucky. But what seems to be happening is _getMostRecentPeasantID is in the while (_mruPeasants.cbegin() + positionInList < _mruPeasants.cend()) loop. We're looking for ID=3, which we don't find in the list of peasants, so _getPeasant ends up returning null. We determine that we should have cleaned out that peasant ID from the MRU entries at that point, so we try again. However, we didn't. The MRU list still contains 3 at index 0, so we go looking for it again.
auto maybeThePeasant = peasantSearch == _peasants.end() ? nullptr : peasantSearch->second; in Monarch::_getPeasant ends up setting maybeThePeasant to null, which we then happily return (without cleaning up any MRU entries for it).
absolutely baseless list of possible commits to investigate:
- Add the ability to quit all terminal instances #11143
- Provide the focused tab title in the Tray Icon's context menu #11043
- Persist window layout on window close #10972
I did not have the tray icon enabled.
Filing on myself so I don't lose this, but I don't think I'm getting anything more out of this debugger
There's no valuable logs other than a million Monarch_Collect_WasDead(Id=3, Desktop={guid})
Activity
zadjii-msft commentedon Sep 13, 2021
Note to self: those repro steps did repro this on 1.12.2521, so this has got to be fairly new. This should be easy enough to write a test for.
Rosefield commentedon Sep 13, 2021
Curious if this is would be fixed in #11189. You of course collected the list of changes to the monarch I made, mostly around adding extra events on open/close of a peasant that can cause some churn. I didn't touch
_getMostRecentPeasantIdor _getPeasantso it might just be heap corruption from simultaneously removing peasants and iterating.Rosefield commentedon Sep 13, 2021
Looking at the code again, the issue might be that in SignalClose I remove from the peasants map, but don't remove from _mruPeasants? Happy to fix this either on its own, or in the above PR with the other changes.
https://github.com/microsoft/terminal/blob/main/src/cascadia/Remoting/Monarch.cpp#L148
zadjii-msft commentedon Sep 13, 2021
See, I knew I could file this, go to lunch, and come back to an answer 😝 Thanks for helping ID this. I'm okay combining this into #11189, so long as we make sure to add a test for this specific case. I just want to have as many tests on this stuff as possible.
Of course I type like a 2nd grader so while I was typing this, you filed the PR 😅. Doing it separately works too!
Make sure we fully clean up state when closing a peasant (#11217)