Hi,
I have submitted a patch for review:
https://gerrit.libreoffice.org/3310
To pull it, you can do:
git pull ssh://gerrit.libreoffice.org:29418/core refs/changes/10/3310/1
fdo#63391 deadlock on opening .odb file that auto-connects to the database
Let foo.odb be a database file that has a macro that connects to the
Database on "Open Document" event (and needs to prompt user for
user/password).
There was a race condition between two actions:
1) the asynchronous treatment of "OnFirstControllerConnected" in
dbaui::OApplicationController,
which tries to get dbaui::OApplicationController's mutex
2) the StarBasic macro calling dbaui::OApplicationController::connect
which needs to display a dialog (to get username and password),
and thus puts that dialog in the main thread's event queue
and waits for it ... with dbaui::OApplicationController's mutex held
Now, if "1)" is before "2)" in the event queue of the the main thread,
*but* "1)" is executed *after* "2)" has taken the lock, there is a deadlock.
Fix:
1) Make OnFirstControllerConnected synchronous.
Make sure (by taking mutex in dbaui::OApplicationController::attachFrame,
its ancestor in the call graph)
that nothing else will happen with the OApplicationController as long as it
is not finished.
---> it does not need to take mutex itself anymore
This avoids the "order in the asynchronous events" dependency.
2) Change dbaui::OApplicationController::ensureConnection to do the user
prompting
WITHOUT HOLDING the mutex, and use the mutex "only" to protect actually
assigning
the connection to m_xDataSourceConnection.
Theoretically, in some race condition, we could connect twice and then
discard one connection <shrug>.
ensureConnection will never return the discarded connection, though.
(I think I got that right with respect to
http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html)
This keeps it from locking on another condition while holding the mutex.
Change-Id: Iab1bbec5d5df12bb89d027d43e498c78c92ffc32
---
M dbaccess/source/ui/app/AppController.cxx
M dbaccess/source/ui/app/AppController.hxx
M dbaccess/source/ui/app/AppControllerDnD.cxx
M dbaccess/source/ui/app/AppControllerGen.cxx
4 files changed, 63 insertions(+), 22 deletions(-)
diff --git a/dbaccess/source/ui/app/AppController.cxx
b/dbaccess/source/ui/app/AppController.cxx
index 53b6555..c943ed3 100644
--- a/dbaccess/source/ui/app/AppController.cxx
+++ b/dbaccess/source/ui/app/AppController.cxx
@@ -304,7 +304,6 @@
,m_aTableCopyHelper(this)
,m_pClipbordNotifier(NULL)
,m_nAsyncDrop(0)
- ,m_aControllerConnectedEvent( LINK( this, OApplicationController,
OnFirstControllerConnected ) )
,m_aSelectContainerEvent( LINK( this, OApplicationController,
OnSelectContainer ) )
,m_ePreviewMode(E_PREVIEWNONE)
,m_eCurrentType(E_NONE)
@@ -361,8 +360,6 @@
//--------------------------------------------------------------------
void SAL_CALL OApplicationController::disposing()
{
- m_aControllerConnectedEvent.CancelCall();
-
::std::for_each(m_aCurrentContainers.begin(),m_aCurrentContainers.end(),XContainerFunctor(this));
m_aCurrentContainers.clear();
m_pSubComponentManager->disposing();
@@ -2644,14 +2641,12 @@
return;
}
- m_aControllerConnectedEvent.Call();
+ OnFirstControllerConnected();
}
//
-----------------------------------------------------------------------------
-IMPL_LINK( OApplicationController, OnFirstControllerConnected, void*, /**/ )
+void OApplicationController::OnFirstControllerConnected()
{
- ::osl::MutexGuard aGuard( getMutex() );
-
if ( !m_xModel.is() )
{
OSL_FAIL( "OApplicationController::OnFirstControllerConnected: too
late!" );
@@ -2665,7 +2660,7 @@
// no need to show this warning, obviously the document supports
embedding scripts
// into itself, so there are no "old-style" forms/reports which have
macros/scripts
// themselves
- return 0L;
+ return;
}
try
@@ -2674,12 +2669,12 @@
// In this case, we should not show the warning, again.
::comphelper::NamedValueCollection aModelArgs( m_xModel->getArgs() );
if ( aModelArgs.getOrDefault( "SuppressMigrationWarning", sal_False ) )
- return 0L;
+ return;
// also, if the document is read-only, then no migration is possible,
and the
// respective menu entry is hidden. So, don't show the warning in this
case, too.
if ( Reference< XStorable >( m_xModel, UNO_QUERY_THROW )->isReadonly()
)
- return 0L;
+ return;
SQLWarning aWarning;
aWarning.Message = OUString( ModuleRes( STR_SUB_DOCS_WITH_SCRIPTS ) );
@@ -2695,12 +2690,14 @@
DBG_UNHANDLED_EXCEPTION();
}
- return 1L;
+ return;
}
//
-----------------------------------------------------------------------------
void SAL_CALL OApplicationController::attachFrame( const Reference< XFrame > &
i_rxFrame ) throw( RuntimeException )
{
+ ::osl::MutexGuard aGuard( getMutex() );
+
OApplicationController_CBASE::attachFrame( i_rxFrame );
if ( getFrame().is() )
onAttachedFrame();
diff --git a/dbaccess/source/ui/app/AppController.hxx
b/dbaccess/source/ui/app/AppController.hxx
index faf029d..bf34876 100644
--- a/dbaccess/source/ui/app/AppController.hxx
+++ b/dbaccess/source/ui/app/AppController.hxx
@@ -121,8 +121,7 @@
OTableCopyHelper m_aTableCopyHelper;
TransferableClipboardListener*
m_pClipbordNotifier; // notifier for
changes in the clipboard
- sal_uLong m_nAsyncDrop;
- OAsyncronousLink m_aControllerConnectedEvent;
+ sal_uLong m_nAsyncDrop;
OAsyncronousLink m_aSelectContainerEvent;
PreviewMode m_ePreviewMode; // the mode of the
preview
ElementType m_eCurrentType;
@@ -458,6 +457,7 @@
virtual ::com::sun::star::uno::Reference<
::com::sun::star::sdbc::XConnection > SAL_CALL getActiveConnection() throw
(::com::sun::star::uno::RuntimeException);
virtual ::com::sun::star::uno::Sequence<
::com::sun::star::uno::Reference< ::com::sun::star::lang::XComponent > >
SAL_CALL getSubComponents() throw (::com::sun::star::uno::RuntimeException);
virtual ::sal_Bool SAL_CALL isConnected( ) throw
(::com::sun::star::uno::RuntimeException);
+ // DO NOT CALL with getMutex() held!!
virtual void SAL_CALL connect( ) throw
(::com::sun::star::sdbc::SQLException, ::com::sun::star::uno::RuntimeException);
virtual ::com::sun::star::beans::Pair< ::sal_Int32, OUString >
SAL_CALL identifySubComponent( const ::com::sun::star::uno::Reference<
::com::sun::star::lang::XComponent >& SubComponent ) throw
(::com::sun::star::lang::IllegalArgumentException,
::com::sun::star::uno::RuntimeException);
virtual ::sal_Bool SAL_CALL closeSubComponents( ) throw
(::com::sun::star::uno::RuntimeException);
@@ -480,6 +480,8 @@
If an error occurs, then this is either stored in the location
pointed to by <arg>_pErrorInfo</arg>,
or, if <code>_pErrorInfo</code> is <NULL/>, then the error is
displayed to the user.
+
+ DO NOT CALL with getMutex() held!!
*/
const SharedConnection& ensureConnection( ::dbtools::SQLExceptionInfo*
_pErrorInfo = NULL );
@@ -541,7 +543,7 @@
DECL_LINK( OnAsyncDrop, void* );
DECL_LINK( OnCreateWithPilot, void* );
DECL_LINK( OnSelectContainer, void* );
- DECL_LINK( OnFirstControllerConnected, void* );
+ void OnFirstControllerConnected();
protected:
using OApplicationController_CBASE::connect;
diff --git a/dbaccess/source/ui/app/AppControllerDnD.cxx
b/dbaccess/source/ui/app/AppControllerDnD.cxx
index f0623e7..8ac9711 100644
--- a/dbaccess/source/ui/app/AppControllerDnD.cxx
+++ b/dbaccess/source/ui/app/AppControllerDnD.cxx
@@ -327,20 +327,63 @@
}
}
//
-----------------------------------------------------------------------------
+// DO NOT CALL with getMutex() held!!
const SharedConnection& OApplicationController::ensureConnection(
::dbtools::SQLExceptionInfo* _pErrorInfo )
{
- SolarMutexGuard aSolarGuard;
- ::osl::MutexGuard aGuard( getMutex() );
- if ( !m_xDataSourceConnection.is() )
+ // This looks like double checked locking, but it is not,
+ // because every access (read *or* write) to m_xDataSourceConnection
+ // is mutexed.
+ // See
http://www.cs.umd.edu/~pugh/java/memoryModel/DoubleCheckedLocking.html
+ // for what I'm refering to.
+ // We cannot use the TLS (thread-local storage) solution
+ // since support for TLS is not up to the snuff on Windows :-(
+
{
- WaitObject aWO(getView());
+ ::osl::MutexGuard aGuard( getMutex() );
+
+ if ( m_xDataSourceConnection.is() )
+ return m_xDataSourceConnection;
+ }
+
+ WaitObject aWO(getView());
+ Reference<XConnection> conn;
+ {
+ SolarMutexGuard aSolarGuard;
+
OUString sConnectingContext( ModuleRes( STR_COULDNOTCONNECT_DATASOURCE
) );
sConnectingContext = sConnectingContext.replaceFirst("$name$",
getStrippedDatabaseName());
- m_xDataSourceConnection.reset( connect( getDatabaseName(),
sConnectingContext, _pErrorInfo ) );
+ // do the connection *without* holding getMutex() to avoid deadlock
+ // when we are not in the main thread and we need username/password
+ // (and thus to display a dialog, which will be done by the main
thread)
+ // and there is an event that needs getMutex() *before* us in the main
thread's queue
+ // See fdo#63391
+ conn.set( connect( getDatabaseName(), sConnectingContext, _pErrorInfo
) );
+ }
+
+ if (conn.is())
+ {
+ ::osl::MutexGuard aGuard( getMutex() );
if ( m_xDataSourceConnection.is() )
{
+ Reference< XComponent > comp (conn, UNO_QUERY);
+ if(comp.is())
+ {
+ try
+ {
+ comp->dispose();
+ }
+ catch( const Exception& )
+ {
+ OSL_FAIL( "dbaui::OApplicationController::ensureConnection
could not dispose of temporary unused connection" );
+ }
+ }
+ conn.clear();
+ }
+ else
+ {
+ m_xDataSourceConnection.reset(conn);
SQLExceptionInfo aError;
try
{
@@ -362,11 +405,13 @@
}
else
{
+ SolarMutexGuard aSolarGuard;
showError( aError );
}
}
}
}
+
return m_xDataSourceConnection;
}
//
-----------------------------------------------------------------------------
diff --git a/dbaccess/source/ui/app/AppControllerGen.cxx
b/dbaccess/source/ui/app/AppControllerGen.cxx
index c308d96..271a6b8 100644
--- a/dbaccess/source/ui/app/AppControllerGen.cxx
+++ b/dbaccess/source/ui/app/AppControllerGen.cxx
@@ -377,9 +377,6 @@
//
-----------------------------------------------------------------------------
void SAL_CALL OApplicationController::connect( ) throw (SQLException,
RuntimeException)
{
- SolarMutexGuard aSolarGuard;
- ::osl::MutexGuard aGuard( getMutex() );
-
SQLExceptionInfo aError;
SharedConnection xConnection = ensureConnection( &aError );
if ( !xConnection.is() )
--
To view, visit https://gerrit.libreoffice.org/3310
To unsubscribe, visit https://gerrit.libreoffice.org/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Iab1bbec5d5df12bb89d027d43e498c78c92ffc32
Gerrit-PatchSet: 1
Gerrit-Project: core
Gerrit-Branch: master
Gerrit-Owner: Lionel Elie Mamane <[email protected]>
_______________________________________________
LibreOffice mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/libreoffice
