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

[Derek Foreman]

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"...
> 
> 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.

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.


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.


>>>  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);
>>>  }
>>
>>
>> Thanks,
>> pq
>>
> 
> _______________________________________________
> wayland-devel mailing list
> wayland-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>