[vlc-devel] commit: dbus : fix a deadlock with p_events array ( Rafaël Carré )

[git version control]

vlc | branch: 1.0-bugfix | Rafaël Carré <rafael.carre at gmail.com> | Wed May 27 13:56:02 2009 +0200| [f193adfbec380ac312c2f1a476a38e3172b7e993] | committer: Rafaël Carré 

dbus : fix a deadlock with p_events array

Mention that MPRIS is at version 1.0
Remove some trailing spaces
Remove unused argument 'lock' from MarshalStatus()

> http://git.videolan.org/gitweb.cgi/vlc.git/?a=commit;h=f193adfbec380ac312c2f1a476a38e3172b7e993
---

 modules/control/dbus.c |   47 ++++++++++++++++++++++++++++++-----------------
 1 files changed, 30 insertions(+), 17 deletions(-)

diff --git a/modules/control/dbus.c b/modules/control/dbus.c
index 2753140..1e73594 100644
--- a/modules/control/dbus.c
+++ b/modules/control/dbus.c
@@ -32,7 +32,7 @@
  *  extract:
  *   "If you use this low-level API directly, you're signing up for some pain."
  *
- * MPRIS Specification (still drafting on Jan, 23 of 2008):
+ * MPRIS Specification version 1.0
  *      http://wiki.xmms2.xmms.se/index.php/MPRIS
  */
 
@@ -73,7 +73,7 @@ static int TrackListChangeEmit( intf_thread_t *, int, int );
 static int AllCallback( vlc_object_t*, const char*, vlc_value_t, vlc_value_t, void* );
 
 static int GetInputMeta ( input_item_t *, DBusMessageIter * );
-static int MarshalStatus ( intf_thread_t *, DBusMessageIter *, bool );
+static int MarshalStatus ( intf_thread_t *, DBusMessageIter * );
 static int UpdateCaps( intf_thread_t* );
 
 /* GetCaps() capabilities */
@@ -311,7 +311,7 @@ DBUS_METHOD( GetStatus )
     REPLY_INIT;
     OUT_ARGUMENTS;
 
-    MarshalStatus( p_this, &args, true );
+    MarshalStatus( p_this, &args );
 
     REPLY_SEND;
 }
@@ -838,12 +838,28 @@ static void Run          ( intf_thread_t *p_intf )
         int canc = vlc_savecancel();
         dbus_connection_read_write_dispatch( p_intf->p_sys->p_conn, 0 );
 
-        // Get the messages
+        /* Get the list of events to process
+         *
+         * We can't keep the lock on p_intf->p_sys->p_events, else we risk a
+         * deadlock:
+         * The signal functions could lock mutex X while p_events is locked;
+         * While some other function in vlc (playlist) might lock mutex X
+         * and then set a variable which would call AllCallback(), which itself
+         * needs to lock p_events to add a new event.
+         */
         vlc_mutex_lock( &p_intf->p_sys->lock );
-        for( int i = vlc_array_count( p_intf->p_sys->p_events ) - 1; i >= 0; i-- )
+        int i_events = vlc_array_count( p_intf->p_sys->p_events );
+        callback_info_t* info[i_events];
+        for( int i = i_events - 1; i >= 0; i-- )
         {
-            callback_info_t* info = vlc_array_item_at_index( p_intf->p_sys->p_events, i );
-            switch( info->signal )
+            info[i] = vlc_array_item_at_index( p_intf->p_sys->p_events, i );
+            vlc_array_remove( p_intf->p_sys->p_events, i );
+        }
+        vlc_mutex_unlock( &p_intf->p_sys->lock );
+
+        for( int i = 0; i < i_events; i++ )
+        {
+            switch( info[i]->signal )
             {
             case SIGNAL_ITEM_CURRENT:
                 TrackChange( p_intf );
@@ -851,7 +867,7 @@ static void Run          ( intf_thread_t *p_intf )
             case SIGNAL_INTF_CHANGE:
             case SIGNAL_PLAYLIST_ITEM_APPEND:
             case SIGNAL_PLAYLIST_ITEM_DELETED:
-                TrackListChangeEmit( p_intf, info->signal, info->i_node );
+                TrackListChangeEmit( p_intf, info[i]->signal, info[i]->i_node );
                 break;
             case SIGNAL_RANDOM:
             case SIGNAL_REPEAT:
@@ -859,15 +875,13 @@ static void Run          ( intf_thread_t *p_intf )
                 StatusChangeEmit( p_intf );
                 break;
             case SIGNAL_STATE:
-                StateChange( p_intf, info->i_input_state );
+                StateChange( p_intf, info[i]->i_input_state );
                 break;
             default:
                 assert(0);
             }
-            free( info );
-            vlc_array_remove( p_intf->p_sys->p_events, i );
+            free( info[i] );
         }
-        vlc_mutex_unlock( &p_intf->p_sys->lock );
         vlc_restorecancel( canc );
     }
 }
@@ -931,7 +945,7 @@ DBUS_SIGNAL( CapsChangeSignal )
 }
 
 /******************************************************************************
- * TrackListChange: tracklist order / length change signal 
+ * TrackListChange: tracklist order / length change signal
  *****************************************************************************/
 DBUS_SIGNAL( TrackListChangeSignal )
 { /* emit the new tracklist lengh */
@@ -1005,7 +1019,7 @@ DBUS_SIGNAL( StatusChangeSignal )
 
     /* we're called from a callback of input_thread_t, so it can not be
      * destroyed before we return */
-    MarshalStatus( (intf_thread_t*) p_data, &args, false );
+    MarshalStatus( (intf_thread_t*) p_data, &args );
 
     SIGNAL_SEND;
 }
@@ -1119,7 +1133,7 @@ static int UpdateCaps( intf_thread_t* p_intf )
     intf_sys_t* p_sys = p_intf->p_sys;
     dbus_int32_t i_caps = CAPS_CAN_HAS_TRACKLIST;
     playlist_t* p_playlist = pl_Hold( p_intf );
-    
+
     PL_LOCK;
     if( p_playlist->current.i_size > 0 )
         i_caps |= CAPS_CAN_PLAY | CAPS_CAN_GO_PREV | CAPS_CAN_GO_NEXT;
@@ -1233,8 +1247,7 @@ static int GetInputMeta( input_item_t* p_input,
  * MarshalStatus: Fill a DBusMessage with the current player status
  *****************************************************************************/
 
-static int MarshalStatus( intf_thread_t* p_intf, DBusMessageIter* args,
-                          bool lock )
+static int MarshalStatus( intf_thread_t* p_intf, DBusMessageIter* args )
 { /* This is NOT the right way to do that, it would be better to sore
      the status information in p_sys and update it on change, thus
      avoiding a long lock */