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 <glogow@fbihome.de>
(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 <thorsten.behrens@allotropia.de>
Reviewed-by: Thorsten Behrens <thorsten.behrens@allotropia.de>

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<BYTE*>(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<SalVirtualDevice> WinSalInstance::CreateVirtualDevice( SalGraphi
         RealizePalette( hDC );
     }
 
-    pVirGraphics->InitGraphics();
     pVDev->setGraphics(pVirGraphics);
 
     return std::unique_ptr<SalVirtualDevice>(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<WPARAM>(mhWnd), reinterpret_cast<LPARAM>(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++;