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

[Marek Chalupa]

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

> Actually, why not just move "xorg_list_append(&xwl_output->link,
> &xwl_screen->output_list);" to xwl_output_create?
>

I'm not an expert on output, but I think that it is in output.done on
purpose. output.done was added to allow "atomic" updates of outputs,
so with xorg_list_append in output.done, the xwayland does not use the
output until it knows it has all the information about it.
So it should stay there IMO.


>
> I can't tell if there's a reason it's in xwl_output_done, or if it's just
> an oversight.
>
> On Thu, May 14, 2015 at 9:37 AM, 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--;
>>
>> (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/20150521/1d2a4529/attachment.html>