From d0e734297046eb0470fdf58f9ad3c001214a797f Mon Sep 17 00:00:00 2001 From: Jan-Marek Glogowski Date: Wed, 15 Sep 2021 17:24:48 +0200 Subject: WIN always (de-)init WinSalGraphics Originally I thought the whole (de-)initialization was some kind of lazy SalGraphics init stuff. But reading the existing code proved me wrong and every InitGraphics came a few lines after a setHDC call and DeInitGraphics before deletion or setHDC(nullptr). $ git grep -n "delete.*pGraph\|setHDC\|InitGraphics" vcl/win/ So just make (De-)Init part of setHDC and drop all the other calls to (De-)InitGraphics, adding a setHDC(nullptr) to the destructor. I've also added some questionable asserts, like assert(pPrinter->mpGraphics->getHDC() == pPrinter->mhDC); As I read the code, you can have a printer object initialized with a DC but without graphics, which will be initialized on demand. But AFAIK it's invalid to have a printer DC != graphics DC. Then there was the hDC check in WinSalPrinter::EndPage, which is IMHO not needed. AFAIK there is no way the graphics DC might be "magically" invalidated, so restoring the old DC values should always work. Reviewed-on: https://gerrit.libreoffice.org/c/core/+/122157 Tested-by: Jenkins Reviewed-by: Jan-Marek Glogowski (cherry picked from commit 42f9d4335bfa4b7299224801fd7ccdd97ae92fbf) Conflicts: vcl/inc/win/salgdi.h vcl/win/gdi/salprn.cxx Change-Id: I1b961cfa733263ce773575a728bcce5c7d3e97ad Reviewed-on: https://gerrit.libreoffice.org/c/core/+/134514 Tested-by: Thorsten Behrens Reviewed-by: Thorsten Behrens --- vcl/inc/win/salgdi.h | 7 ++++--- vcl/win/gdi/salgdi.cxx | 24 ++++++++++++++++++++++++ vcl/win/gdi/salprn.cxx | 36 ++++++++++++------------------------ vcl/win/gdi/salvd.cxx | 13 +++++++------ vcl/win/window/salframe.cxx | 4 +--- 5 files changed, 48 insertions(+), 36 deletions(-) diff --git a/vcl/inc/win/salgdi.h b/vcl/inc/win/salgdi.h index d6d8dea14c24..5c83afae34b7 100644 --- a/vcl/inc/win/salgdi.h +++ b/vcl/inc/win/salgdi.h @@ -170,20 +170,21 @@ private: bool CacheGlyphs(const GenericSalLayout& rLayout); bool DrawCachedGlyphs(const GenericSalLayout& rLayout); + // just call both from setHDC! + void InitGraphics(); + void DeInitGraphics(); public: HFONT ImplDoSetFont(FontSelectPattern const & i_rFont, const PhysicalFontFace * i_pFontFace, float& o_rFontScale, HFONT& o_rOldFont); HDC getHDC() const { return mhLocalDC; } - void setHDC(HDC aNew) { mhLocalDC = aNew; } + void setHDC(HDC aNew); HPALETTE getDefPal() const; void setDefPal(HPALETTE hDefPal); HRGN getRegion() const; - void InitGraphics(); - void DeInitGraphics(); enum Type { diff --git a/vcl/win/gdi/salgdi.cxx b/vcl/win/gdi/salgdi.cxx index 2d4a283bb818..328f7c247599 100644 --- a/vcl/win/gdi/salgdi.cxx +++ b/vcl/win/gdi/salgdi.cxx @@ -449,6 +449,9 @@ void ImplUpdateSysColorEntries() void WinSalGraphics::InitGraphics() { + if (!getHDC()) + return; + // calculate the minimal line width for the printer if ( isPrinter() ) { @@ -468,19 +471,38 @@ void WinSalGraphics::InitGraphics() void WinSalGraphics::DeInitGraphics() { + if (!getHDC()) + return; + // clear clip region SelectClipRgn( getHDC(), nullptr ); // select default objects if ( mhDefPen ) + { SelectPen( getHDC(), mhDefPen ); + mhDefPen = nullptr; + } if ( mhDefBrush ) + { SelectBrush( getHDC(), mhDefBrush ); + mhDefBrush = nullptr; + } if ( mhDefFont ) + { SelectFont( getHDC(), mhDefFont ); + mhDefFont = nullptr; + } mpImpl->DeInit(); } +void WinSalGraphics::setHDC(HDC aNew) +{ + DeInitGraphics(); + mhLocalDC = aNew; + InitGraphics(); +} + HDC ImplGetCachedDC( sal_uLong nID, HBITMAP hBmp ) { SalData* pSalData = GetSalData(); @@ -638,6 +660,8 @@ WinSalGraphics::~WinSalGraphics() // delete cache data delete [] reinterpret_cast(mpStdClipRgnData); + + setHDC(nullptr); } SalGraphicsImpl* WinSalGraphics::GetImpl() const diff --git a/vcl/win/gdi/salprn.cxx b/vcl/win/gdi/salprn.cxx index 83d5aa749e14..adb9f0e102d0 100644 --- a/vcl/win/gdi/salprn.cxx +++ b/vcl/win/gdi/salprn.cxx @@ -1031,7 +1031,6 @@ static WinSalGraphics* ImplCreateSalPrnGraphics( HDC hDC ) WinSalGraphics* pGraphics = new WinSalGraphics(WinSalGraphics::PRINTER, false, nullptr, /* CHECKME */ nullptr); pGraphics->SetLayout( SalLayoutFlags::NONE ); pGraphics->setHDC(hDC); - pGraphics->InitGraphics(); return pGraphics; } @@ -1043,9 +1042,9 @@ static bool ImplUpdateSalPrnIC( WinSalInfoPrinter* pPrinter, const ImplJobSetup* if ( pPrinter->mpGraphics ) { - pPrinter->mpGraphics->DeInitGraphics(); - DeleteDC( pPrinter->mpGraphics->getHDC() ); + assert(pPrinter->mpGraphics->getHDC() == pPrinter->mhDC); delete pPrinter->mpGraphics; + DeleteDC(pPrinter->mhDC); } pPrinter->mpGraphics = ImplCreateSalPrnGraphics( hNewDC ); @@ -1103,9 +1102,11 @@ WinSalInfoPrinter::~WinSalInfoPrinter() { if ( mpGraphics ) { - mpGraphics->DeInitGraphics(); - DeleteDC( mpGraphics->getHDC() ); + // we get intermittent crashes on the Windows jenkins box around here, let us see if there is something weird about the DC + SAL_WARN("vcl", "Graphics DC " << mpGraphics->getHDC()); + assert(mpGraphics->getHDC() == mhDC); delete mpGraphics; + DeleteDC(mhDC); } } @@ -1369,12 +1370,7 @@ WinSalPrinter::~WinSalPrinter() HDC hDC = mhDC; if ( hDC ) { - if ( mpGraphics ) - { - mpGraphics->DeInitGraphics(); - delete mpGraphics; - } - + delete mpGraphics; DeleteDC( hDC ); } @@ -1532,12 +1528,8 @@ bool WinSalPrinter::EndJob() HDC hDC = mhDC; if ( isValid() && hDC ) { - if ( mpGraphics ) - { - mpGraphics->DeInitGraphics(); - delete mpGraphics; - mpGraphics = nullptr; - } + delete mpGraphics; + mpGraphics = nullptr; // #i54419# Windows fax printer brings up a dialog in EndDoc // which text previously copied in soffice process can be @@ -1598,17 +1590,13 @@ SalGraphics* WinSalPrinter::StartPage( ImplJobSetup* pSetupData, bool bNewJobDat void WinSalPrinter::EndPage() { - HDC hDC = mhDC; - if ( hDC && mpGraphics ) - { - mpGraphics->DeInitGraphics(); - delete mpGraphics; - mpGraphics = nullptr; - } + delete mpGraphics; + mpGraphics = nullptr; if( ! isValid() ) return; + HDC hDC = mhDC; volatile int nRet = 0; CATCH_DRIVER_EX_BEGIN; nRet = ::EndPage( hDC ); diff --git a/vcl/win/gdi/salvd.cxx b/vcl/win/gdi/salvd.cxx index 57ad9581b1c4..d06412bc6743 100644 --- a/vcl/win/gdi/salvd.cxx +++ b/vcl/win/gdi/salvd.cxx @@ -135,7 +135,6 @@ std::unique_ptr WinSalInstance::CreateVirtualDevice( SalGraphi RealizePalette( hDC ); } - pVirGraphics->InitGraphics(); pVDev->setGraphics(pVirGraphics); return std::unique_ptr(pVDev); @@ -171,14 +170,16 @@ WinSalVirtualDevice::~WinSalVirtualDevice() if( *ppVirDev ) *ppVirDev = mpNext; - // destroy saved DC + HDC hDC = mpGraphics->getHDC(); + // restore the mpGraphics' original HDC values, so the HDC can be deleted in the !mbForeignDC case + mpGraphics->setHDC(nullptr); + if( mpGraphics->getDefPal() ) - SelectPalette( mpGraphics->getHDC(), mpGraphics->getDefPal(), TRUE ); - mpGraphics->DeInitGraphics(); + SelectPalette(hDC, mpGraphics->getDefPal(), TRUE); if( mhDefBmp ) - SelectBitmap( mpGraphics->getHDC(), mhDefBmp ); + SelectBitmap(hDC, mhDefBmp); if( !mbForeignDC ) - DeleteDC( mpGraphics->getHDC() ); + DeleteDC(hDC); } SalGraphics* WinSalVirtualDevice::AcquireGraphics() diff --git a/vcl/win/window/salframe.cxx b/vcl/win/window/salframe.cxx index aedba78e3fca..0997d3de9149 100644 --- a/vcl/win/window/salframe.cxx +++ b/vcl/win/window/salframe.cxx @@ -913,12 +913,11 @@ bool WinSalFrame::ReleaseFrameGraphicsDC( WinSalGraphics* pGraphics ) return false; if ( pGraphics->getDefPal() ) SelectPalette( hDC, pGraphics->getDefPal(), TRUE ); - pGraphics->DeInitGraphics(); + pGraphics->setHDC(nullptr); SendMessageW( pSalData->mpInstance->mhComWnd, SAL_MSG_RELEASEDC, reinterpret_cast(mhWnd), reinterpret_cast(hDC) ); if ( pGraphics == mpThreadGraphics ) pSalData->mnCacheDCInUse--; - pGraphics->setHDC(nullptr); return true; } @@ -997,7 +996,6 @@ bool WinSalFrame::InitFrameGraphicsDC( WinSalGraphics *pGraphics, HDC hDC, HWND pGraphics->setDefPal(SelectPalette( hDC, pSalData->mhDitherPal, TRUE )); RealizePalette( hDC ); } - pGraphics->InitGraphics(); if ( pGraphics == mpThreadGraphics ) pSalData->mnCacheDCInUse++; -- cgit