This is an automated email from the ASF dual-hosted git repository. asf-gitbox-commits pushed a commit to branch trunk in repository https://gitbox.apache.org/repos/asf/openoffice.git
commit e1397ed79746b1482a549a3b8499549cff06c5f4 Author: Pedro Giffuni <[email protected]> AuthorDate: Sat Sep 12 00:08:00 2026 -0500 vcl: GTK2 ownership and lifecycle audit. Re-architected the GTK2 backend event dispatch pipeline in Apache OpenOffice by replacing linear frame searches with a centralized, O(1) X11 window-to-frame lookup map (m_aWindowFrameMap). Separated low-level X11 event routing into GtkSalFrame::dispatchXEvent() with explicit event consumption semantics while enforcing lifecycle invariants and registration safety. Changes Implemented findFrameByXWindow(), registerFrameWindow(), and deregisterFrameWindow() using std::map<XLIB_Window, GtkSalFrame*> in GtkSalDisplay (gtkdata.hxx, gtkdata.cxx), rejecting duplicate registrations with diagnostic logging. Replaced linear m_aFrames scans in GtkSalDisplay::filterGdkEvent and GtkSalDisplay::Dispatch with direct map lookup. Extracted GtkSalFrame::dispatchXEvent() in gtkframe.hxx and gtkframe.cxx to handle PropertyNotify, foreign-window ConfigureNotify, and _NET_WM_XEMBED client messages with explicit handling indicators, while retaining legacy Dispatch() as a continuation wrapper. Hardened GtkSalFrame lifecycle state handling (InitCommon, moveToScreen, createNewWindow, and ~GtkSalFrame) to deregister old X11 window IDs prior to widget realization/recreation and cleanly reset intermediate window state. Verification Audited all call sites and lifecycle paths (InitCommon, createNewWindow, moveToScreen, frame destruction, and foreign window resizing) to confirm window IDs are registered only after realization and deregistered before widget teardown. Verified that dispatchXEvent() null guards protect against unrealized widget access and that legacy callers of Dispatch() retain identical continuation semantics. Produced with JetBrain's CLion with some help from ChatGPT --- main/vcl/inc/unx/gtk/gtkdata.hxx | 11 +++++ main/vcl/inc/unx/gtk/gtkframe.hxx | 7 +-- main/vcl/unx/gtk/app/gtkdata.cxx | 90 +++++++++++++++++++++++++++------- main/vcl/unx/gtk/window/gtkframe.cxx | 95 ++++++++++++++++++++++++++++++++---- 4 files changed, 172 insertions(+), 31 deletions(-) diff --git a/main/vcl/inc/unx/gtk/gtkdata.hxx b/main/vcl/inc/unx/gtk/gtkdata.hxx index 614008b57f..20a35b4e70 100644 --- a/main/vcl/inc/unx/gtk/gtkdata.hxx +++ b/main/vcl/inc/unx/gtk/gtkdata.hxx @@ -24,6 +24,8 @@ #ifndef _VCL_GTKDATA_HXX #define _VCL_GTKDATA_HXX +#include <map> + #include <tools/prex.h> #include <gdk/gdk.h> #include <gdk/gdkx.h> @@ -53,6 +55,12 @@ class GtkSalDisplay : public SalDisplay GdkDisplay* m_pGdkDisplay; GdkCursor *m_aCursors[ POINTER_COUNT ]; bool m_bStartupCompleted; + /* + * Each live X11 window ID may be associated with at most one frame. + * Window IDs must be deregistered before destruction or reassignment. + * Duplicate registration is a lifecycle error and is not overwritten. + */ + std::map< XLIB_Window, GtkSalFrame* > m_aWindowFrameMap; GdkCursor* getFromXPM( const char *pBitmap, const char *pMask, int nWidth, int nHeight, int nXHot, int nYHot ); @@ -63,6 +71,9 @@ public: GdkDisplay* GetGdkDisplay() const { return m_pGdkDisplay; } virtual void deregisterFrame( SalFrame* pFrame ); + GtkSalFrame* findFrameByXWindow( XLIB_Window aWindow ) const; + void registerFrameWindow( XLIB_Window aWindow, GtkSalFrame* pFrame ); + void deregisterFrameWindow( XLIB_Window aWindow, GtkSalFrame* pFrame = NULL ); GdkCursor *getCursor( PointerStyle ePointerStyle ); virtual int CaptureMouse( SalFrame* pFrame ); virtual long Dispatch( XEvent *pEvent ); diff --git a/main/vcl/inc/unx/gtk/gtkframe.hxx b/main/vcl/inc/unx/gtk/gtkframe.hxx index f6e336f51d..a948ffdd2f 100644 --- a/main/vcl/inc/unx/gtk/gtkframe.hxx +++ b/main/vcl/inc/unx/gtk/gtkframe.hxx @@ -269,9 +269,10 @@ public: GtkSalFrame( SalFrame* pParent, sal_uLong nStyle ); GtkSalFrame( SystemParentData* pSysData ); - // dispatches an event, returns true if dispatched - // and false else; if true was returned the event should - // be swallowed + // Dispatches an XEvent; true means handled and swallowed. + bool dispatchXEvent( const XEvent* pEvent ); + + // Legacy wrapper: true means continue, false means swallowed. bool Dispatch( const XEvent* pEvent ); void grabPointer( sal_Bool bGrab, sal_Bool bOwnerEvents = sal_False ); diff --git a/main/vcl/unx/gtk/app/gtkdata.cxx b/main/vcl/unx/gtk/app/gtkdata.cxx index ee91cbc3a1..21a33225a4 100644 --- a/main/vcl/unx/gtk/app/gtkdata.cxx +++ b/main/vcl/unx/gtk/app/gtkdata.cxx @@ -94,9 +94,69 @@ void GtkSalDisplay::deregisterFrame( SalFrame* pFrame ) static_cast<GtkSalFrame*>(m_pCapture)->grabPointer( FALSE ); m_pCapture = NULL; } + for( std::map< XLIB_Window, GtkSalFrame* >::iterator it = m_aWindowFrameMap.begin(); + it != m_aWindowFrameMap.end(); ) + { + if( it->second == static_cast<GtkSalFrame*>(pFrame) ) + { + std::map< XLIB_Window, GtkSalFrame* >::iterator itNext = it; + ++itNext; + m_aWindowFrameMap.erase( it ); + it = itNext; + } + else + ++it; + } SalDisplay::deregisterFrame( pFrame ); } +/* + * Each live X11 window ID may be associated with at most one frame. + * Window IDs must be deregistered before destruction or reassignment. + * Duplicate registration is a lifecycle error and is not overwritten. + */ +void GtkSalDisplay::registerFrameWindow( XLIB_Window aWindow, GtkSalFrame* pFrame ) +{ + if( aWindow != None && pFrame != NULL ) + { + std::map< XLIB_Window, GtkSalFrame* >::iterator it = + m_aWindowFrameMap.find( aWindow ); + + if( it != m_aWindowFrameMap.end() && it->second != pFrame ) + { + OSL_TRACE( + "GtkSalDisplay::registerFrameWindow: refusing duplicate " + "mapping for X window %lu", + static_cast<unsigned long>(aWindow) ); + return; + } + + m_aWindowFrameMap[ aWindow ] = pFrame; + } +} + +void GtkSalDisplay::deregisterFrameWindow( XLIB_Window aWindow, GtkSalFrame* pFrame ) +{ + if( aWindow == None ) + return; + std::map< XLIB_Window, GtkSalFrame* >::iterator it = m_aWindowFrameMap.find( aWindow ); + if( it != m_aWindowFrameMap.end() ) + { + if( pFrame == NULL || it->second == pFrame ) + m_aWindowFrameMap.erase( it ); + } +} + +GtkSalFrame* GtkSalDisplay::findFrameByXWindow( XLIB_Window aWindow ) const +{ + if( aWindow == None ) + return NULL; + std::map< XLIB_Window, GtkSalFrame* >::const_iterator it = m_aWindowFrameMap.find( aWindow ); + if( it != m_aWindowFrameMap.end() ) + return it->second; + return NULL; +} + extern "C" { GdkFilterReturn call_filterGdkEvent( GdkXEvent* sys_event, GdkEvent* event, @@ -157,19 +217,11 @@ GdkFilterReturn GtkSalDisplay::filterGdkEvent( GdkXEvent* sys_event, } // let's see if one of our frames wants to swallow these events // get the frame - for( std::list< SalFrame* >::const_iterator it = pDisplay->m_aFrames.begin(); - it != pDisplay->m_aFrames.end(); ++it ) + GtkSalFrame* pFrame = pDisplay->findFrameByXWindow( pEvent->xany.window ); + if( pFrame ) { - GtkSalFrame* pFrame = static_cast<GtkSalFrame*>(*it); - if( (GdkNativeWindow)pFrame->GetSystemData()->aWindow == pEvent->xany.window || - ( pFrame->getForeignParent() && pFrame->getForeignParentWindow() == pEvent->xany.window ) || - ( pFrame->getForeignTopLevel() && pFrame->getForeignTopLevelWindow() == pEvent->xany.window ) - ) - { - if( ! pFrame->Dispatch( pEvent ) ) - aFilterReturn = GDK_FILTER_REMOVE; - break; - } + if( pFrame->dispatchXEvent( pEvent ) ) + aFilterReturn = GDK_FILTER_REMOVE; } X11SalObject::Dispatch( pEvent ); } @@ -250,13 +302,15 @@ long GtkSalDisplay::Dispatch( XEvent* pEvent ) { if( GetDisplay() == pEvent->xany.display ) { - // let's see if one of our frames wants to swallow these events - // get the child frame - for( std::list< SalFrame* >::const_iterator it = m_aFrames.begin(); - it != m_aFrames.end(); ++it ) + GtkSalFrame* pFrame = findFrameByXWindow( pEvent->xany.window ); + if( pFrame ) { - if( (GdkNativeWindow)(*it)->GetSystemData()->aWindow == pEvent->xany.window ) - return static_cast<GtkSalFrame*>(*it)->Dispatch( pEvent ); + /* + * dispatchXEvent() returns true when the frame handled the event. + * This method returns a GdkFilterReturn, not the legacy Dispatch() + * continuation boolean. + */ + return pFrame->dispatchXEvent( pEvent ) ? GDK_FILTER_REMOVE : GDK_FILTER_CONTINUE; } } diff --git a/main/vcl/unx/gtk/window/gtkframe.cxx b/main/vcl/unx/gtk/window/gtkframe.cxx index 578bcb16ff..e622639eab 100644 --- a/main/vcl/unx/gtk/window/gtkframe.cxx +++ b/main/vcl/unx/gtk/window/gtkframe.cxx @@ -382,6 +382,11 @@ GtkSalFrame::GraphicsHolder::~GraphicsHolder() GtkSalFrame::GtkSalFrame( SalFrame* pParent, sal_uLong nStyle ) { + memset( &m_aSystemData, 0, sizeof(m_aSystemData) ); + m_aForeignParentWindow = None; + m_aForeignTopLevelWindow = None; + m_pForeignParent = NULL; + m_pForeignTopLevel = NULL; m_nScreen = getDisplay()->GetDefaultScreenNumber(); getDisplay()->registerFrame( this ); m_bDefaultPos = true; @@ -392,6 +397,11 @@ GtkSalFrame::GtkSalFrame( SalFrame* pParent, sal_uLong nStyle ) GtkSalFrame::GtkSalFrame( SystemParentData* pSysData ) { + memset( &m_aSystemData, 0, sizeof(m_aSystemData) ); + m_aForeignParentWindow = None; + m_aForeignTopLevelWindow = None; + m_pForeignParent = NULL; + m_pForeignTopLevel = NULL; m_nScreen = getDisplay()->GetDefaultScreenNumber(); getDisplay()->registerFrame( this ); getDisplay()->setHaveSystemChildFrame(); @@ -413,6 +423,15 @@ GtkSalFrame::~GtkSalFrame() if( m_pParent ) m_pParent->m_aChildren.remove( this ); + // Early explicit removal of registered native and foreign window IDs + if( m_pWindow && m_pWindow->window ) + getDisplay()->deregisterFrameWindow( GDK_WINDOW_XWINDOW(m_pWindow->window), this ); + if( m_aForeignParentWindow != None ) + getDisplay()->deregisterFrameWindow( (XLIB_Window)m_aForeignParentWindow, this ); + if( m_aForeignTopLevelWindow != None ) + getDisplay()->deregisterFrameWindow( (XLIB_Window)m_aForeignTopLevelWindow, this ); + + // Final safety net cleanup for frame list and window map getDisplay()->deregisterFrame( this ); if( m_pRegion ) @@ -573,6 +592,10 @@ void GtkSalFrame::InitCommon() // show the widgets gtk_widget_show( GTK_WIDGET(m_pFixedContainer) ); + XLIB_Window aOldWindow = (XLIB_Window)m_aSystemData.aWindow; + if( aOldWindow != None ) + getDisplay()->deregisterFrameWindow( aOldWindow, this ); + // realize the window, we need an XWindow id gtk_widget_realize( m_pWindow ); @@ -580,7 +603,10 @@ void GtkSalFrame::InitCommon() SalDisplay* pDisp = GetX11SalData()->GetDisplay(); m_aSystemData.nSize = sizeof( SystemChildData ); m_aSystemData.pDisplay = pDisp->GetDisplay(); - m_aSystemData.aWindow = GDK_WINDOW_XWINDOW(m_pWindow->window); + if( m_pWindow && m_pWindow->window ) + m_aSystemData.aWindow = GDK_WINDOW_XWINDOW(m_pWindow->window); + else + m_aSystemData.aWindow = None; m_aSystemData.pSalFrame = this; m_aSystemData.pWidget = m_pWindow; m_aSystemData.pVisual = pDisp->GetVisual( m_nScreen ).GetVisual(); @@ -591,6 +617,9 @@ void GtkSalFrame::InitCommon() m_aSystemData.aShellWindow = m_aSystemData.aWindow; m_aSystemData.pShellWidget = m_aSystemData.pWidget; + if( m_aSystemData.aWindow != None ) + getDisplay()->registerFrameWindow( (XLIB_Window)m_aSystemData.aWindow, this ); + // fake an initial geometry, gets updated via configure event or SetPosSize if( m_bDefaultPos || m_bDefaultSize ) @@ -916,6 +945,11 @@ void GtkSalFrame::Init( SystemParentData* pSysData ) m_nStyle = SAL_FRAME_STYLE_PLUG; InitCommon(); + if( m_aForeignParentWindow != None ) + getDisplay()->registerFrameWindow( (XLIB_Window)m_aForeignParentWindow, this ); + if( m_aForeignTopLevelWindow != None && m_aForeignTopLevelWindow != m_aForeignParentWindow ) + getDisplay()->registerFrameWindow( (XLIB_Window)m_aForeignTopLevelWindow, this ); + m_pForeignParent = gdk_window_foreign_new_for_display( getGdkDisplay(), m_aForeignParentWindow ); gdk_window_set_events( m_pForeignParent, GDK_STRUCTURE_MASK ); int x_ret, y_ret; @@ -1729,17 +1763,26 @@ void GtkSalFrame::moveToScreen( int nScreen ) { m_nScreen = nScreen; gtk_window_set_screen( GTK_WINDOW(m_pWindow), pScreen ); + XLIB_Window aOldWin = (XLIB_Window)m_aSystemData.aWindow; + if( aOldWin != None ) + getDisplay()->deregisterFrameWindow( aOldWin, this ); + // realize the window, we need an XWindow id gtk_widget_realize( m_pWindow ); // update system data GtkSalDisplay* pDisp = getDisplay(); - m_aSystemData.aWindow = GDK_WINDOW_XWINDOW(m_pWindow->window); + if( m_pWindow && m_pWindow->window ) + m_aSystemData.aWindow = GDK_WINDOW_XWINDOW(m_pWindow->window); + else + m_aSystemData.aWindow = None; m_aSystemData.pVisual = pDisp->GetVisual( m_nScreen ).GetVisual(); m_aSystemData.nScreen = nScreen; m_aSystemData.nDepth = pDisp->GetVisual( m_nScreen ).GetDepth(); m_aSystemData.aColormap = pDisp->GetColormap( m_nScreen ).GetXColormap(); m_aSystemData.pAppContext = NULL; m_aSystemData.aShellWindow = m_aSystemData.aWindow; + if( m_aSystemData.aWindow != None ) + pDisp->registerFrameWindow( (XLIB_Window)m_aSystemData.aWindow, this ); // update graphics if necessary for( unsigned int i = 0; i < sizeof(m_aGraphics)/sizeof(m_aGraphics[0]); i++ ) { @@ -2404,14 +2447,34 @@ void GtkSalFrame::createNewWindow( XLIB_Window aNewParent, bool bXEmbed, int nSc } if( m_pRegion ) gdk_region_destroy( m_pRegion ); + if( m_pWindow && m_pWindow->window ) + getDisplay()->deregisterFrameWindow( GDK_WINDOW_XWINDOW(m_pWindow->window), this ); + if( m_aForeignParentWindow != None ) + getDisplay()->deregisterFrameWindow( (XLIB_Window)m_aForeignParentWindow, this ); + if( m_aForeignTopLevelWindow != None ) + getDisplay()->deregisterFrameWindow( (XLIB_Window)m_aForeignTopLevelWindow, this ); if( m_pFixedContainer ) gtk_widget_destroy( GTK_WIDGET(m_pFixedContainer) ); if( m_pWindow ) gtk_widget_destroy( m_pWindow ); + m_pFixedContainer = NULL; + m_pWindow = NULL; + + m_aSystemData.aWindow = None; + m_aSystemData.aShellWindow = None; + m_aForeignParentWindow = None; + m_aForeignTopLevelWindow = None; + if( m_pForeignParent ) + { g_object_unref( G_OBJECT(m_pForeignParent) ); + m_pForeignParent = NULL; + } if( m_pForeignTopLevel ) + { g_object_unref( G_OBJECT(m_pForeignTopLevel) ); + m_pForeignTopLevel = NULL; + } // init new window m_bDefaultPos = m_bDefaultSize = false; @@ -2491,25 +2554,29 @@ void GtkSalFrame::EndSetClipRegion() gdk_window_shape_combine_region( m_pWindow->window, m_pRegion, 0, 0 ); } -bool GtkSalFrame::Dispatch( const XEvent* pEvent ) +bool GtkSalFrame::dispatchXEvent( const XEvent* pEvent ) { - bool bContinueDispatch = true; + bool bHandled = false; if( pEvent->type == PropertyNotify ) { vcl_sal::WMAdaptor* pAdaptor = getDisplay()->getWMAdaptor(); Atom nDesktopAtom = pAdaptor->getAtom( vcl_sal::WMAdaptor::NET_WM_DESKTOP ); if( pEvent->xproperty.atom == nDesktopAtom && - pEvent->xproperty.state == PropertyNewValue ) + pEvent->xproperty.state == PropertyNewValue && + m_pWindow && m_pWindow->window ) { m_nWorkArea = pAdaptor->getWindowWorkArea( GDK_WINDOW_XWINDOW( m_pWindow->window) ); } } else if( pEvent->type == ConfigureNotify ) { - if( m_pForeignParent && pEvent->xconfigure.window == m_aForeignParentWindow ) + if( m_pForeignParent && + m_pWindow && + m_pWindow->window && + pEvent->xconfigure.window == m_aForeignParentWindow ) { - bContinueDispatch = false; + bHandled = true; gtk_window_resize( GTK_WINDOW(m_pWindow), pEvent->xconfigure.width, pEvent->xconfigure.height ); if( ( sal::static_int_cast< int >(maGeometry.nWidth) != pEvent->xconfigure.width ) || @@ -2522,9 +2589,11 @@ bool GtkSalFrame::Dispatch( const XEvent* pEvent ) getDisplay()->SendInternalEvent( this, NULL, SALEVENT_RESIZE ); } } - else if( m_pForeignTopLevel && pEvent->xconfigure.window == m_aForeignTopLevelWindow ) + else if( m_pForeignTopLevel && + m_pWindow && m_pWindow->window && + pEvent->xconfigure.window == m_aForeignTopLevelWindow ) { - bContinueDispatch = false; + bHandled = true; // update position int x = 0, y = 0; XLIB_Window aChild; @@ -2544,6 +2613,7 @@ bool GtkSalFrame::Dispatch( const XEvent* pEvent ) } else if( pEvent->type == ClientMessage && pEvent->xclient.message_type == getDisplay()->getWMAdaptor()->getAtom( vcl_sal::WMAdaptor::XEMBED ) && + m_pWindow && m_pWindow->window && pEvent->xclient.window == GDK_WINDOW_XWINDOW(m_pWindow->window) && m_bWindowIsGtkPlug ) @@ -2563,7 +2633,12 @@ bool GtkSalFrame::Dispatch( const XEvent* pEvent ) } } - return bContinueDispatch; + return bHandled; +} + +bool GtkSalFrame::Dispatch( const XEvent* pEvent ) +{ + return !dispatchXEvent( pEvent ); } void GtkSalFrame::SetBackgroundBitmap( SalBitmap* pBitmap )
