[9front] focus bug in libdraw program

[9front] focus bug in libdraw program

[telephil9]

Hi,

I have noticed a strange behaviour in a libdraw program where mouse events are wrongfully retriggered when the window focus is "stolen" by another window because of an event.
To illustrate this, here is a test case (full code below):
- A window has a clickable area where a right-click plumbs a mail message. This opens a nedmail window so focus is no more on the main window.
- Hide our window through winwatch or the rio menu, you'll see the nedmail window open once again
- Unhide our window, nedmail window for the third time

This 2 last steps highlight the bug where our main window still receives event and the Mouse struct read from the Mousectl chan has coordinates inside our window whereas we're not there anymore.

I'll try to dig into this but I'm not quite sure, at this stage, where the issue might be. Any kind of help/hints would be appreciated.

--phil


#include <u.h>
#include <libc.h>
#include <draw.h>
#include <mouse.h>
#include <keyboard.h>
#include <thread.h>
#include <plumb.h>

enum
{
	Emouse,
	Eresize,
	Ekeyboard
};

Rectangle clickr;

void
redraw(void)
{
	clickr = insetrect(screen->r, 20);
	draw(screen, screen->r, display->white, nil, ZP);
	border(screen, clickr, 1, display->black, ZP);
	flushimage(display, 1);
}
	
void
plumbmsg(void)
{
	int fd;

	fd = plumbopen("send", OWRITE|OCEXEC);
	if(fd<0)
		return;
	plumbsendtext(fd, "test", nil, nil, "/mail/fs/mbox/1");
	close(fd);
}

void
threadmain(int argc, char *argv[])
{
	USED(argc);
	USED(argv);
	Mousectl *mctl;
	Keyboardctl *kctl;
	Mouse m;
	Rune k;
	Alt alts[] = {
		{ nil, &m,  CHANRCV },
		{ nil, nil, CHANRCV },
		{ nil, &k,  CHANRCV },
		{ nil, nil, CHANEND },
	};

	if(initdraw(nil, nil, "test")<0)
		sysfatal("initdraw: %r");
	if((mctl = initmouse(nil, screen))==nil)
		sysfatal("initmouse: %r");
	if((kctl = initkeyboard(nil))==nil)
		sysfatal("initkeyboard: %r");
	alts[Emouse].c = mctl->c;
	alts[Eresize].c = mctl->resizec;
	alts[Ekeyboard].c = kctl->c;
	redraw();
	for(;;){
		switch(alt(alts)){
		case Emouse:
			if(ptinrect(m.xy, clickr)){
				if(m.buttons&4)
					plumbmsg();
			}
			break;
		case Eresize:
			if(getwindow(display, Refnone)<0)
				sysfatal("getwindow: %r");
			redraw();
			break;
		case Ekeyboard:
			if(k=='q' || k==Kdel)
				threadexitsall(nil);
			break;
		}
	}
}

Re: [9front] focus bug in libdraw program

[istvan bak]

In case nobody touched this yet, here's my take on it.

two symptom notes:
1. if you print the whole Mouse struct you receive, when reproducing
the bug the timestamps are always the same. In fact the whole struct
value is always the same.
2. if you run the program in a subrio, and summon the focus-grabbing
window, and then don't give the focus back to the program, then
resizing the subrio also reproduces the bug. You don't need the
Hide/Unhide for it.

relevant routines:
/sys/src/libdraw/mouse.c:/^_ioproc/
/sys/src/cmd/rio/xfid.c:/^xfidread/
/sys/src/cmd/rio/wind.c:/^winctl/
/sys/src/cmd/rio/wind.c:/^wresize/

phil's program gets a resize from mctl->resizec, then a mouse message
from mctl->c. This is because _ioproc() reads from /dev/mouse a single
'r' message, which it splits into a send(mc->resizec), then a
send(mc->c). This is correct, I believe 'r' may hold a valid mouse
event (can't confirm/deny, haven't read all of rio, yet). xfidread()
checks w->resized and rewrites 'm' to 'r'. It gets the mouse message
from winctl(), which either gets it from the queue (<- only clicks go
here I think), or copies "the current mouse state" (<- mouse motion is
reported such, this is not a rare code path). Hide/Unhide is a resize
(see routines whide, wsendctlmesg, wctlmesg), wresize() sets
w->resized, and increments w->mouse.counter. The counter is a serial
number for mouse events, which winctl() looks at for changes, so
wresize() faked a mouse event to be delived in a mouse message.

the bug:
usually, the wresize() counter++ creates (in winctl()'s WMouseread
case) a mouse message holding the "current mouse state", and it's
usually to the effect of "i moved from current position to current
position at the exact same timestamp", which I guess is a no-op for
most client programs. The "current" state is the last state as when
the window had focus (or whatever). With the above program though, the
last state is "right click into the rectangle". So the resize
generates a bogus mouse event with the Mouse.buttons field set.

weakly proposed fix:
make the 'r' message be not a resize+mouse message, but just a resize
message with zeroed-out fields after it. libdraw (and some other
programs) will have to be tweaked, to not fall through from 'r' to
'm', but to break out instead. Thus, a resize won't generate a bogus
mouse event, be it no-op or otherwise. I do not yet see what problems
this may cause.

Programs that open /dev/mouse would have to be tweaked, I don't know
how many, a quick grep reveals a dozen. I considered making it an 'R'
message and keeping the old 'r' as well, but I think that causes bugs
and confusion. Also, the fix makes the file interface subtly different
from the bell labs version ☹. Arguably that deserves a bugfix too.

Someone who knows rio source better should chime in, please. If not, I
will return in a few weeks after having consumed it.

Example patch, feel free to make it less clumsy (and note, gmail will
fucking mangle the long lines):

diff -r e43de7ad21b2 sys/src/cmd/rio/dat.h
--- a/sys/src/cmd/rio/dat.h	Thu Apr 22 08:40:50 2021 +0200
+++ b/sys/src/cmd/rio/dat.h	Thu Apr 22 16:34:15 2021 +0200
@@ -154,6 +154,7 @@
 	 * Now they're always the same but the code doesn't assume so.
 	*/
 	Rectangle		screenr;	/* screen coordinates of window */
+	int			sendresized;
 	int			resized;
 	int			wctlready;
 	Rectangle		lastsr;
diff -r e43de7ad21b2 sys/src/cmd/rio/wind.c
--- a/sys/src/cmd/rio/wind.c	Thu Apr 22 08:40:50 2021 +0200
+++ b/sys/src/cmd/rio/wind.c	Thu Apr 22 16:34:15 2021 +0200
@@ -376,9 +376,8 @@
 	flushimage(display, 1);
 	wsetname(w);
 	w->topped = ++topped;
-	w->resized = TRUE;
+	w->sendresized = TRUE;
 	w->winnameread = FALSE;
-	w->mouse.counter++;
 	w->wctlready = 1;
 }

@@ -1549,7 +1548,7 @@
 		} else {
 			alts[WKbdread].op = (w->kbdopen && kbdqw != kbdqr) ?
 				CHANSND : CHANNOP;
-			alts[WMouseread].op = (w->mouseopen && w->mouse.counter !=
w->mouse.lastcounter) ?
+			alts[WMouseread].op = (w->mouseopen && w->mouse.counter !=
w->mouse.lastcounter || w->sendresized) ?
 				CHANSND : CHANNOP;
 			alts[WCwrite].op = w->scrolling || w->mouseopen || (w->qh <=
w->org+w->nchars) ?
 				CHANSND : CHANNOP;
@@ -1625,6 +1624,14 @@
 				wmousectl(w);
 			break;
 		case WMouseread:
+			if(w->sendresized){
+				w->sendresized = 0;
+				w->resized = 1;
+				memset(&m, 0, sizeof(Mousestate));
+				send(mrm.cm, &m.Mouse);
+				continue;
+			}
+
 			/* send a queued event or, if the queue is empty, the current state */
 			/* if the queue has filled, we discard all the events it contained. */
 			/* the intent is to discard frantic clicking by the user during
long latencies. */
diff -r e43de7ad21b2 sys/src/libdraw/mouse.c
--- a/sys/src/libdraw/mouse.c	Thu Apr 22 08:40:50 2021 +0200
+++ b/sys/src/libdraw/mouse.c	Thu Apr 22 16:34:15 2021 +0200
@@ -68,7 +68,7 @@
 			one = 1;
 			if(nbsend(mc->resizec, &one) < 0)
 				continue;
-			/* fall through */
+			break;
 		case 'm':
 			m.xy.x = atoi(buf+1+0*12);
 			m.xy.y = atoi(buf+1+1*12);

Re: [9front] focus bug in libdraw program

[cinap_lenrek]

Interesting. What about we just force the buttons to 0
when a window looses its focus?

It makes sense to me as theres no way to get the button up
when the window is not in focus.

We also probably want to set wctlready to produce a event
in the wctl file in this case.

diff -r 9c76e0d472ba sys/src/cmd/rio/wind.c
--- a/sys/src/cmd/rio/wind.c	Wed Apr 21 16:58:27 2021 +0200
+++ b/sys/src/cmd/rio/wind.c	Sat Apr 24 13:13:59 2021 +0200
@@ -1380,6 +1380,10 @@
 			Channel *c = p;
 			input = recvp(c);
 			sendp(c, w);
+
+			w->mc.buttons = 0;
+			w->mouse.counter++;
+			w->wctlready = 1;
 		}
 		if(w->i==nil || Dx(w->screenr)<=0)
 			break;

--
cinap

Re: [9front] focus bug in libdraw program

[istvan bak]

That works nicely, thanks. I attempted to trip it up with something like:
	sleep 5; echo hide > /dev/wsys/2/wctl; echo unhide > /dev/wsys/2/wctl
while pressing the button down, but your committed fix caught that too ☺.

Re: [9front] focus bug in libdraw program

[telephil9]

Thanks istvan for the analysis and cinap for the fix.