Re: --headless broken with --enable-headless

Hi Stephan,

On Wed, 2012-12-12 at 13:12 +0100, Stephan Bergmann wrote:
> On 10/01/2012 07:40 PM, Riccardo Magliocchetti wrote:
> > nAcquireCount is always 0 and the patch above let me run two unoconv
> > testsuite runs in a row without valgrind logs.
>
> It indeed looks like Application::Yield shall be routinely called with 
> the solar mutex unlocked, and that timer callbacks called from within 
> Yield shall be called with the solar mutex locked

        Hmm; that really seems broken to me - we shouldn't really call anything
in vcl without the solarmutex held IMHO.

> sal_gtk_timeout_dispatch (vcl/unx/gtk/app/gtkdata.cxx; when using the 
> GTK VCL plug-in) explicitly locks the solar mutex  before calling the 
> timer callback in a stack like

        Sure sure - I appreciate that; I'm not sure it's correct myself - it
may well be to work around the equivalent of a missing:

> Now, the interesting thing is that the Linux-specific CreateSalInstance 
> (called from ImplSVMain -> InitVCL before calling the application's Main 
> in ImplSVMain) explicitly locks the solar mutex thanks to 
> <http://cgit.freedesktop.org/libreoffice/core/commit/?id=852574f46f686a936a1b267e5780ca17d0f0d5ab>
>  
> "INTEGRATION: CWS ause0c2 (1.3.18); FILE MERGED: 2004/03/12 15:25:43 hjs 
> 1.3.18.1: #115868# anti freeze," while all the other variants of 
> CreateSalInstance apparently do not (incl. the one in 
> vcl/headless/headlessinst.cxx that is relevant when configured 
> --enable-headless, as is the case for this mail thread).

        That looks like the interesting bug to me.

>  That means that all of desktop::Desktop::Main -> Application::Execute -> 
> Application::Yield runs with the solar mutex locked on Linux (sans 
> --enable-headless configuration) and with the solar mutex unlocked 
> everywhere else.

        And that's what worries me - IMHO that is a bug; and one I'd like to
see fixed across platforms.

> The commit message for 852574f46f686a936a1b267e5780ca17d0f0d5ab is 
> unfortunately rather unhelpful, but temporarily reverting it and doing 
> "make check" on a Linux box starts to produce crashes within 
> desktop::Desktop::Main -> Application::Execute -> Application::Yield -> 
> ..., in code that apparently expects the solar mutex to be locked.

        Sure - the solar mutex should be protecting VCL during our startup /
bootstrap, and if we're not acquiring it as we create that thing
then ... life is going to be really bad IMHO.

        So - I propose something like the attached; at least for Beta2 - so we
can see what the effect is - I hope a positive one, particularly on
startup / migration / etc.

        Thoughts ?

                Michael.

-- 
michael.meeks@suse.com  <><, Pseudo Engineer, itinerant idiot

diff --git a/vcl/headless/headlessinst.cxx b/vcl/headless/headlessinst.cxx
index 439de86..31a1293 100644
--- a/vcl/headless/headlessinst.cxx
+++ b/vcl/headless/headlessinst.cxx
@@ -126,7 +126,6 @@ SalInstance *CreateSalInstance()
 
 void DestroySalInstance( SalInstance *pInst )
 {
-    pInst->ReleaseYieldMutex();
     delete pInst;
 }
 
diff --git a/vcl/source/app/svmain.cxx b/vcl/source/app/svmain.cxx
index f2135b2..91cb00b 100644
--- a/vcl/source/app/svmain.cxx
+++ b/vcl/source/app/svmain.cxx
@@ -274,6 +274,10 @@ sal_Bool InitVCL()
     pSVData->mpDefInst = CreateSalInstance();
     if ( !pSVData->mpDefInst )
         return sal_False;
+
+    // acquire SolarMutex
+    pSVData->mpDefInst->AcquireYieldMutex( 1 );
+
     RTL_LOGFILE_CONTEXT_TRACE( aLog, "} ::CreateSalInstance" );
 
     // Desktop Environment context (to be able to get value of "system.desktop-environment" as 
soon as possible)
@@ -569,6 +573,9 @@ void DeInitVCL()
     delete pSVData->mpSalTimer;
     pSVData->mpSalTimer = NULL;
 
+    // release SolarMutex
+    pSVData->mpDefInst->ReleaseYieldMutex();
+
     // Sal deinitialisieren
     DestroySalInstance( pSVData->mpDefInst );
 
diff --git a/vcl/unx/generic/plugadapt/salplug.cxx b/vcl/unx/generic/plugadapt/salplug.cxx
index 2b6b31f..ece83bb 100644
--- a/vcl/unx/generic/plugadapt/salplug.cxx
+++ b/vcl/unx/generic/plugadapt/salplug.cxx
@@ -240,17 +240,11 @@ SalInstance *CreateSalInstance()
         _exit( 1 );
     }
 
-    // acquire SolarMutex
-    pInst->AcquireYieldMutex( 1 );
-
     return pInst;
 }
 
 void DestroySalInstance( SalInstance *pInst )
 {
-    // release SolarMutex
-    pInst->ReleaseYieldMutex();
-
     delete pInst;
     if( pCloseModule )
         osl_unloadModule( pCloseModule );