On 27/08/14 05:41 AM, Pekka Paalanen wrote: > From: Pekka Paalanen <[email protected]> > > The desktop shell plugin registers both a wl_client destroy signal > listener, and a sigchld handler, when launching weston-desktop-shell. > However, nothing guarantees in which order do the wl_client destructor > and the sigchld handler run. > > Luckily, the sigchld handler cannot interrupt any code, because we > handle the signal via signalfd, which means it is handled like any event > in the compositor's main event loop. > > Still, shell.c has a race, that when lost, can cause a crash, as > described in bug #82957. > > If the sigchld handler happens to run first, it will try to launch a new > weston-desktop-shell without removing the destroy listener from the old > wl_client first. This leads to list corruption, that may cause a crash > when the old wl_client gets destroyed. > > Simply removing the destroy listener in the sigchld handler is not > enough, because respawning sets shell->child.client pointer, and if > the wl_client destructor runs after, it will reset it to NULL. > > OTOH, the wl_client destroy handler cannot reset shell->child.process, > because that would cause the sigchld handler in weston core to not find > the process tracker anymore, and report that an unknown process exited. > > Turns out, that to make everything work, we would need to wait for both > the wl_client destructor and the sigchld handler to have run, before > respawn. This gets tricky. > > Instead, solve the problem by removing shell->child.process. Use the new > weston_client_start() which automatically creates and manages the struct > weston_process. The shell does not need to know about the process exit, > it only needs to know about the client disconnect. Weston-desktop-shell > will never attempt to reconnect, and it would not work even if it did, > so disconnect is equivalent to weston-desktop-shell exiting. > > This should permanently solve the race for weston-desktop-shell. > > Bug: https://bugs.freedesktop.org/show_bug.cgi?id=82957 > Cc: Boyan Ding <[email protected]> > Cc: Derek Foreman <[email protected]> > Signed-off-by: Pekka Paalanen <[email protected]> > --- > desktop-shell/shell.c | 35 +++++++++++++++++++++-------------- > desktop-shell/shell.h | 1 - > 2 files changed, 21 insertions(+), 15 deletions(-) > > diff --git a/desktop-shell/shell.c b/desktop-shell/shell.c > index 99f3343..f82c47a 100644 > --- a/desktop-shell/shell.c > +++ b/desktop-shell/shell.c > @@ -5294,14 +5294,9 @@ shell_surface_configure(struct weston_surface *es, > int32_t sx, int32_t sy) > static void launch_desktop_shell_process(void *data); > > static void > -desktop_shell_sigchld(struct weston_process *process, int status) > +respawn_desktop_shell_process(struct desktop_shell *shell) > { > uint32_t time; > - struct desktop_shell *shell = > - container_of(process, struct desktop_shell, child.process); > - > - shell->child.process.pid = 0; > - shell->child.client = NULL; /* already destroyed by wayland */ > > /* if desktop-shell dies more than 5 times in 30 seconds, give up */ > time = weston_compositor_get_time(); > @@ -5312,13 +5307,12 @@ desktop_shell_sigchld(struct weston_process *process, > int status) > > shell->child.deathcount++; > if (shell->child.deathcount > 5) { > - weston_log("%s died, giving up.\n", shell->client); > + weston_log("%s disconnected, giving up.\n", shell->client);
Once again, being hugely pedantic - we'll get the disconnected message for a failed exec - so we may be printing this when nothing has actually connected in the first place. Really not sure what the best way is to make these errors meaningful though - assigning exit() codes? Other than that, this fixes the problem as I understand it and passes my simple test (launching the headless backend with --no-config and WESTON_BUILD_DIR set to an invalid directory) Cosmetics of log messages aside, Reviewed-by: Derek Foreman <[email protected]> for all three patches... I like the third one unconditionally. :) > return; > } > > - weston_log("%s died, respawning...\n", shell->client); > + weston_log("%s disconnected, respawning...\n", shell->client); > launch_desktop_shell_process(shell); > - shell_fade_startup(shell); > } > > static void > @@ -5329,7 +5323,18 @@ desktop_shell_client_destroy(struct wl_listener > *listener, void *data) > shell = container_of(listener, struct desktop_shell, > child.client_destroy_listener); > > + wl_list_remove(&shell->child.client_destroy_listener.link); > shell->child.client = NULL; > + /* > + * unbind_desktop_shell() will reset shell->child.desktop_shell > + * before the respawned process has a chance to create a new > + * desktop_shell object, because we are being called from the > + * wl_client destructor which destroys all wl_resources before > + * returning. > + */ > + > + respawn_desktop_shell_process(shell); > + shell_fade_startup(shell); > } > > static void > @@ -5337,10 +5342,8 @@ launch_desktop_shell_process(void *data) > { > struct desktop_shell *shell = data; > > - shell->child.client = weston_client_launch(shell->compositor, > - &shell->child.process, > - shell->client, > - desktop_shell_sigchld); > + shell->child.client = weston_client_start(shell->compositor, > + shell->client); > > if (!shell->child.client) > weston_log("not able to start %s\n", shell->client); > @@ -6152,8 +6155,12 @@ shell_destroy(struct wl_listener *listener, void *data) > > /* Force state to unlocked so we don't try to fade */ > shell->locked = false; > - if (shell->child.client) > + > + if (shell->child.client) { > + /* disable respawn */ > + wl_list_remove(&shell->child.client_destroy_listener.link); > wl_client_destroy(shell->child.client); > + } > > wl_list_remove(&shell->idle_listener.link); > wl_list_remove(&shell->wake_listener.link); > diff --git a/desktop-shell/shell.h b/desktop-shell/shell.h > index c6ea328..67c5f50 100644 > --- a/desktop-shell/shell.h > +++ b/desktop-shell/shell.h > @@ -134,7 +134,6 @@ struct desktop_shell { > struct weston_surface *grab_surface; > > struct { > - struct weston_process process; > struct wl_client *client; > struct wl_resource *desktop_shell; > struct wl_listener client_destroy_listener; > _______________________________________________ wayland-devel mailing list [email protected] http://lists.freedesktop.org/mailman/listinfo/wayland-devel
