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

[Havoc Pennington]

John (J5) Palmieri wrote:
> @@ -3734,6 +3734,10 @@
>                                    "Disconnected"))
>          {
>            _dbus_bus_check_connection_and_unref (connection);
> +
> +          /* remove all active pending calls */
> +          _dbus_hash_table_remove_all (connection->pending_replies);
> +

Here we just delete the pending calls. But it might be nicer to call the 
notify on all of them, with an error message, don't you think?
Then people blocking on a pending call don't have to also watch for 
disconnection - the pending call notifier is guaranteed to be called.

One way to do this; in the place that currently queues the 
disconnect_message_link, also cancel all the pending call timeouts and 
queue all the timeout errors, then queue the disconnect_message_link.

Then, be sure we warn or otherwise prevent adding new pending calls 
while disconnected.

This should guarantee that no pending calls remain in the point above 
(when processing the disconnected message) since it guarantees that 
there's always a reply to all pending calls, before the disconnect 
message arrives.

Does this make sense? I'm not sure I thought about all the angles.

>  
>    _dbus_assert (timeout_milliseconds >= 0 || timeout_milliseconds == -1);
>    
> +  if (connection != NULL && !dbus_connection_get_is_connected (connection))
> +    return NULL;
> + 

Is calling this with a null connection really allowed? What does it mean 
in that case?

I think the above get_is_connected() might make more sense as an 
assertion/warning here, and have dbus-connection.c not call this 
function if it's not connected.

It looks to me like dbus_connection_send_with_reply() needs more thought 
about what happens if it's called while disconnected. The options:
  - it's a return_if_fail - callers must check this (seems painful
    for app developers, also could not be checked in a threadsafe
    way)
  - it silently does nothing - doesn't really work since it returns
    the pending call object, and we'd have none to return
  - it creates a pending call and immediately times it out - but
    this is a disaster since it doesn't make sense to queue error
    replies after disconnected is already received
  - we add a DBusError and throw an error if you call while
    disconnected... but this sounds like an annoying api break
    and most people would just ignore the error
  - we return a pending call with an error reply already set...
    but then when is the notify function called? synchronously
    when it gets set?

my best thought right now is that we return TRUE (had enough memory) but 
return NULL for the pending call? But existing apps might crash in that 
case.

I'm not sure this works at all for existing apps right now - pending 
calls returned after disconnect might do something nasty like block 
forever even??? Or maybe something sane happens to work out?

>    if (timeout_milliseconds == -1)
>      timeout_milliseconds = _DBUS_DEFAULT_TIMEOUT_VALUE;
>  
> @@ -119,8 +122,12 @@
>    
>    pending->refcount.value = 1;
>    pending->connection = connection;
> +  if (pending->connection != NULL)
> +    dbus_connection_ref (pending->connection);
> +

(same here about null connection not allowed?)

Hmm. Maybe set up a test case for calling send_with_reply after 
disconnect, and see what happens right now and then we can also verify 
the new behavior we decide is right...

Havoc