[PATCH weston v2] compositor: make sure to reset views' pointers to destroyed output

Pekka Paalanen ppaalanen at gmail.com
Mon Jul 13 03:26:48 PDT 2015

On Sun, 12 Jul 2015 10:52:32 +0300
Giulio Camuffo <giuliocamuffo at gmail.com> wrote:

> When an output is being destroyed reassign the output of the views
> that were in it, to be sure not to keep a dangling pointer which could
> be used later on by calling weston_surface_assign_output() on the
> view's surface.
> Also make sure we send wl_surface.leave events to the surfaces that
> were in that output.
> ---
> I had forgotten about this one...
> changes since v1:
> - reassign the output for all relevant views
> - send wl_surface.leave


this looks like a nice patch, but there are some questions below.

>  src/compositor.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> diff --git a/src/compositor.c b/src/compositor.c
> index 150d1fb..2a117d8 100644
> --- a/src/compositor.c
> +++ b/src/compositor.c
> @@ -1079,6 +1079,9 @@ weston_view_assign_output(struct weston_view *ev)
>  	mask = 0;
>  	pixman_region32_init(&region);
>  	wl_list_for_each(output, &ec->output_list, link) {
> +		if (output->destroying)
> +			continue;
> +
>  		pixman_region32_intersect(&region, &ev->transform.boundingbox,
>  					  &output->region);
> @@ -3888,9 +3891,15 @@ WL_EXPORT void
>  weston_output_destroy(struct weston_output *output)
>  {
>  	struct wl_resource *resource;
> +	struct weston_view *view;
>  	output->destroying = 1;
> +	wl_list_for_each(view, &output->compositor->view_list, link) {
> +		if (view->output_mask & (1 << output->id))
> +			weston_view_assign_output(view);
> +	}

This now happens before the shell has a chance to react to the output
removed signal and move windows to remaining outputs, does it not?

The least, this means that clients will get first leave/enter events
from this, and another set from the shell's move. The first round
might even leave the view and the surface completely without an output,

The shell calls weston_view_geometry_dirty() when it does the move.
This leads to weston_view_update_transform() later, likely as part of
the next repaint.

weston_{view,surface}_assign_output() uses the derived state, which is
updated by weston_view_update_transform(). This means that simply
moving the weston_view_assign_output() calls in weston_output_destroy()
after the signal emissions does not really help, because it will be
using outdated state.

Did you consider all this? Do you see the double-events?

I tried to see if there was any harm from having the output assignment
be NULL temporarily, but it didn't seem like
weston_compositor_build_view_list() machinery would break because of
it. Not for non-subsurface views, anyway...

Considering outdated state, I suppose weston_output_destroy() may
happen at any time... particularly while view->transform.dirty is true.
Maybe the call should be weston_view_update_transform() instead? If
transform.dirty, then also the output assignments are out-of-date, I

I'm not sure what the best course of action is.

Hmm, I wonder if we should sprinkle assert(!view->transform.dirty)
around things that use weston_view::transform fields...

Sprinkling weston_view_update_transform() around is not the right way,
because then we may end up sending several leave/enter events to
clients, which again may respond by redrawing the wl_surface (if e.g.
the applicable output_scale changes).


> +
>  	wl_event_source_remove(output->repaint_timer);
>  	weston_presentation_feedback_discard_list(&output->feedback_list);


More information about the wayland-devel mailing list