[PATCH weston] input: Don't use uninitialized variables in default_grab_pointer_focus()

[Pekka Paalanen]

On Fri, 10 Apr 2015 11:13:22 -0500
Derek Foreman <derekf at osg.samsung.com> wrote:

> On 10/04/15 09:41 AM, Derek Foreman wrote:
> > On 10/04/15 02:16 AM, Pekka Paalanen wrote:
> >> On Wed,  8 Apr 2015 15:18:41 -0500
> >> Derek Foreman <derekf at osg.samsung.com> wrote:
> >>
> >>> If we have no pointer focus and weston_compositor_pick_view() returns NULL
> >>> default_grab_pointer_focus() will test the unset sx, sy output parameters
> >>> from weston_compositor_pick_view().
> >>>
> >>> Instead, assume that since both pointers are NULL focus hasn't changed and
> >>> we don't need to update pointer focus.
> >>>
> >>> Signed-off-by: Derek Foreman <derekf at osg.samsung.com>
> >>> ---
> >>>
> >>> This fixes an error reported by valgrind.  I think the patch is correct but
> >>> it does stop pointer->focus_signal from being emitted when focus switches
> >>> from NULL to NULL.  I think that shouldn't have happened anyway?
> >>
> >> If focus switches from NULL to NULL, weston_pointer_set_focus() would
> >> never be called because of the pointer->focus != view check, right?
> >>
> >> So the case you saw in Valgrind must be a case of !NULL -> NULL switch,
> >> which has to be processed but does not have valid sx, sy by definition.
> >> Therefore...
> >>
> >>> Initializing sx, sy to wl_fixed_from_int(0) would silence the warning too,
> >>> but I think this way is more correct...
> >>
> >> ...isn't initializing the sx, sy the right thing to do, and this patch
> >> actually fixes nothing?
> >>
> >> Did I miss something?
> > 
> > As did I.
> > 
> > As discussed on irc, the null to null case doesn't short circuit the ||
> > so the patch does "something"...

Ahaha, yeah, silly me, can't read logic operators right... sorry.

> > However, we probably shouldn't be getting here for a null to null switch
> > anyway, so I need to investigate why that's happened in the first place.
> > 
> > Initializing sx and sy would really have the same result, so I'm going
> > to avoid that for now as well.
> 
> On further investigation this happens in two places:
> From udev_input_init() which is called very early, so device_added()
> gets called before the view list contains anything at all.  This could
> be hidden by setting input->suspended for the duration of
> udev_input_init().  However, if the user is moving the mouse around
> during startup, it'll still happen.

Sounds like that special-casing is not worth it, then.

> During the first repaint when only the shell's fade surface exists.
> This can be hidden by giving the shell's fade surface an input region, I
> guess.  This'll also hide the mouse cursor during fade in, which is a
> little weird/wrong.

Yeah, not good.

> I think we need to deal with this case with either what I've done, or
> initializing sx, sy.  exposay appears to set a NULL focus with non-zero
> co-ordinates, so there may actually be a functional difference between
> these, though I'm having a hard time figuring out what it is, and if
> it's even intentional.

Right...

Let's take a step back. If weston_compositor_pick_view() returns NULL,
then there is no surface having input at that point. As there is no
surface, no value for sx,sy makes sense, because you don't even have a
coordinate system to begin with (a surface would define the coordinate
system). Therefore, sx,sy should not be used at all, they are invalid
in any case.

Comparing invalid sx,sy to previous sx,sy makes no sense either. It
just should not be done. Comparing previous sx,sy to new sx,sy only
makes sense if the coordinate systems are the same, that means, the
previous and current picked surface are the same and not NULL.

This brings us to what default_grab_pointer_focus() should do. Sx,sy
should be compared only if the previous and current surface are the
same non-NULL. In all other cases, the comparison is illegal.

If the picked surface is different than the previous one, then we
obviously need to call weston_pointer_set_focus(). If previous and
current are NULL, then it should just do nothing, because there are no
sx,sy and going from no-target to no-target is not a change.

This leaves the problem when previous surface was non-NULL, but the new
pick is NULL, because weston_pointer_set_focus() needs some sx,sy
arguments. In this case, I'd suggest we pick some arbitrary sx,sy that
is not easily confused with proper coordinates. Maybe something like
wl_fixed_from_int(-1000000).

If anything ends up actually computing with that value while the pick
is NULL, that's another bug to fix.

Would this make sense?

The strange thing here I do not understand is why would you need to
call weston_pointer_set_focus() if the view stays the same but only
coordinates change? Is that something that shouldn't happen? Is that
about bumping the input serial? It would cause leave/enter, and... ooh!

I know! We use an arbitrary leave/enter pair to signal pointer location
changes to clients when there is no physical input. That is, the
pointer device doesn't move, but the surface or view moves. We cannot
fake a wl_pointer.motion event, because it's not a user's physical
action. It happens when the surface/view moves under the pointer for any
reason.


Thanks,
pq

> >>>  src/input.c | 3 +++
> >>>  1 file changed, 3 insertions(+)
> >>>
> >>> diff --git a/src/input.c b/src/input.c
> >>> index 142c670..18d1262 100644
> >>> --- a/src/input.c
> >>> +++ b/src/input.c
> >>> @@ -157,6 +157,9 @@ default_grab_pointer_focus(struct weston_pointer_grab *grab)
> >>>  					   pointer->x, pointer->y,
> >>>  					   &sx, &sy);
> >>>  
> >>> +	if (!pointer->focus && !view)
> >>> +		return;
> >>> +
> >>>  	if (pointer->focus != view || pointer->sx != sx || pointer->sy != sy)
> >>>  		weston_pointer_set_focus(pointer, view, sx, sy);
> >>>  }