[PATCH] fdo#63391 deadlock on opening .odb file that auto-connects t...

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 <lionel@mamane.lu>