[Bug] Crash on pressing a modifier key with focus in tri-state toggle in search dropdown

[S4qFBxkFFg]

Background

• Operating System: Linux (Ubuntu 21.04)
• CKAN Version: 1.30.4
• KSP Version: 1.12.2.3167
• Mono version: 6.8.0.105+dfsg-3

Problem

CKAN crashes when holding CTRL or ALT key
highlighting changes from blue to grey (not a problem, but may be relevant)

Steps to reproduce
Start CKAN
change view (e.g. to show all installed mods) WITHOUT typing in the main search box
hold CTRL key to try to select multiple mods, or try to ALT-Tab to another window

Expected behavior
Multiple mods can be selected? Not sure if this is possible normally.

Screenshots (if applicable)
https://cdn.imgchest.com/files/w7pjcbnk67p.png

CKAN error code (if applicable):

13641 [1] ERROR CKAN.ErrorDialog (null) - Unhandled exception:
System.InvalidCastException: Specified cast is not valid.
  at System.Windows.Forms.ToolStripMenuItem.ProcessCmdKey (System.Windows.Forms.Message& m, System.Windows.Forms.Keys keyData) [0x0001e] in <a3daa9b84fd241a497578a25f68bc3c7>:0 
  at System.Windows.Forms.ToolStripManager.ProcessCmdKey (System.Windows.Forms.Message& m, System.Windows.Forms.Keys keyData) [0x00028] in <a3daa9b84fd241a497578a25f68bc3c7>:0 
  at System.Windows.Forms.ContainerControl.ProcessCmdKey (System.Windows.Forms.Message& msg, System.Windows.Forms.Keys keyData) [0x00000] in <a3daa9b84fd241a497578a25f68bc3c7>:0 
  at System.Windows.Forms.Control.ProcessCmdKey (System.Windows.Forms.Message& msg, System.Windows.Forms.Keys keyData) [0x0002a] in <a3daa9b84fd241a497578a25f68bc3c7>:0 
  at System.Windows.Forms.Control.InternalPreProcessMessage (System.Windows.Forms.Message& msg) [0x00035] in <a3daa9b84fd241a497578a25f68bc3c7>:0 
  at System.Windows.Forms.Control.PreProcessMessage (System.Windows.Forms.Message& msg) [0x00000] in <a3daa9b84fd241a497578a25f68bc3c7>:0 
  at System.Windows.Forms.Control.PreProcessControlMessageInternal (System.Windows.Forms.Message& msg) [0x00062] in <a3daa9b84fd241a497578a25f68bc3c7>:0 
  at (wrapper remoting-invoke-with-check) System.Windows.Forms.Control.PreProcessControlMessageInternal(System.Windows.Forms.Message&)
  at System.Windows.Forms.Application.RunLoop (System.Boolean Modal, System.Windows.Forms.ApplicationContext context) [0x00288] in <a3daa9b84fd241a497578a25f68bc3c7>:0 
  at System.Windows.Forms.Application.Run (System.Windows.Forms.ApplicationContext context) [0x00014] in <a3daa9b84fd241a497578a25f68bc3c7>:0 
  at System.Windows.Forms.Application.Run (System.Windows.Forms.Form mainForm) [0x00006] in <a3daa9b84fd241a497578a25f68bc3c7>:0 
  at CKAN.Main..ctor (System.String[] cmdlineArgs, CKAN.GameInstanceManager mgr, System.Boolean showConsole) [0x002fd] in <933dc9d8b62a44ff9a8f866e3767654d>:0 
  at (wrapper remoting-invoke-with-check) CKAN.Main..ctor(string[],CKAN.GameInstanceManager,bool)
  at CKAN.GUI.Main_ (System.String[] args, CKAN.GameInstanceManager manager, System.Boolean showConsole) [0x0003b] in <933dc9d8b62a44ff9a8f866e3767654d>:0 
  at CKAN.CmdLine.MainClass.Gui (CKAN.GameInstanceManager manager, CKAN.CmdLine.GuiOptions options, System.String[] args) [0x00008] in <933dc9d8b62a44ff9a8f866e3767654d>:0 
  at CKAN.CmdLine.MainClass.RunSimpleAction (CKAN.CmdLine.Options cmdline, CKAN.CmdLine.CommonOptions options, System.String[] args, CKAN.IUser user, CKAN.GameInstanceManager manager) [0x002d1] in <933dc9d8b62a44ff9a8f866e3767654d>:0 
  at CKAN.CmdLine.MainClass.Execute (CKAN.GameInstanceManager manager, CKAN.CmdLine.CommonOptions opts, System.String[] args) [0x00251] in <933dc9d8b62a44ff9a8f866e3767654d>:0 
  at CKAN.CmdLine.MainClass.Main (System.String[] args) [0x00091] in <933dc9d8b62a44ff9a8f866e3767654d>:0

[HebaruSan]

This looks like a problem with your Mono installation, since there is no actual CKAN code in the stack trace other than the CKAN.Main constructor calling Application.Run.

What distribution do you have?
What version of Mono do you have?

[S4qFBxkFFg]

I'm on Ubuntu 21.04, Mono version is 6.8.0.105+dfsg-3

[HebaruSan]

Here's the function that throws the exception.

https://github.com/mono/mono/blob/d4a369b3e651ca92e27ae711c37177e1f42fa300/mcs/class/System.Windows.Forms/System.Windows.Forms/ToolStripMenuItem.cs#L402-L413

There's one cast:

			Form f = source == null ? null : (Form)source.TopLevelControl;

Which suggests the problem is that source.TopLevelControl isn't a Form. Which should be fine, since its proper type is Control:

https://docs.microsoft.com/en-us/dotnet/api/system.windows.forms.control.toplevelcontrol?view=netframework-4.7.2

So that part is a clear Mono bug, which we could try to help them fix. But it also means this should be reproducible in any Linux environment. I'm still in Windows currently and can't test that.

The most likely related CKAN code is the stuff we had to do to make the search dropdown work:

CKAN/GUI/Controls/EditModSearchDetails.cs

Lines 21 to 22 in d398f2e

	// Float over other controls
	SetTopLevel(true);

But there's no reason to change that since it isn't doing anything wrong.

[HebaruSan]

Possible additional Mono bug, it must not be respecting this part of the documentation to throw here:

If the control is not parented on a Form, this property will return null.

Somewhat confirmed, Mono's Control.TopLevelControl just walks the parent hierarchy and doesn't check whether the result is a Form:

https://github.com/mono/mono/blob/d4a369b3e651ca92e27ae711c37177e1f42fa300/mcs/class/System.Windows.Forms/System.Windows.Forms/Control.cs#L3209-L3220

[HebaruSan]

Might be easiest to go along with Mono's assumptions here and make EditModSearchDetails into a Form. No idea what the consequences of that would be, though.

[S4qFBxkFFg]

This may or may not be relevant, but I've just noticed the crash is associated with the highlighting behaviour of CKAN. If I start it, then click on a mod, it is highlighted blue, I can use CTRL to select multiple mods, and no crash occurs.
If I follow the steps to reproduce in the initial report, the highlighting is grey, not blue, and crashes occur (incidentally, using the ALT key will also cause a crash in this situation).
It looks like using one of the search options (e.g. typing in an individual search box like "Name:" or clicking one of the buttons like the ones next to "Compatible:") without typing in the main search box causing the highlighting to change from blue to grey.
In other words, performing a search without typing in the main search box, causes CKAN to enter a weird state where highlighting is different, and the reported crashes occur.

[HebaruSan]

I had been trying to reproduce this on Ubuntu unsuccessfully, but I finally have thanks to this part:

clicking one of the buttons like the ones next to "Compatible:"

(Typing in the text boxes doesn't seem to do it.)

So, the steps that reproduce it seem to be:

1. Open the search dropdown
2. Click one of the tri-state toggle buttons
3. Press Ctrl or Alt or Shift

[HebaruSan]

Adding some extra debug output to Mono, the ToolStripMenuItem that throws seems to be ExitToolButton, in the File menu! And if I remove that menu item for testing, the crash goes away! I can't think of any finding that could have surprised me more than that.

This menu item is almost identical to the many other items in our menus, except for the Ctrl+Q hotkey:

CKAN/GUI/Main/Main.Designer.cs

Line 220 in 2564dfb

this.ExitToolButton.ShortcutKeys = ((System.Windows.Forms.Keys)((System.Windows.Forms.Keys.Control | System.Windows.Forms.Keys.Q)));

And indeed, if I re-add the Exit item but remove the shortcut key, the crash goes away. I guess Mono is checking whether the key we pressed matches that shortcut key, so it can trigger it if needed?

[HebaruSan]

The ContainerControl appears to be SearchDetails, which is far less surprising. But it's not clear how we get from there to the Exit menu item.

That only appears in my debugging output if I remove the hotkey, so it happens after the crash in a separate code flow.

The Control is the button you click, and the ContainerControl is the tri state toggle. Somehow we jump from there to the Exit item:

CONTROL  (System.Windows.Forms.RadioButton)
CALLING PARENT 
CONTAINER CONTROL  (CKAN.TriStateToggle)
TOOL STRIP MANAGER
TOOL STRIP MENU ITEM ExitToolButton

Maybe ToolStripManager is a shared static/global object that all the controls use to report keystrokes?

[HebaruSan]

Important realization: The source.TopLevelControl check isn't looking at the Exit item, it's looking at the source of the Message m object! Which again would be either the radio button you clicked or the tri state toggle.

At a high level, ToolStripMenuItem.ProcessCmdKey is using that value to check whether it has the same top level control as the control that had focus when you pressed the key; if so, then it triggers itself, otherwise it doesn't. So only the hotkeys of menu items in your currently focused window are active.

[HebaruSan]

Submitted a fix to the Mono team in mono/mono#21285. Hopefully they like it, or give us a better idea.