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

[Pekka Paalanen]

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

Hi,

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,
right?

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

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

Tricky.

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


Thanks,
pq