Opened 10 years ago

Closed 10 years ago

#3842 closed bug (fixed)

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

Reported by: timeless Owned by: jackburton
Priority: normal Milestone: R1
Component: Kits/Interface Kit Version: R1/pre-alpha1
Keywords: Cc:
Blocked By: Blocking:
Has a Patch: no Platform: x86

Description

steps:

  1. w/ bezillabrowser open (to the default page from the desktop)
  2. tap it in deskbar
  3. mouse over "Welcome to Haiku! - Bon Echo"
  4. 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 ?? ()

Change History (14)

comment:1 Changed 10 years ago by timeless

Platform: x64x86

comment:2 Changed 10 years ago by anevilyak

Component: User InterfaceKits/Interface Kit
Owner: changed from stippi to axeld

comment:3 Changed 10 years ago by anevilyak

Owner: changed from axeld to anevilyak
Status: newassigned

comment:4 Changed 10 years ago by 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.

comment:5 Changed 10 years ago by 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.

comment:6 Changed 10 years ago by anevilyak

Owner: changed from anevilyak to jackburton
Status: assignednew

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

comment:7 Changed 10 years ago by 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();
}

comment:8 in reply to:  7 Changed 10 years ago by 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.

comment:9 Changed 10 years ago by 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.

comment:10 Changed 10 years ago by 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().

comment:11 Changed 10 years ago by axeld

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

comment:12 Changed 10 years ago by 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...

comment:13 Changed 10 years ago by 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.

comment:14 Changed 10 years ago by jackburton

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.