[PATCH] Have pending call keep a ref to its connection (fixes the NetworkManager crash)

[Havoc Pennington]

John (J5) Palmieri wrote:
> I opted to send TRUE and set the pending_reply = NULL.  If this breaks
> apps they should have been checking for it in the first place (and now
> is the time to break apps).  It is the simplest code path and easy to
> fix in apps that are broken.

Sounds fine, no better ideas from me. I'd be sure it's in the docs though.

>> +
>> +  CONNECTION_LOCK (connection);
>> +
>> +   if (!_dbus_connection_get_is_connected_unlocked (connection))
>> +    {
>> +      /* throw a warning here so we can see if this happens often 
>> +         and perhaps handle it in a better way (e.g. throw an error in
>> +         the pending call) */
>> +      _dbus_warn ("dbus_connection_send_with_reply called on a disconnected connection");
>> +      CONNECTION_UNLOCK (connection);
>> +
>> +      *pending_return = NULL;
>> +
>> +      return TRUE;
>> +    }

I don't think we should warn *and* document that it sets the pending 
call to null; otherwise people can do something legitimate that has a 
warning they can't get rid of. If it warns it should be due to someone 
making a programming error. There is no thread-safe way right now 
someone could avoid sending messages on a disconnected connection.

>>  static void
>> +connection_timeout_and_complete_all_pending_calls (DBusConnection *connection)
>> +{
>> +  DBusHashIter iter;
>> +
>> +  _dbus_hash_iter_init (connection->pending_replies, &iter);
>> +
>> +  /* create list while we remove the iters from the hash
>> +     because we need to go over it a couple of times */
>> +  while (_dbus_hash_iter_next (&iter))
>> +    {
>> +      DBusPendingCall *pending;
>> +      DBusDispatchStatus status;  
>> + 
>> +      pending = (DBusPendingCall *) _dbus_hash_iter_get_value (&iter);
>> +      dbus_pending_call_ref (pending);
>> +     
>> +      _dbus_pending_call_queue_timeout_error (pending, 
>> +                                              connection);
>> +      _dbus_connection_remove_timeout_unlocked (connection,
>> +                                                _dbus_pending_call_get_timeout (pending));
>> +
>> +      status = _dbus_connection_get_dispatch_status_unlocked (connection);
>> +
>> +      /* Unlocks, and calls out to user code */
>> +      _dbus_connection_update_dispatch_status_and_unlock (connection, status);
>> +
>> +      /* lock it again */
>> +      CONNECTION_LOCK (connection);
>> +   
>> +      _dbus_hash_iter_remove_entry (&iter);
>> +
>> +      dbus_pending_call_unref (pending);
>> +      
>> +    }

The unlocking and updating dispatch status can be outside the loop I 
think, in fact it should be a lot safer and more efficient that way.

>>                                    DBUS_INTERFACE_LOCAL,
>>                                    "Disconnected"))
>>          {
>> +          /* timeout all active pending calls */
>> +          connection_timeout_and_complete_all_pending_calls (connection);
>> +

This will put the timeouts *after* the disconnect message; instead, 
should time them all out just before where we currently *queue* the 
disconnect message.

>> +  if (pending->connection != NULL)
>> +    dbus_connection_ref (pending->connection);
>> +

This != null check should be obsolete right?

Havoc