[PATCH weston v2] xwm: use always a valid 'primary view' for an X window

[Giulio Camuffo]

2015-09-23 19:46 GMT+03:00 Derek Foreman <derekf at osg.samsung.com>:
> On 01/02/15 08:18 AM, Giulio Camuffo wrote:
>> The xwm gets a primary view for a X window using the get_primary_view
>> vfunc of the shell_interface struct. Storing it is dangerous though because
>> it doesn't listen for its destruction so it may end up using the old stored
>> view pointer after that view was freed, or after the primary view for that
>> window was changed to another one.
>> Fetch the primary view just before using it every time and try to not
>> abuse this 'primary view' concept which may map badly to some shells:
>> iterate over all the views instead when it makes sense.
>> ---
>
> Good commit log, makes it obvious that we should be doing this...
>
>> v2: check window->shsurf
>>
>>  xwayland/window-manager.c | 39 +++++++++++++++++++++------------------
>>  1 file changed, 21 insertions(+), 18 deletions(-)
>>
>> diff --git a/xwayland/window-manager.c b/xwayland/window-manager.c
>> index 07c2ef3..be56ffc 100644
>> --- a/xwayland/window-manager.c
>> +++ b/xwayland/window-manager.c
>> @@ -124,7 +124,6 @@ struct weston_wm_window {
>>       uint32_t surface_id;
>>       struct weston_surface *surface;
>>       struct shell_surface *shsurf;
>> -     struct weston_view *view;
>>       struct wl_listener surface_destroy_listener;
>>       struct wl_event_source *repaint_source;
>>       struct wl_event_source *configure_source;
>> @@ -750,18 +749,27 @@ weston_wm_window_transform(struct wl_listener *listener, void *data)
>>       struct weston_wm_window *window = get_wm_window(surface);
>>       struct weston_wm *wm =
>>               container_of(listener, struct weston_wm, transform_listener);
>> +     struct weston_view *view;
>> +     struct weston_shell_interface *shell_interface =
>> +             &wm->server->compositor->shell_interface;
>>       uint32_t mask, values[2];
>>
>> -     if (!window || !wm)
>> +     if (!window || !wm || !window->shsurf)
>>               return;
>>
>> -     if (!window->view || !weston_view_is_mapped(window->view))
>> +     if (!shell_interface->get_primary_view)
>>
>>               return;
>>
>> -     if (window->x != window->view->geometry.x ||
>> -         window->y != window->view->geometry.y) {
>> -             values[0] = window->view->geometry.x;
>> -             values[1] = window->view->geometry.y;
>> +     view = shell_interface->get_primary_view(shell_interface->shell,
>> +                                              window->shsurf);
>> +
>> +     if (!view || !weston_view_is_mapped(view))
>> +             return;
>> +
>> +     if (window->x != view->geometry.x ||
>> +         window->y != view->geometry.y) {
>> +             values[0] = view->geometry.x;
>> +             values[1] = view->geometry.y;
>>               mask = XCB_CONFIG_WINDOW_X | XCB_CONFIG_WINDOW_Y;
>>
>>               xcb_configure_window(wm->conn, window->frame_id, mask, values);
>> @@ -972,7 +980,6 @@ weston_wm_handle_unmap_notify(struct weston_wm *wm, xcb_generic_event_t *event)
>>               wl_list_remove(&window->surface_destroy_listener.link);
>>       window->surface = NULL;
>>       window->shsurf = NULL;
>> -     window->view = NULL;
>>
>>       weston_wm_window_set_wm_state(window, ICCCM_WITHDRAWN_STATE);
>>       weston_wm_window_set_virtual_desktop(window, -1);
>> @@ -992,6 +999,7 @@ weston_wm_window_draw_decoration(void *data)
>>       struct weston_shell_interface *shell_interface =
>>               &wm->server->compositor->shell_interface;
>>       uint32_t flags = 0;
>> +     struct weston_view *view;
>>
>>       weston_wm_window_read_properties(window);
>>
>> @@ -1033,8 +1041,8 @@ weston_wm_window_draw_decoration(void *data)
>>                                                 window->width + 2,
>>                                                 window->height + 2);
>>               }
>> -             if (window->view)
>> -                     weston_view_geometry_dirty(window->view);
>> +             wl_list_for_each(view, &window->surface->views, surface_link)
>> +                     weston_view_geometry_dirty(view);
>
> Is this an unrelated functional change?  Previously we only dirtied the
> primary view, now we dirty all views?
>
>>
>>               pixman_region32_fini(&window->surface->pending.input);
>>
>> @@ -1060,6 +1068,7 @@ static void
>>  weston_wm_window_schedule_repaint(struct weston_wm_window *window)
>>  {
>>       struct weston_wm *wm = window->wm;
>> +     struct weston_view *view;
>>       int width, height;
>>
>>       if (window->frame_id == XCB_WINDOW_NONE) {
>> @@ -1072,8 +1081,8 @@ weston_wm_window_schedule_repaint(struct weston_wm_window *window)
>>                               pixman_region32_init_rect(&window->surface->pending.opaque, 0, 0,
>>                                                         width, height);
>>                       }
>> -                     if (window->view)
>> -                             weston_view_geometry_dirty(window->view);
>> +                     wl_list_for_each(view, &window->surface->views, surface_link)
>> +                             weston_view_geometry_dirty(view);
>
> Same as above...
>
> It seems like these two hunks are actually a separate bug fix?  Or is
> this because some shells might not have a get_primary_view?  Either way,
> it's a functional change from before?

That's the "and try to not abuse this 'primary view' concept which may
map badly to some shells: iterate over all the views instead when it
makes sense." part of the commit message.
I guess it may be a bug fix in some cases but the main reason was to
reduce the usage of the primary view. I can split it out though.


Thanks,
Giulio

>
> I'd like to see this as two patches, the one that's removing the view
> caching is already
> Reviewed-by: Derek Foreman <derekf at osg.samsung.com>
>
> (CC me and I'll review the other quickly, I just want to see a commit
> log and understand the change)
>
>>               }
>>               return;
>>       }
>> @@ -1430,7 +1439,6 @@ surface_destroy(struct wl_listener *listener, void *data)
>>        * Don't try to use it later. */
>>       window->shsurf = NULL;
>>       window->surface = NULL;
>> -     window->view = NULL;
>>  }
>>
>>  static void
>> @@ -2409,9 +2417,6 @@ xserver_map_shell_surface(struct weston_wm_window *window,
>>       if (!shell_interface->create_shell_surface)
>>               return;
>>
>> -     if (!shell_interface->get_primary_view)
>> -             return;
>> -
>>       if (window->surface->configure) {
>>               weston_log("warning, unexpected in %s: "
>>                          "surface's configure hook is already set.\n",
>> @@ -2423,8 +2428,6 @@ xserver_map_shell_surface(struct weston_wm_window *window,
>>               shell_interface->create_shell_surface(shell_interface->shell,
>>                                                     window->surface,
>>                                                     &shell_client);
>> -     window->view = shell_interface->get_primary_view(shell_interface->shell,
>> -                                                      window->shsurf);
>>
>>       if (window->name)
>>               shell_interface->set_title(window->shsurf, window->name);
>>
>