mir/libreoffice
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

fdo#63391 deadlock on opening .odb file that auto-connects to the database

Browse Source 
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
Reviewed-on: https://gerrit.libreoffice.org/3310
Reviewed-by: David Tardon <dtardon@redhat.com>
Tested-by: David Tardon <dtardon@redhat.com>
This commit is contained in:
Lionel Elie Mamane 2013-04-10 18:56:01 +02:00 committed by David Tardon
parent 8c91339211
commit 1981819e81
4 changed files with 63 additions and 22 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

19
dbaccess/source/ui/app/AppController.cxx
View File

@ -304,7 +304,6 @@ OApplicationController::OApplicationController(const Reference< XComponentContex
,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 OApplicationController::disconnect()
//--------------------------------------------------------------------
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 @@ void OApplicationController::onAttachedFrame()
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 @@ IMPL_LINK( OApplicationController, OnFirstControllerConnected, void*, /**/ )
// 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 @@ IMPL_LINK( OApplicationController, OnFirstControllerConnected, void*, /**/ )
// 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 @@ IMPL_LINK( OApplicationController, OnFirstControllerConnected, void*, /**/ )
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();

8
dbaccess/source/ui/app/AppController.hxx
View File

@ -121,8 +121,7 @@ namespace dbaui
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 @@ namespace dbaui
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 @@ namespace dbaui
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 @@ namespace dbaui
DECL_LINK( OnAsyncDrop, void* );
DECL_LINK( OnCreateWithPilot, void* );
DECL_LINK( OnSelectContainer, void* );
DECL_LINK( OnFirstControllerConnected, void* );
void OnFirstControllerConnected();
protected:
using OApplicationController_CBASE::connect;

55
dbaccess/source/ui/app/AppControllerDnD.cxx
View File

@ -327,20 +327,63 @@ void OApplicationController::deleteEntries()
}
}
// -----------------------------------------------------------------------------
// 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 @@ const SharedConnection& OApplicationController::ensureConnection( ::dbtools::SQL
}
else
{
SolarMutexGuard aSolarGuard;
showError( aError );
}
}
}
}
return m_xDataSourceConnection;
}
// -----------------------------------------------------------------------------

3
dbaccess/source/ui/app/AppControllerGen.cxx
View File

@ -377,9 +377,6 @@ Reference< XConnection > SAL_CALL OApplicationController::getActiveConnection()
// -----------------------------------------------------------------------------
void SAL_CALL OApplicationController::connect( ) throw (SQLException, RuntimeException)
{
SolarMutexGuard aSolarGuard;
::osl::MutexGuard aGuard( getMutex() );
SQLExceptionInfo aError;
SharedConnection xConnection = ensureConnection( &aError );
if ( !xConnection.is() )