Stacking two Chesto Screens - specifically in HBAS - causes an infinite CPU-eating loop, freezing HBAS

[compucat]

Describe the bug
Title. (original title: When attempting to leave feedback, HBAS freezes.)

Sometimes after waiting a while, the keyboard will disappear. I suspect this is somewhere in Chesto - spotted a similar issue that called SDL_SetCursor a ridiculous number of times per frame.

Platform: Linux, uncertain of others

To Reproduce

Launch HBAS; from the app list, select HBAS itself, then click on "Report Issue"

Per git-bisect, from oldest to newest, the first bad commit could be any of:

• 5833ed2
• 1b56523
• 0ee4429
• 9900b89
• 65260a4
• 464b329

EDIT: after further testing, the first ones are all red herrings due to a missing return. 464b329 is the first commit with the issue proper.

Some of these commits will instead crash HBAS entirely when attempting to provide feedback. IIRC the last commit is the one that actually freezes, rather than crashing?

[vgmoose]

When the on-screen keyboard is shown, physical keyboard key events start being processed which can create kind of strange input handling behavior sometimes. Also hitting ALT toggles the on-screen keyboard on/off (a feature from vgedit, to allow fullscreen physical keyboard editing).

When it's frozen, does it still accept keyboard inputs, or it's frozen-frozen? Submitting is also a synchronous call to the feedback server where the app hangs until hearing a response, but that should be unrelated.

[compucat]

Frozen-frozen; it pins one of my CPU threads at 100%. Chucking it at gdb now...

[compucat]

Hm. This appears to be an issue with Chesto's Screen system - it's only triggered when launching one subscreen from another subscreen. Still trying to figure out why. (edit: a trivial modification of ChestoTesto that launches nested subscreens does not reproduce the issue...)

For instance: going from AppList to SettingsScreen to AboutScreen fails. However, if you hack AppList::launchSettings() to go to AboutScreen directly, it works fine.

Incidentally, the app does appear to start responding again - for a couple frames at a time - if you let it chill and eat CPU long enough.

[compucat]

Bah. This is a very subtle bug - I'm not convinced I have the chain of cause perfectly down, or a proper fix in mind. But I was able to monkey-patch around the problem enough to somewhat understand it.

As I understand it: every frame, MainDisplay runs RootDisplay::mainLoop(). If any input events are triggered, MainDisplay.process() is called, and one of two paths occur:

• Ordinarily (if not in a subscreen), .process() runs a bunch of checks.
  • These are culminated by if (needsRedraw) appList->update(); - which is a very compute-expensive method.
  • Afterwards, control is passed to the superclass method RootDisplay::process() - in this case, that also passes control up to Element::process() to work its way down its child elements.
  • Key to note: Element::process() always sets this->needsRedraw = false;. To be explicit, in this case, that is MainDisplay.needsRedraw - which prevents appList->update() from being called again until explicitly requested.
• When in a subscreen, the process is different.
  • The bulk of MainDisplay.process() is skipped - except for if (needsRedraw) appList->update(); - then control passes to the superclass.
  • Because there are subscreen(s) on the stack, RootDisplay::process() passes control to the topmost screen's process() method. However, this is no longer MainDisplay thus, MainDisplay.needsRedraw is never set to false - i.e. if it was previously true, applist->update() will be infinitely called.

This is half of the puzzle, but the obvious follow-up remains: how is that flag getting stuck as true? Note what happens when a subscreen is requested - in HBAS, this only ever happens as the result of an input event. Thus:

• Ordinarily (if not in a subscreen), MainDisplay.process() eventually works its way down to RootDisplay::process() - which itself works its way down the element tree, and somewhere RootDisplay::pushScreen() is called. That explicitly sets MainDisplay->needsRedraw = true.
  • However, once control is handed back and falls down to Element::process() (with this = MainDisplay), this->needsRedraw = false occurs. Thus, nothing happens.
• If already in a subscreen, control passes MainDisplay.process() -> RootDisplay::process() - which then skips to the subscreen's .process(), resetting this to the subscreen - before working down the element chain.
  • RootDisplay::pushScreen() is eventually called, so MainDisplay->needsRedraw is set to true.
  • However, since this is now the subscreen, Element::process() does not clear MainDisplay->needsRedraw - triggering the bug.

(Misc note: MainDisplay creates the AppList directly - AppListScreen appears to be either a draft or a vestige? Minor red herring.)

[compucat]

Above commit gets this unstuck, but leaving issue open pending internal discussion on how best to "properly" solve it. Possibly requires Chesto changes?

[vgmoose]

I'm confident enough in the fix to consider it not to be hacky: AppList's update is too intense of an operation to be invoked by the main redraw flag in the first place. A long term fix may be to prevent needsRedraw from being manually read-- with the assumption that the implementor (HBAS in this case) shouldn't be hijacking calls for a re-render with their own logic. Something like a one way public markNeedsRedraw() could do it.

We also discussed on Discord that a contributing factor to this bug was allowing RootDisplay to be subclassed full stop, which lets the implementing app subvert any normal subscreen stack logic. If RootDisplay were unsubclassable, we could provide other hooks for first time initialization logic, and leave the UI parts to "normal" Chesto stacks/Screens.

To be more specific, the layout in HBAS's MainDisplay could better fit a MainScreen kind of model, which then contains the other components (like AppList, Sidebar, etc).