[PATCH xwayland] xwayland: do not add output into output_list multiple times

[Marek Chalupa]

Hi,

On Thu, May 14, 2015 at 6:37 PM, Dima Ryazanov <dima at gmail.com> wrote:

> Oh wow, I was playing around with outputs, and never realized output_handle_done
> was being called after any geometry change, not just after the output was
> created.
>
> On Thu, May 14, 2015 at 2:58 AM, Marek Chalupa <mchqwerty at gmail.com>
> wrote:
>
>> output.done event can be sent even on some property change, not only
>> when announcing the output. Therefore we must check if we already have it
>> otherwise we may corrupt the list by adding it multiple times.
>>
>> This fixes bug when xwayland looped indefinitely in output.done handler
>> and that can be reproduced following these steps (under X without
>> multi-monitor setup):
>>  1) run weston --output-count=2
>>  2) run xterm, move it so that half is on one output
>>     and half on the other
>>  3) close second output, try run weston-terminal
>>
>
> (I can only repro it after closing the first output, not the second one;
> is that what you meant?)
>
>
>> weston sends updated outputs which trigger this bug.
>>
>> Signed-off-by: Marek Chalupa <mchqwerty at gmail.com>
>> ---
>>  hw/xwayland/xwayland-output.c | 25 +++++++++++++++++++------
>>  1 file changed, 19 insertions(+), 6 deletions(-)
>>
>> diff --git a/hw/xwayland/xwayland-output.c b/hw/xwayland/xwayland-output.c
>> index 155cbc1..0c96e87 100644
>> --- a/hw/xwayland/xwayland-output.c
>> +++ b/hw/xwayland/xwayland-output.c
>> @@ -116,15 +116,28 @@ output_handle_mode(void *data, struct wl_output
>> *wl_output, uint32_t flags,
>>  static void
>>  output_handle_done(void *data, struct wl_output *wl_output)
>>  {
>> -    struct xwl_output *xwl_output = data;
>> +    struct xwl_output *it, *xwl_output = data;
>>      struct xwl_screen *xwl_screen = xwl_output->xwl_screen;
>> -    int width, height;
>> +    int width = 0, height = 0, has_this_output = 0;
>> +
>> +    xorg_list_for_each_entry(it, &xwl_screen->output_list, link) {
>> +        /* output done event is sent even when some property
>> +         * of output is changed. That means that we may already
>> +         * have this output. If it is true, we must not add it
>> +         * into the output_list otherwise we'll corrupt it */
>> +        if (it == xwl_output)
>> +            has_this_output = 1;
>> +
>> +        if (width < it->x + it->width)
>> +            width = it->x + it->width;
>> +        if (height < it->y + it->height)
>> +            height = it->y + it->height;
>> +    }
>>
>> -    xorg_list_append(&xwl_output->link, &xwl_screen->output_list);
>> +    if (!has_this_output) {
>> +        xorg_list_append(&xwl_output->link, &xwl_screen->output_list);
>>
>
> I think this line should also be moved here:
>     xwl_screen->expecting_event--;
>

Yeah, you're right, the expecting_event is for new outputs only.



> (It might not matter since it's only used by xwl_screen_init - but the
> code seems to assume it would only be decremented once after the output is
> created.)
>
> -    width = 0;
>> -    height = 0;
>> -    xorg_list_for_each_entry(xwl_output, &xwl_screen->output_list, link)
>> {
>> +        /* we did not check this output for new screen size, do it now */
>>          if (width < xwl_output->x + xwl_output->width)
>>              width = xwl_output->x + xwl_output->width;
>>          if (height < xwl_output->y + xwl_output->height)
>> --
>> 2.1.0
>>
>> _______________________________________________
>> wayland-devel mailing list
>> wayland-devel at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/wayland-devel
>>
>
> Just a thought... You could instead:
> - check if the output already exists
> - add it if necessary
> - update the width and height
>
> That way, the width/height calculation code won't be duplicated. (Though
> it will require iterating over output_list twice.) Anyways, it's up to you.
>

Yes, I didn't want to iterate over the list twice, though it wouldn't bring
up much overhead, since usually there are like 1 or 2 items in the list.
Can fix it if majority likes two iteration more :)

Thanks,
Marek
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150521/05f8c3c6/attachment.html>