gamescope icon indicating copy to clipboard operation
gamescope copied to clipboard

Fix issues that caused gamescope to abort at exit

Open sharkautarch opened this issue 9 months ago • 10 comments

The first commit fixes issue #1305 Note: I used a user event instead of the sdl quit event, to ensure that the sdl thread is only closed from inside steamcompmgr_exit()

After fixing the aforementioned issue, I noticed that gamescope would still sometimes abort/coredump at exit.

So I found a separate issue, wherein the present_wait thread, when running GetVBlankTimer().MarkVBlank( vblanktime, true );, would segfault upon trying to access a backend while steamcompmgr_exit() was deleting said backend. ~~The second commit fixes this w/ an awkward dance between the two threads, where steamcompmgr_exit() asks the present_wait thread to exit, and then waits for the present_wait thread to respond back. The reason why I did that weird stuff in the second commit is that I wanted to minimize the overhead being added to present_wait_thread_func(): for most of the runtime, this commit will not add any delay to present_wait_thread_func()~~ ~~update: Slight edit to the second commit: in steamcompmgr_exit(), I moved the atomic store to g_presentThreadShouldExit so that it happens before the atomic store to g_currentPresentWaitId, just to cover any rare edge case w/ atomic load/store timing~~ EDIT: I just revised this PR w.r.t. the second issue, and instead of trying to insert checks inside the present wait thread, I changed how backend creation and deletion worked, so that it is now safe to do SetBackend() while other threads are accessing the backend via GetBackend(). The added synchronization overhead should be pretty low, since the only change effecting GetBackend() and IBackend::Get() is that s_pBackend is now an atomic ptr, and the plain atomic load (even w/ the default seq_cst ordering) from s_pBackend gets compiled down to a plain move instruction on x86_64 and on aarch64, it's only a load-acquire atomic instruction (LDAR instruction, which still allows non-atomic accesses to be reordered around it)

I also fixed an additional issue that I noticed, where, at least when using SDL backend, gamescope would hang at exit (tho this isn't only able to be seen after fixing the two previous issue that would otherwise coredump gamescope at exit before it could hang).

For whatever reason, it seems like gamescope could hang at exit if the xwayland_server_guard lock was still locked when calling pthread_exit(), so this PR addresses that issue by simply unlocking that lock before calling pthread_exit().

Also disclaimer that when testing this PR w/ UBSAN, I noticed some warnings from UBSAN about vptr issues w.r.t. CVBlankTimer which only appeared while gamescope was exiting, and which don't seem to appear in upstream gamescope. Not sure if that's just a false positive, and I couldn't find anything wrong besides that.

sharkautarch avatar May 24 '24 20:05 sharkautarch