WaylandBackend: Re-map toplevel upon becoming visible

[layercak3]

When becoming invisible, a NULL buffer is attached to the toplevel's surface which unmaps it. The compositor resets their state and the surface may also be considered unconfigured. Upon becoming visible, the surface must be re-mapped using the same process during initialization (commit without a buffer and wait for configure) before we begin attaching an actual buffer. The default properties should also be recovered.

Fixes: #1456

[layercak3]

I was able to hit assert( !m_Planes[0].GetCurrentState() ); somewhere according to some recent coredump, so I need to change that at some point.

[layercak3]

I don't use the gamescope reaper or mangoapp (I start gamescope separately from my application process), so it's possible that I broke something there. Though, when I run gamescope vkcube and press ESC, I get 'Primary child shut down!' and both processes exit properly. I don't really understand why this patch would affect that.

[layercak3]

rebased on current-ish master

diff --git a/src/Backends/WaylandBackend.cpp b/src/Backends/WaylandBackend.cpp
index ec28d28..a743450 100644
--- a/src/Backends/WaylandBackend.cpp
+++ b/src/Backends/WaylandBackend.cpp
@@ -1129,6 +1129,28 @@ namespace gamescope
             return;
 
         m_bVisible = bVisible;
+
+        // Surface must be re-mapped before using it again, and default
+        // properties should be recovered as the compositor resets the state
+        if (m_bVisible && !m_Planes[0].GetCurrentState())
+        {
+            wl_surface_commit( m_Planes[0].GetSurface() );
+            wl_display_roundtrip( m_pBackend->GetDisplay() );
+
+            libdecor_frame_set_title( m_Planes[0].GetFrame(), "Gamescope" );
+            libdecor_frame_set_app_id( m_Planes[0].GetFrame(), "gamescope" );
+
+            g_nOutputWidth = g_nPreferredOutputWidth;
+            g_nOutputHeight = g_nPreferredOutputHeight;
+            if ( g_nOutputHeight == 0 )
+                g_nOutputHeight = 720;
+            if ( g_nOutputWidth == 0 )
+                g_nOutputWidth = g_nOutputHeight * 16 / 9;
+
+            m_Planes[0].CommitLibDecor( nullptr );
+            wl_display_roundtrip( m_pBackend->GetDisplay() );
+        }
+
         force_repaint();
     }
     void CWaylandConnector::SetTitle( std::shared_ptr<std::string> pAppTitle )

I have different issues and crashes since the virtual connector refactor (to the point where I cannot reproduce the crash this PR is fixing anymore), so I won't update this branch until those issues are fixed. I am quite busy so probably won't get to reporting those issues until several days from now however.

[MithicSpirit]

Yeah, I think we're facing a different issue now, and the old issue may actually be fixed.

Running something like gamescope -- sh -c 'vkcube; glxgears' and then pressing esc in vkcube (so it exits) makes the gamescope window containing vkcube stick around (but be frozen), then a new window is created for glxgears. Pressing escape in either window makes glxgears exit, but there is different behavior depending on whether the glxgears window is displayed. In particular, if is not displayed*, then gamescope exits cleanly. Otherwise, it crashes with a segmentation fault (and not an abort, which is different from before).

I've been able to fix the segmentation fault with a patch (IsSurfacePlane was being called with a null pointer), but this feels like a band-aid solution that does not tackle the root cause.

diff --git a/src/Backends/WaylandBackend.cpp b/src/Backends/WaylandBackend.cpp
index e924ac8..75c92bd 100644
--- a/src/Backends/WaylandBackend.cpp
+++ b/src/Backends/WaylandBackend.cpp
@@ -73,13 +73,13 @@ static inline uint32_t WaylandScaleToPhysical( uint32_t pValue, uint32_t pFactor
 }
 static inline uint32_t WaylandScaleToLogical( uint32_t pValue, uint32_t pFactor ) {
     return div_roundup( pValue * WL_FRACTIONAL_SCALE_DENOMINATOR, pFactor );
 }
 
 static bool IsSurfacePlane( wl_surface *pSurface ) {
-    return wl_proxy_get_tag( (wl_proxy *)pSurface ) == &GAMESCOPE_plane_tag;
+    return pSurface && (wl_proxy_get_tag( (wl_proxy *)pSurface ) == &GAMESCOPE_plane_tag);
 }
 
 #define WAYLAND_NULL() []<typename... Args> ( void *pData, Args... args ) { }
 #define WAYLAND_USERDATA_TO_THIS(type, name) []<typename... Args> ( void *pData, Args... args ) { type *pThing = (type *)pData; pThing->name( std::forward<Args>(args)... ); }
 
 // Libdecor puts its userdata ptr at the end... how fun! I shouldn't have spent so long writing this total atrocity to mankind.

*In sway, this occurs if it is in an undisplayed workspace or if it is tiled and behind another window.

[MithicSpirit]

Been looking a bit more into this. It looks like the issue is that the first window has an extra count on its std::shared_ptr<gamescope::IBackendConnector>, so it doesn't get destroyed when the global_focus_t is destroyed (via pVirtualConnector) at steamcompmgr.cpp:7840. Trying to figure out what's duplicating it.

[MithicSpirit]

I believe I've found the culprit, but it's difficult to fix.

gamescope/src/Backends/WaylandBackend.cpp

Lines 722 to 723 in ef1e8db

	// TODO: Restructure and remove the need for this.
	std::shared_ptr<CWaylandConnector> m_pFocusConnector;

gamescope/src/Backends/WaylandBackend.cpp

Lines 2116 to 2117 in ef1e8db

	if ( !m_pFocusConnector )
	m_pFocusConnector = pConnector;

---

EDIT:
Turns out it's not that difficult if you don't care about memory safety. Changing it to a weak reference seems to work, but I make no guarantees it won't segfault.

diff --git a/src/Backends/WaylandBackend.cpp b/src/Backends/WaylandBackend.cpp
index e924ac8..7aed647 100644
--- a/src/Backends/WaylandBackend.cpp
+++ b/src/Backends/WaylandBackend.cpp
@@ -73,13 +73,13 @@ static inline uint32_t WaylandScaleToPhysical( uint32_t pValue, uint32_t pFactor
 }
 static inline uint32_t WaylandScaleToLogical( uint32_t pValue, uint32_t pFactor ) {
     return div_roundup( pValue * WL_FRACTIONAL_SCALE_DENOMINATOR, pFactor );
 }
 
 static bool IsSurfacePlane( wl_surface *pSurface ) {
-    return wl_proxy_get_tag( (wl_proxy *)pSurface ) == &GAMESCOPE_plane_tag;
+    return pSurface && (wl_proxy_get_tag( (wl_proxy *)pSurface ) == &GAMESCOPE_plane_tag);
 }
 
 #define WAYLAND_NULL() []<typename... Args> ( void *pData, Args... args ) { }
 #define WAYLAND_USERDATA_TO_THIS(type, name) []<typename... Args> ( void *pData, Args... args ) { type *pThing = (type *)pData; pThing->name( std::forward<Args>(args)... ); }
 
 // Libdecor puts its userdata ptr at the end... how fun! I shouldn't have spent so long writing this total atrocity to mankind.
@@ -717,13 +717,13 @@ namespace gamescope
         zwp_pointer_constraints_v1 *m_pPointerConstraints = nullptr;
         zwp_relative_pointer_manager_v1 *m_pRelativePointerManager = nullptr;
         wp_fractional_scale_manager_v1 *m_pFractionalScaleManager = nullptr;
         xdg_toplevel_icon_manager_v1 *m_pToplevelIconManager = nullptr;
 
         // TODO: Restructure and remove the need for this.
-        std::shared_ptr<CWaylandConnector> m_pFocusConnector;
+        std::weak_ptr<CWaylandConnector> m_pFocusConnector;
 
         wl_data_device_manager *m_pDataDeviceManager = nullptr;
         wl_data_device *m_pDataDevice = nullptr;
         std::shared_ptr<std::string> m_pClipboard = nullptr;
 
         zwp_primary_selection_device_manager_v1 *m_pPrimarySelectionDeviceManager = nullptr;
@@ -2038,13 +2038,13 @@ namespace gamescope
 
         return std::span<const uint64_t>{ iter->second.begin(), iter->second.end() };
     }
 
     IBackendConnector *CWaylandBackend::GetCurrentConnector()
     {
-        return m_pFocusConnector.get();
+        return m_pFocusConnector.lock().get();
     }
     IBackendConnector *CWaylandBackend::GetConnector( GamescopeScreenType eScreenType )
     {
         if ( eScreenType == GAMESCOPE_SCREEN_TYPE_INTERNAL )
             return GetCurrentConnector();
 
@@ -2110,13 +2110,13 @@ namespace gamescope
     {
         return true;
     }
     std::shared_ptr<IBackendConnector> CWaylandBackend::CreateVirtualConnector( uint64_t ulVirtualConnectorKey )
     {
         std::shared_ptr<CWaylandConnector> pConnector = std::make_shared<CWaylandConnector>( this, ulVirtualConnectorKey );
-        if ( !m_pFocusConnector )
+        if ( m_pFocusConnector.expired() )
             m_pFocusConnector = pConnector;
 
         if ( !pConnector->Init() )
         {
             return nullptr;
         }

If no one has any better ideas on how to fix the problem, I'll submit this as a PR in a couple of days. At the very least, it's better than the current situation.

[LionHeartP]

Replying to #1611 (comment)

For me this patch properly fixes the issue on latest master.