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.me...@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 );
_______________________________________________
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/libreoffice
