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

[Daniel Stone]

Hi,

On 13 July 2015 at 11:26, Pekka Paalanen <ppaalanen at gmail.com> wrote:
> On Sun, 12 Jul 2015 10:52:32 +0300
> Giulio Camuffo <giuliocamuffo at gmail.com> wrote:
>> @@ -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.

Indeed. These are all good questions, but not necessarily possible to
resolve in bounded time, and this patch was an objective improvement
on the previous situation. I reviewed it pretty carefully, so pushed
with both mine & Jonas's review. Resolving those questions at some
stage would be good, however. :)

Cheers,
Daniel