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

[Dima Ryazanov]

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--;

(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.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.freedesktop.org/archives/wayland-devel/attachments/20150514/507a6ec2/attachment.html>