pressing enter in window list for app triggers Looper must be locked.

[timeless]

steps:

0. w/ bezillabrowser open (to the default page from the desktop)
1. tap it in deskbar
2. mouse over "Welcome to Haiku! - Bon Echo"
3. press enter

<boom>

[tcsetpgrp failed in terminal_inferior: Invalid Argument]
Thread 4188 called debugger(): Looper must be locked.
[tcsetpgrp failed in terminal_inferior: Invalid Argument]
[Switching to team /boot/system/Deskbar (81) thread w>ExpandoMenuBar cached menu
(gdb) bt
#0  0xffff0104 in ?? ()
#1  0x006d78b6 in debugger () from /boot/system/lib/libroot.so
#2  0x00312f9d in BLooper::check_lock () from /boot/system/lib/libbe.so
#3  0x003c7d60 in BView::_CheckOwnerLock () from /boot/system/lib/libbe.so
#4  0x003bd7a7 in BView::_ConvertToScreen () from /boot/system/lib/libbe.so
#5  0x003bd801 in BView::ConvertToScreen () from /boot/system/lib/libbe.so
#6  0x003bd965 in BView::ConvertToScreen () from /boot/system/lib/libbe.so
#7  0x003bd9b4 in BView::ConvertToScreen () from /boot/system/lib/libbe.so
#8  0x0023a682 in TWindowMenuItem::Invoke ()
#9  0x0036ab2b in BMenu::_InvokeItem () from /boot/system/lib/libbe.so
#10 0x00367350 in BMenu::KeyDown () from /boot/system/lib/libbe.so
#11 0x003cb8cf in BWindow::DispatchMessage () from /boot/system/lib/libbe.so
#12 0x00374087 in BPrivate::BMenuWindow::DispatchMessage ()
   from /boot/system/lib/libbe.so
#13 0x003cfe0c in BWindow::task_looper () from /boot/system/lib/libbe.so
#14 0x003126ef in BLooper::_task0_ () from /boot/system/lib/libbe.so
#15 0x006dbd78 in thread_entry () from /boot/system/lib/libroot.so
#16 0x78143fec in ?? ()

[anevilyak]

Quite puzzling...what happens here is Deskbar tries to do a ConvertToScreen invocation on a parent menu (it selects the parent if it's not in Expando mode). However, when invoked via the keyboard, the parent menu winds up not being locked, while when invoking via the mouse it does...have not yet figured out why.

[anevilyak]

On a related note though, the mouse positioning is irrelevant here. Picking anything from the team submenu via the keyboard suffices to reproduce this bug, regardless of mouse location.

[anevilyak]

Don't suppose you have any idea on this one Stefano?

[jackburton]

Well, from a first look, when clicking on the item with the mouse, the code path which triggers the call to BMenuItem::Invoke() is inside MenuBar.cpp (lines 642 - 650):

	if (window->Lock()) {
		if (fSelected != NULL)
			_SelectItem(NULL);

		if (fChosenItem != NULL)
			fChosenItem->Invoke();
		_RestoreFocus();
		window->Unlock();
	}

while when using the keyboard, the code path is completely different, and doesn't lock the looper

BMenu::_InvokeItem(BMenuItem *item, bool now)
{
	if (!item->IsEnabled())
		return;

	// Do the "selected" animation
	// TODO: Doesn't work. This is supposed to highlight
	// and dehighlight the item, works on beos but not on haiku.
	if (!item->Submenu() && LockLooper()) {
		snooze(50000);
		item->Select(true);
		Sync();
		snooze(50000);
		item->Select(false);
		Sync();
		snooze(50000);
		item->Select(true);
		Sync();
		snooze(50000);
		item->Select(false);
		Sync();
		UnlockLooper();
	}

	item->Invoke();
}

[anevilyak]

Replying to jackburton:

Well, from a first look, when clicking on the item with the mouse, the code path which triggers the call to BMenuItem::Invoke() is inside MenuBar.cpp (lines 642 - 650): while when using the keyboard, the code path is completely different, and doesn't lock the looper

That's to be expected though, BView::KeyDown is called with the looper locked implicitly. The problem here though is that deskbar's WindowMenuItem is calling an operation on the *parent* menu's window, not the window it's sitting in. In the mouse case the parent also appears to be locked, while in the keydown case it isn't, which causes the debugger() call to be triggered. What I can't figure out is what's locking the parent window in the mouse case.

[jackburton]

But it's still caused by the different code path, see: In the first case, the BMenuBar is the one which calls InvokeItem(), and in that case, it locks the BMenuBar's window, not the BMenu window. In the latter case, InvokeItem() is called by BMenu itself, and the BMenuBar window is not locked, since BMenuBar::_Track() explicitly unlocks the BMenuBar window before calling _track() on the submenu. I guess we should change something in BMenu::Keydown() or in BMenu::InvokeItem(). I'll try to have a look, but we best would have to look at how beos handled this.

[anevilyak]

The Deskbar code in question worked in BeOS, but yeah, I'm not sure either. It seems as if we'd need to guarantee that the whole menu chain is locked when calling Invoke().

[axeld]

AFAIK BeOS R5 had the same problem, it might have been fixed in Dano, though.

[jackburton]

I fixed the problem in hrev30662 by locking the root menu's looper before Invoke(). Somehow, though, I don't feel it's so nice, since it could introduce deadlocks in some cases...

[anevilyak]

Yeah, doesn't seem ideal but then, the design of BMenu's API kinda sucks as a whole :/ Maybe some of this would be easier to handle by reviving/finishing that patch to make BMenu entirely mouse/keyboard event driven instead of the polling loop? Since that still needs to be done anyways in order to make keyboard navigation workable.