Pressing Tab in message window leads to stack-overflow

[Notabilis]

Starting a new game, opening the messages window and pressing the tab key leads to ASAN aborting the game:

AddressSanitizer:DEADLYSIGNAL
=================================================================
==15067==ERROR: AddressSanitizer: stack-overflow on address 0x7ffc017f9f98 (pc 0x563d5c63fab4 bp 0x7ffc017fa030 sp 0x7ffc017f9f80 T0)
    #0 0x563d5c63fab3 in UI::Table<void*>::handle_key(bool, SDL_Keysym) ../src/ui_basic/table.cc:336
    #1 0x563d5c942dbd in GameMessageMenu::handle_key(bool, SDL_Keysym) ../src/wui/game_message_menu.cc:421
[...]
   #248 0x563d5c63fcb0 in UI::Table<void*>::handle_key(bool, SDL_Keysym) ../src/ui_basic/table.cc:345
    #249 0x563d5c942dbd in GameMessageMenu::handle_key(bool, SDL_Keysym) ../src/wui/game_message_menu.cc:421

SUMMARY: AddressSanitizer: stack-overflow ../src/ui_basic/table.cc:336 in UI::Table<void*>::handle_key(bool, SDL_Keysym)
==15067==ABORTING

[Arty (artydent)]

From a quick peak at the code it looks like an endless key handling loop.

The game message window doesn't handle the tab key itself, so it refers handling down to its table:
GameMessageMenu::handle_key(bool, SDL_Keysym) ../src/wui/game_message_menu.cc:421

But tables specifically refer tab key handling up to their parents:
UI::Table<void*>::handle_key(bool, SDL_Keysym) ../src/ui_basic/table.cc:345

Solution: GameMessageMenu::handle_key(bool, SDL_Keysym) should simply handle the tab key with a default
  return UI::Panel::handle_key(down, code);

I'll check tomorrow if I find other UI instances with similar handling loops. Not sure if I can already manage to upload a fix, if not someone else can make this small change. (I'll report back here if I find similar instances.)

[GunChleoc (gunchleoc)]

Thanks for your analysis! A branch would be great, but an attachment to this bug will also be fine :)

[Arty (artydent)]

Just uploaded a branch to my repository. How do I connect this to this here? (Haven't worked with launchpad before.)

[Nasenbaer (nasenbaer)]

Sorry Arty saw only now that you were working on that bug too.
I checked all occurances and the message window seems to be the only one (seafaring statistic window seems to have the same bug at the first sight, but the table is not initialized with (this,...), so that seems to be okay.

I uploaded a branch for the bug fix. Feel free to merge if the fix is okay for you.

[Nasenbaer (nasenbaer)]

... or use the one of arty... after all he was the first one to answer the bug :).
Sorry for the confusion :-D

[Arty (artydent)]

Bzw, I didn't find any other UI elements with such a handling loop.

Interestingly, the seafaring statistics kinda looks and works similar, but the loop doesn't occur there, because the seafaring statistics uses additional boxes which seve as parents for the table and various buttons. Thus when the table passes tab key handling to its parent, it lands at such a box which then handles it.

Not sure what other advantages/disadvantages those two UI structure approaches have, but maybe we should make them have a similar structure.

[Nasenbaer (nasenbaer)]

@Arty use following option for bzr commit:

bzr commit -m "your message" --fixes lp:1791527
bzr push lp:...your branch

[Arty (artydent)]

@Nasenbaer It's fine. Doesn't matter to me what branch is used. And I see, we both checked the same thing with the seafaring statistics. :-)

[Nasenbaer (nasenbaer)]

Committed your fix in bzr rev. 8843 - after all you forwarded the key while my fix ignored it so your fix was better :)

[GunChleoc (gunchleoc)]

Thanks for the fix! :)

@Nasenbaer: your changes broke our build environment, and also broke it for everybody who merged trunk and delayed at least 1 other branch. So, Hessenfarmer cleaned up after you:

https://code.launchpad.net/~widelands-dev/widelands/fix-travis

Please use branches and merge requests for your changes, even if they seem trivial.

[GunChleoc (gunchleoc)]

Fixed in build20-rc1