vcl/inc/win/salframe.h | 5 - vcl/source/uitest/uitest.cxx | 66 +++++++++----- vcl/win/window/salframe.cxx | 192 ++++++++++++------------------------------- 3 files changed, 100 insertions(+), 163 deletions(-)
New commits: commit 90004b730eda4d06313f6e14603f1c1dad0f8f88 Author: Noel Grandin <[email protected]> AuthorDate: Tue Dec 23 20:42:14 2025 +0200 Commit: Noel Grandin <[email protected]> CommitDate: Wed Dec 31 11:18:48 2025 +0100 tdf#153554 fix crash when running python ODK tests WinSalFrame has a single mbGraphicsAcquired flag protecting two pointers to WinSalGraphics objects. When two threads intersperse calls to AcquireGraphics and ReleaseGraphics, things get very confused. However, adding another boolean results in horrible rendering artifacts because skia, unlike GDI, really does not like two threads drawing to the same surface via different WinSalGraphics objects. So just have a single WinSalGraphics object, which seems to fix things. (But which might, of course, break other things in this multi-threaded rendering nightmare we have) Also make the UNO calls from the python tests run on the main thread. This prevents various deadlocks from occurring. Change-Id: Ibef1a876508760b74bad7df9bee47c3ef956d2b6 Reviewed-on: https://gerrit.libreoffice.org/c/core/+/196171 Tested-by: Jenkins Reviewed-by: Noel Grandin <[email protected]> diff --git a/vcl/inc/win/salframe.h b/vcl/inc/win/salframe.h index c481cf49187d..34a4e168713d 100644 --- a/vcl/inc/win/salframe.h +++ b/vcl/inc/win/salframe.h @@ -40,10 +40,7 @@ public: HWND mhWnd; // Window handle HCURSOR mhCursor; // cursor handle HIMC mhDefIMEContext; // default IME-Context - WinSalGraphics* mpLocalGraphics; // current main thread frame graphics - /// Some threads will call vcl functions, and even though they hold the SolarMutex, the GDI - /// library will error, so we need to keep a separate graphics and DC for them. - WinSalGraphics* mpThreadGraphics; // current frame graphics for other threads (DCX_CACHE) + WinSalGraphics* mpGraphics; // current main thread frame graphics WinSalFrame* mpNextFrame; // pointer to next frame HMENU mSelectedhMenu; // the menu where highlighting is currently going on HMENU mLastActivatedhMenu; // the menu that was most recently opened diff --git a/vcl/source/uitest/uitest.cxx b/vcl/source/uitest/uitest.cxx index 5c6b03bbaaa9..6e882f38e4b5 100644 --- a/vcl/source/uitest/uitest.cxx +++ b/vcl/source/uitest/uitest.cxx @@ -13,6 +13,7 @@ #include <vcl/uitest/uiobject.hxx> #include <vcl/toolkit/dialog.hxx> +#include <vcl/threadex.hxx> #include <svdata.hxx> @@ -21,46 +22,61 @@ bool UITest::executeCommand(const OUString& rCommand) { - return comphelper::dispatchCommand( - rCommand, - {{u"SynchronMode"_ustr, -1, css::uno::Any(true), - css::beans::PropertyState_DIRECT_VALUE}}); + return vcl::solarthread::syncExecute( + [&rCommand]() + { + return comphelper::dispatchCommand(rCommand, + { { u"SynchronMode"_ustr, -1, css::uno::Any(true), + css::beans::PropertyState_DIRECT_VALUE } }); + }); } bool UITest::executeCommandWithParameters(const OUString& rCommand, const css::uno::Sequence< css::beans::PropertyValue >& rArgs) { - css::uno::Sequence< css::beans::PropertyValue > lNewArgs = - {{u"SynchronMode"_ustr, -1, css::uno::Any(true), - css::beans::PropertyState_DIRECT_VALUE}}; - - if ( rArgs.hasElements() ) - { - sal_uInt32 nIndex( lNewArgs.getLength() ); - lNewArgs.realloc( lNewArgs.getLength()+rArgs.getLength() ); - - std::copy(rArgs.begin(), rArgs.end(), std::next(lNewArgs.getArray(), nIndex)); - } - return comphelper::dispatchCommand(rCommand,lNewArgs); + return vcl::solarthread::syncExecute( + [&rCommand, &rArgs]() + { + css::uno::Sequence<css::beans::PropertyValue> lNewArgs + = { { u"SynchronMode"_ustr, -1, css::uno::Any(true), + css::beans::PropertyState_DIRECT_VALUE } }; + + if (rArgs.hasElements()) + { + sal_uInt32 nIndex(lNewArgs.getLength()); + lNewArgs.realloc(lNewArgs.getLength() + rArgs.getLength()); + + std::copy(rArgs.begin(), rArgs.end(), std::next(lNewArgs.getArray(), nIndex)); + } + return comphelper::dispatchCommand(rCommand, lNewArgs); + }); } bool UITest::executeCommandForProvider( const OUString& rCommand, const css::uno::Reference< css::frame::XDispatchProvider >& xProvider) { - return comphelper::dispatchCommand( - rCommand, - xProvider, - {{u"SynchronMode"_ustr, -1, css::uno::Any(true), - css::beans::PropertyState_DIRECT_VALUE}}); + return vcl::solarthread::syncExecute( + [&rCommand, &xProvider]() + { + return comphelper::dispatchCommand( + rCommand, + xProvider, + {{u"SynchronMode"_ustr, -1, css::uno::Any(true), + css::beans::PropertyState_DIRECT_VALUE}}); + }); } bool UITest::executeDialog(const OUString& rCommand) { - return comphelper::dispatchCommand( - rCommand, - {{u"SynchronMode"_ustr, -1, css::uno::Any(false), - css::beans::PropertyState_DIRECT_VALUE}}); + return vcl::solarthread::syncExecute( + [&rCommand]() + { + return comphelper::dispatchCommand( + rCommand, + {{u"SynchronMode"_ustr, -1, css::uno::Any(false), + css::beans::PropertyState_DIRECT_VALUE}}); + }); } std::unique_ptr<UIObject> UITest::getFocusTopWindow() diff --git a/vcl/win/window/salframe.cxx b/vcl/win/window/salframe.cxx index 85937d6f30fa..4e243ebc2044 100644 --- a/vcl/win/window/salframe.cxx +++ b/vcl/win/window/salframe.cxx @@ -864,8 +864,7 @@ WinSalFrame::WinSalFrame() mhWnd = nullptr; mhCursor = LoadCursor( nullptr, IDC_ARROW ); mhDefIMEContext = nullptr; - mpLocalGraphics = nullptr; - mpThreadGraphics = nullptr; + mpGraphics = nullptr; m_eState = vcl::WindowState::Normal; mnShowState = SW_SHOWNORMAL; mnMinWidth = 0; @@ -955,28 +954,18 @@ WinSalFrame::~WinSalFrame() *ppFrame = mpNextFrame; mpNextFrame = nullptr; - // destroy the thread SalGraphics - if ( mpThreadGraphics ) + // destroy the SalGraphics + if (mpGraphics) { - HDC hDC = mpThreadGraphics->getHDC(); - if (hDC) - { - mpThreadGraphics->setHDC(nullptr); - pSalData->mpInstance->SendComWndMessage(SAL_MSG_RELEASEDC, - reinterpret_cast<WPARAM>(mhWnd), reinterpret_cast<LPARAM>(hDC) ); - } - delete mpThreadGraphics; - mpThreadGraphics = nullptr; - } - - // destroy the local SalGraphics - if ( mpLocalGraphics ) - { - HDC hDC = mpLocalGraphics->getHDC(); - mpLocalGraphics->setHDC(nullptr); - ReleaseDC(mhWnd, hDC); - delete mpLocalGraphics; - mpLocalGraphics = nullptr; + HDC hDC = mpGraphics->getHDC(); + mpGraphics->setHDC(nullptr); + if (pSalData->mpInstance->IsMainThread()) + ReleaseDC(mhWnd, hDC); + else + pSalData->mpInstance->SendComWndMessage( + SAL_MSG_RELEASEDC, reinterpret_cast<WPARAM>(mhWnd), reinterpret_cast<LPARAM>(hDC)); + delete mpGraphics; + mpGraphics = nullptr; } if ( m_pTaskbarList3 ) @@ -1006,61 +995,31 @@ WinSalFrame::~WinSalFrame() SalGraphics* WinSalFrame::AcquireGraphics() { - if ( mbGraphicsAcquired || !mhWnd ) + if (!mhWnd || mbGraphicsAcquired) return nullptr; SalData* pSalData = GetSalData(); - // Other threads get an own DC, because Windows modify in the - // other case our DC (changing clip region), when they send a - // WM_ERASEBACKGROUND message - if ( !pSalData->mpInstance->IsMainThread() ) + if (!mpGraphics) { - HDC hDC = reinterpret_cast<HDC>(static_cast<sal_IntPtr>(pSalData->mpInstance->SendComWndMessage( - SAL_MSG_GETCACHEDDC, reinterpret_cast<WPARAM>(mhWnd), 0 ))); - if ( !hDC ) + HDC hDC; + if (pSalData->mpInstance->IsMainThread()) + hDC = GetDC(mhWnd); + else + hDC = reinterpret_cast<HDC>( + static_cast<sal_IntPtr>(pSalData->mpInstance->SendComWndMessage( + SAL_MSG_GETCACHEDDC, reinterpret_cast<WPARAM>(mhWnd), 0))); + if (!hDC) return nullptr; - - if ( !mpThreadGraphics ) - mpThreadGraphics = new WinSalGraphics(WinSalGraphics::WINDOW, true, mhWnd, this); - - assert(!mpThreadGraphics->getHDC() && "this is supposed to be zeroed when ReleaseGraphics is called"); - mpThreadGraphics->setHDC( hDC ); - - mbGraphicsAcquired = true; - return mpThreadGraphics; - } - else - { - if ( !mpLocalGraphics ) - { - HDC hDC = GetDC( mhWnd ); - if ( !hDC ) - return nullptr; - mpLocalGraphics = new WinSalGraphics(WinSalGraphics::WINDOW, true, mhWnd, this); - mpLocalGraphics->setHDC( hDC ); - } - mbGraphicsAcquired = true; - return mpLocalGraphics; + mpGraphics = new WinSalGraphics(WinSalGraphics::WINDOW, true, mhWnd, this); + mpGraphics->setHDC(hDC); } + mbGraphicsAcquired = true; + return mpGraphics; } -void WinSalFrame::ReleaseGraphics( SalGraphics* pGraphics ) +void WinSalFrame::ReleaseGraphics(SalGraphics* /*pGraphics*/) { - assert(mbGraphicsAcquired && "Can only call ReleaseGraphics when you own the graphics"); - if ( mpThreadGraphics == pGraphics ) - { - SalData* pSalData = GetSalData(); - HDC hDC = mpThreadGraphics->getHDC(); - assert(hDC); - mpThreadGraphics->setHDC(nullptr); - pSalData->mpInstance->SendComWndMessage(SAL_MSG_RELEASEDC, - reinterpret_cast<WPARAM>(mhWnd), reinterpret_cast<LPARAM>(hDC) ); - } - else - { - assert(mpLocalGraphics == pGraphics); - } mbGraphicsAcquired = false; } @@ -1486,38 +1445,19 @@ void WinSalFrame::ImplSetParentFrame( HWND hNewParentWnd, bool bAsChild ) } // to recreate the DCs, if they were destroyed - bool bHadLocalGraphics = false, bHadThreadGraphics = false; + bool bHadGraphics = false; - HFONT hFont = nullptr; - HPEN hPen = nullptr; - HBRUSH hBrush = nullptr; - - // release the thread DC - if ( mpThreadGraphics ) + // release the DC + if ( mpGraphics ) { - // save current gdi objects before hdc is gone - HDC hDC = mpThreadGraphics->getHDC(); - if (hDC) - { - hFont = static_cast<HFONT>(GetCurrentObject( hDC, OBJ_FONT )); - hPen = static_cast<HPEN>(GetCurrentObject( hDC, OBJ_PEN )); - hBrush = static_cast<HBRUSH>(GetCurrentObject( hDC, OBJ_BRUSH )); - - mpThreadGraphics->setHDC(nullptr); - pSalData->mpInstance->SendComWndMessage(SAL_MSG_RELEASEDC, - reinterpret_cast<WPARAM>(mhWnd), reinterpret_cast<LPARAM>(hDC) ); - - bHadThreadGraphics = true; - } - } - - // release the local DC - if ( mpLocalGraphics ) - { - bHadLocalGraphics = true; - HDC hDC = mpLocalGraphics->getHDC(); - mpLocalGraphics->setHDC(nullptr); - ReleaseDC(mhWnd, hDC); + bHadGraphics = true; + HDC hDC = mpGraphics->getHDC(); + mpGraphics->setHDC(nullptr); + if (pSalData->mpInstance->IsMainThread()) + ReleaseDC(mhWnd, hDC); + else + pSalData->mpInstance->SendComWndMessage(SAL_MSG_RELEASEDC, reinterpret_cast<WPARAM>(mhWnd), + reinterpret_cast<LPARAM>(hDC)); } // create a new hwnd with the same styles @@ -1530,33 +1470,19 @@ void WinSalFrame::ImplSetParentFrame( HWND hNewParentWnd, bool bAsChild ) // succeeded ? SAL_WARN_IF( !IsWindow( hWnd ), "vcl", "WinSalFrame::SetParent not successful"); - // re-create thread DC - if( bHadThreadGraphics ) + // re-create DC + if( bHadGraphics ) { - mpThreadGraphics->setHWND( hWnd ); - HDC hDC = reinterpret_cast<HDC>(static_cast<sal_IntPtr>( - pSalData->mpInstance->SendComWndMessage( - SAL_MSG_GETCACHEDDC, reinterpret_cast<WPARAM>(hWnd), 0 ))); - if ( hDC ) - { - mpThreadGraphics->setHDC( hDC ); - // re-select saved gdi objects - if( hFont ) - SelectObject( hDC, hFont ); - if( hPen ) - SelectObject( hDC, hPen ); - if( hBrush ) - SelectObject( hDC, hBrush ); - } - } - - // re-create local DC - if( bHadLocalGraphics ) - { - mpLocalGraphics->setHWND( hWnd ); - HDC hDC = GetDC( hWnd ); + mpGraphics->setHWND( hWnd ); + HDC hDC; + if (pSalData->mpInstance->IsMainThread()) + hDC = GetDC( hWnd ); + else + hDC = reinterpret_cast<HDC>( + static_cast<sal_IntPtr>(pSalData->mpInstance->SendComWndMessage( + SAL_MSG_GETCACHEDDC, reinterpret_cast<WPARAM>(hWnd), 0))); if (hDC) - mpLocalGraphics->setHDC( hDC ); + mpGraphics->setHDC( hDC ); } // TODO: add SetParent() call for SalObjects @@ -2186,10 +2112,8 @@ void WinSalFrame::SetPointerPos( tools::Long nX, tools::Long nY ) void WinSalFrame::Flush() { - if(mpLocalGraphics) - mpLocalGraphics->Flush(); - if(mpThreadGraphics) - mpThreadGraphics->Flush(); + if (mpGraphics) + mpGraphics->Flush(); GdiFlush(); } @@ -3787,9 +3711,9 @@ static bool ImplHandleKeyMsg( HWND hWnd, UINT nMsg, // reset the background mode for each text input, // as some tools such as RichWin may have changed it - if ( pFrame->mpLocalGraphics && - pFrame->mpLocalGraphics->getHDC() ) - SetBkMode( pFrame->mpLocalGraphics->getHDC(), TRANSPARENT ); + if ( pFrame->mpGraphics && + pFrame->mpGraphics->getHDC() ) + SetBkMode( pFrame->mpGraphics->getHDC(), TRANSPARENT ); // determine modifiers if ( GetKeyState( VK_SHIFT ) & 0x8000 ) @@ -4229,7 +4153,7 @@ static bool ImplHandlePaintMsg( HWND hWnd ) { // clip region must be set, as we don't get a proper // bounding rectangle otherwise - WinSalGraphics *pGraphics = pFrame->mpLocalGraphics; + WinSalGraphics *pGraphics = pFrame->mpGraphics; bool bHasClipRegion = pGraphics && pGraphics->getHDC() && pGraphics->getRegion(); if ( bHasClipRegion ) @@ -5349,9 +5273,9 @@ static bool ImplHandleIMEComposition( HWND hWnd, LPARAM lParam ) { // reset the background mode for each text input, // as some tools such as RichWin may have changed it - if ( pFrame->mpLocalGraphics && - pFrame->mpLocalGraphics->getHDC() ) - SetBkMode( pFrame->mpLocalGraphics->getHDC(), TRANSPARENT ); + if ( pFrame->mpGraphics && + pFrame->mpGraphics->getHDC() ) + SetBkMode( pFrame->mpGraphics->getHDC(), TRANSPARENT ); } if ( pFrame && pFrame->mbHandleIME )
