<div dir="ltr">Hi Pekka,<div> </div><div>Thanks for the explanation. You're probably right; this shouldn't be possible.</div><div> </div><div>I haven't been able to reproduce the bug again. The one time I got it was when I was working on my show_window_menu patches - so it's quite possible that something there was causing it. (In fact, the window menu involves two different clients - the owner of the window and the desktop shell - so maybe that was the problem.)</div><div> </div><div>Anyways, let's ignore this patch unless I figure out what actually happened there.</div></div><div class="gmail_extra"> <div class="gmail_quote">On Mon, Dec 5, 2016 at 3:12 AM, Pekka Paalanen <<a href="mailto:ppaalanen@gmail.com" target="_blank">ppaalanen@gmail.com</a>> wrote: <blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On Sat, 3 Dec 2016 21:21:44 -0800 Dima Ryazanov <<a href="mailto:dima@gmail.com">dima@gmail.com</a>> wrote: > This should fix some (fairly rare) crashes where the client goes away at exactly > the wrong moment, e.g.: > - Weston sends a message to the client > - Message causes the client to crash for whatever reason > - Weston keeps processing the remaining requests from the now dead client Hi, I'm curious. Even if the client crashes, this should happen: 1. the server processes queued requests while it is not aware of the client going away 2. libwayland-server notices the client has gone away, and calls destructors on all objects During step 1, the client being already dead does not affect anything because the disconnection has not been processed, so any user data still remains valid. Once the destruction begins, no requests should dispatch anymore. If some requests are still being dispatched, that would be a bug in libwayland-server. I'd really like to know how it is possible to have these functions called with invalid user data. I fear that this patch might just paper over some real bugs. Resources do not just go away, especially those that represent objects created by the client. I can understand that wl_output user data can be NULL if the weston_output has been destroyed, but not with e.g. wl_surface since the surface is created by the client, so it cannot go away without the client destroying it or disconnecting. Could there perhaps be some object destruction sequence that e.g. libweston-desktop does not handle? Is that what is going on here? Does the protocol define a destruction order, should we be sending protocol errors somewhere? > (I've only seen it happen once, while stepping through Weston's code in gdb, but > seems like the right thing to fix.) Stepping in gdb only makes Weston extremely slow to execute, so it can expose races or trigger "unresponsive client" etc. timeouts, but it does not affect steps 1 and 2 mentioned above. So if there is a race, we need to fix the race instead. Hence, I'm not yet convinced this patch is the right thing to do. On the contrary, without any other explanation I would expect the checks you added to be asserts instead of silently ignoring the unexpected. Thanks, pq <div class="HOEnZb"><div class="h5"> > Signed-off-by: Dima Ryazanov <<a href="mailto:dima@gmail.com">dima@gmail.com</a>> > --- > libweston-desktop/xdg-shell-v6.c | 141 ++++++++++++++++++++++++++++----------- > 1 file changed, 101 insertions(+), 40 deletions(-) > > diff --git a/libweston-desktop/xdg-shell-v6.c b/libweston-desktop/xdg-shell-v6.c > index 7d0bd8e..30f6d82 100644 > --- a/libweston-desktop/xdg-shell-v6.c > +++ b/libweston-desktop/xdg-shell-v6.c > @@ -302,10 +302,14 @@ weston_desktop_xdg_toplevel_protocol_set_parent(struct wl_client *wl_client, > { > struct weston_desktop_surface *dsurface = > wl_resource_get_user_data(resource); > - struct weston_desktop_xdg_toplevel *toplevel = > - weston_desktop_surface_get_implementation_data(dsurface); > + struct weston_desktop_xdg_toplevel *toplevel; > struct weston_desktop_surface *parent = NULL; > > + if (!dsurface) > + return; > + > + toplevel = weston_desktop_surface_get_implementation_data(dsurface); > + > if (parent_resource != NULL) > parent = wl_resource_get_user_data(parent_resource); > > @@ -346,8 +350,12 @@ weston_desktop_xdg_toplevel_protocol_show_window_menu(struct wl_client *wl_clien > wl_resource_get_user_data(resource); > struct weston_seat *seat = > wl_resource_get_user_data(seat_resource); > - struct weston_desktop_xdg_toplevel *toplevel = > - weston_desktop_surface_get_implementation_data(dsurface); > + struct weston_desktop_xdg_toplevel *toplevel; > + > + if (!dsurface) > + return; > + > + toplevel = weston_desktop_surface_get_implementation_data(dsurface); > > if (!toplevel->base.configured) { > wl_resource_post_error(toplevel->resource, > @@ -370,8 +378,12 @@ weston_desktop_xdg_toplevel_protocol_move(struct wl_client *wl_client, > wl_resource_get_user_data(resource); > struct weston_seat *seat = > wl_resource_get_user_data(seat_resource); > - struct weston_desktop_xdg_toplevel *toplevel = > - weston_desktop_surface_get_implementation_data(dsurface); > + struct weston_desktop_xdg_toplevel *toplevel; > + > + if (!dsurface) > + return; > + > + toplevel = weston_desktop_surface_get_implementation_data(dsurface); > > if (!toplevel->base.configured) { > wl_resource_post_error(toplevel->resource, > @@ -394,11 +406,15 @@ weston_desktop_xdg_toplevel_protocol_resize(struct wl_client *wl_client, > wl_resource_get_user_data(resource); > struct weston_seat *seat = > wl_resource_get_user_data(seat_resource); > - struct weston_desktop_xdg_toplevel *toplevel = > - weston_desktop_surface_get_implementation_data(dsurface); > + struct weston_desktop_xdg_toplevel *toplevel; > enum weston_desktop_surface_edge surf_edges = > (enum weston_desktop_surface_edge) edges; > > + if (!dsurface) > + return; > + > + toplevel = weston_desktop_surface_get_implementation_data(dsurface); > + > if (!toplevel->base.configured) { > wl_resource_post_error(toplevel->resource, > ZXDG_SURFACE_V6_ERROR_NOT_CONSTRUCTED, > @@ -423,9 +439,12 @@ weston_desktop_xdg_toplevel_protocol_set_min_size(struct wl_client *wl_client, > { > struct weston_desktop_surface *dsurface = > wl_resource_get_user_data(resource); > - struct weston_desktop_xdg_toplevel *toplevel = > - weston_desktop_surface_get_implementation_data(dsurface); > + struct weston_desktop_xdg_toplevel *toplevel; > > + if (!dsurface) > + return; > + > + toplevel = weston_desktop_surface_get_implementation_data(dsurface); > toplevel->next_min_size.width = width; > toplevel->next_min_size.height = height; > } > @@ -437,9 +456,12 @@ weston_desktop_xdg_toplevel_protocol_set_max_size(struct wl_client *wl_client, > { > struct weston_desktop_surface *dsurface = > wl_resource_get_user_data(resource); > - struct weston_desktop_xdg_toplevel *toplevel = > - weston_desktop_surface_get_implementation_data(dsurface); > + struct weston_desktop_xdg_toplevel *toplevel; > + > + if (!dsurface) > + return; > > + toplevel = weston_desktop_surface_get_implementation_data(dsurface); > toplevel->next_max_size.width = width; > toplevel->next_max_size.height = height; > } > @@ -450,9 +472,12 @@ weston_desktop_xdg_toplevel_protocol_set_maximized(struct wl_client *wl_client, > { > struct weston_desktop_surface *dsurface = > wl_resource_get_user_data(resource); > - struct weston_desktop_xdg_toplevel *toplevel = > - weston_desktop_surface_get_implementation_data(dsurface); > + struct weston_desktop_xdg_toplevel *toplevel; > > + if (!dsurface) > + return; > + > + toplevel = weston_desktop_surface_get_implementation_data(dsurface); > weston_desktop_xdg_toplevel_ensure_added(toplevel); > weston_desktop_api_maximized_requested(toplevel->base.desktop, dsurface, true); > } > @@ -463,9 +488,12 @@ weston_desktop_xdg_toplevel_protocol_unset_maximized(struct wl_client *wl_client > { > struct weston_desktop_surface *dsurface = > wl_resource_get_user_data(resource); > - struct weston_desktop_xdg_toplevel *toplevel = > - weston_desktop_surface_get_implementation_data(dsurface); > + struct weston_desktop_xdg_toplevel *toplevel; > + > + if (!dsurface) > + return; > > + toplevel = weston_desktop_surface_get_implementation_data(dsurface); > weston_desktop_xdg_toplevel_ensure_added(toplevel); > weston_desktop_api_maximized_requested(toplevel->base.desktop, dsurface, false); > } > @@ -477,10 +505,14 @@ weston_desktop_xdg_toplevel_protocol_set_fullscreen(struct wl_client *wl_client, > { > struct weston_desktop_surface *dsurface = > wl_resource_get_user_data(resource); > - struct weston_desktop_xdg_toplevel *toplevel = > - weston_desktop_surface_get_implementation_data(dsurface); > + struct weston_desktop_xdg_toplevel *toplevel; > struct weston_output *output = NULL; > > + if (!dsurface) > + return; > + > + toplevel = weston_desktop_surface_get_implementation_data(dsurface); > + > if (output_resource != NULL) > output = wl_resource_get_user_data(output_resource); > > @@ -495,9 +527,12 @@ weston_desktop_xdg_toplevel_protocol_unset_fullscreen(struct wl_client *wl_clien > { > struct weston_desktop_surface *dsurface = > wl_resource_get_user_data(resource); > - struct weston_desktop_xdg_toplevel *toplevel = > - weston_desktop_surface_get_implementation_data(dsurface); > + struct weston_desktop_xdg_toplevel *toplevel; > + > + if (!dsurface) > + return; > > + toplevel = weston_desktop_surface_get_implementation_data(dsurface); > weston_desktop_xdg_toplevel_ensure_added(toplevel); > weston_desktop_api_fullscreen_requested(toplevel->base.desktop, dsurface, > false, NULL); > @@ -509,9 +544,12 @@ weston_desktop_xdg_toplevel_protocol_set_minimized(struct wl_client *wl_client, > { > struct weston_desktop_surface *dsurface = > wl_resource_get_user_data(resource); > - struct weston_desktop_xdg_toplevel *toplevel = > - weston_desktop_surface_get_implementation_data(dsurface); > + struct weston_desktop_xdg_toplevel *toplevel; > > + if (!dsurface) > + return; > + > + toplevel = weston_desktop_surface_get_implementation_data(dsurface); > weston_desktop_xdg_toplevel_ensure_added(toplevel); > weston_desktop_api_minimized_requested(toplevel->base.desktop, dsurface); > } > @@ -737,12 +775,19 @@ weston_desktop_xdg_popup_protocol_grab(struct wl_client *wl_client, > { > struct weston_desktop_surface *dsurface = > wl_resource_get_user_data(resource); > - struct weston_desktop_xdg_popup *popup = > - weston_desktop_surface_get_implementation_data(dsurface); > + struct weston_desktop_xdg_popup *popup; > struct weston_seat *wseat = wl_resource_get_user_data(seat_resource); > - struct weston_desktop_seat *seat = weston_desktop_seat_from_seat(wseat); > + struct weston_desktop_seat *seat; > struct weston_desktop_surface *topmost; > - bool parent_is_toplevel = > + bool parent_is_toplevel; > + > + if (!dsurface || !wseat) > + return; > + > + seat = weston_desktop_seat_from_seat(wseat); > + popup = weston_desktop_surface_get_implementation_data(dsurface); > + > + parent_is_toplevel = > popup->parent->role == WESTON_DESKTOP_XDG_SURFACE_ROLE_TOPLEVEL; > > if (popup->committed) { > @@ -891,10 +936,14 @@ weston_desktop_xdg_surface_protocol_get_toplevel(struct wl_client *wl_client, > { > struct weston_desktop_surface *dsurface = > wl_resource_get_user_data(resource); > - struct weston_surface *wsurface = > - weston_desktop_surface_get_surface(dsurface); > - struct weston_desktop_xdg_toplevel *toplevel = > - weston_desktop_surface_get_implementation_data(dsurface); > + struct weston_surface *wsurface; > + struct weston_desktop_xdg_toplevel *toplevel; > + > + if (!dsurface) > + return; > + > + toplevel = weston_desktop_surface_get_implementation_data(dsurface); > + wsurface = weston_desktop_surface_get_surface(dsurface); > > if (weston_surface_set_role(wsurface, weston_desktop_xdg_toplevel_role, > resource, ZXDG_SHELL_V6_ERROR_ROLE) < 0) > @@ -920,17 +969,21 @@ weston_desktop_xdg_surface_protocol_get_popup(struct wl_client *wl_client, > { > struct weston_desktop_surface *dsurface = > wl_resource_get_user_data(resource); > - struct weston_surface *wsurface = > - weston_desktop_surface_get_surface(dsurface); > - struct weston_desktop_xdg_popup *popup = > - weston_desktop_surface_get_implementation_data(dsurface); > + struct weston_surface *wsurface; > + struct weston_desktop_xdg_popup *popup; > struct weston_desktop_surface *parent_surface = > wl_resource_get_user_data(parent_resource); > - struct weston_desktop_xdg_surface *parent = > - weston_desktop_surface_get_implementation_data(parent_surface); > + struct weston_desktop_xdg_surface *parent; > struct weston_desktop_xdg_positioner *positioner = > wl_resource_get_user_data(positioner_resource); > > + if (!dsurface || !parent_surface || !positioner) > + return; > + > + wsurface = weston_desktop_surface_get_surface(dsurface); > + popup = weston_desktop_surface_get_implementation_data(dsurface); > + parent = weston_desktop_surface_get_implementation_data(parent_surface); > + > /* Checking whether the size and anchor rect both have a positive size > * is enough to verify both have been correctly set */ > if (positioner->size.width == 0 || positioner->anchor_rect.width == 0) { > @@ -994,8 +1047,12 @@ weston_desktop_xdg_surface_protocol_set_window_geometry(struct wl_client *wl_cli > { > struct weston_desktop_surface *dsurface = > wl_resource_get_user_data(resource); > - struct weston_desktop_xdg_surface *surface = > - weston_desktop_surface_get_implementation_data(dsurface); > + struct weston_desktop_xdg_surface *surface; > + > + if (!dsurface) > + return; > + > + surface = weston_desktop_surface_get_implementation_data(dsurface); > > if (!weston_desktop_xdg_surface_check_role(surface)) > return; > @@ -1014,8 +1071,12 @@ weston_desktop_xdg_surface_protocol_ack_configure(struct wl_client *wl_client, > { > struct weston_desktop_surface *dsurface = > wl_resource_get_user_data(resource); > - struct weston_desktop_xdg_surface *surface = > - weston_desktop_surface_get_implementation_data(dsurface); > + struct weston_desktop_xdg_surface *surface; > + > + if (!dsurface) > + return; > + > + surface = weston_desktop_surface_get_implementation_data(dsurface); > > if (!weston_desktop_xdg_surface_check_role(surface)) > return; </div></div></blockquote></div> </div>