Re: ModemManager: MMCallbackInfo scheduled twice when device removed

[Dan Williams <dcbw redhat com>]

On Wed, 2011-05-11 at 16:20 +0200, Aleksander Morgado wrote:
> Hi all,
> 
> Each MMCallbackInfo holds a weak reference to the MMModem to which the
> AT command was sent. When the MMModem is destroyed,
> mm-callback-info.c::modem_destroyed_cb() gets called and the modem
> pointer in the callback info is reset to NULL, to avoid having a pointer
> to an already disposed MMModem. See [1].
> 
> But, in addition to setting the modem pointer to NULL, an explicit error
> is just set directly in the callback info, and it gets scheduled. The
> problem here is that after destroying the modem, the ports it held also
> get closed as part of the destruction, and during port closing there's
> the task of finalizing all pending commands (see
> mm-serial-port.c::mm_serial_port_close()), which is done by passing an
> error to the specific response handler, and which will then (usually)
> propagate that error to the MMCallbackInfo and schedule it right away.
> 
> That ends up in a memory leak (most response handlers will just set
> info->error without assuming there may already be a GError set); plus a
> critical in the second call to mm_callback_info_schedule() because a
> callback info can't be scheduled twice.
> 
> I've been looking at this problem some time, and the best way to fix it
> seems to be to just avoid setting the first error and scheduling in
> modem_destroyed_cb():
> 
> diff --git a/src/mm-callback-info.c b/src/mm-callback-info.c
> index 1986bb5..8f8ecca 100644
> --- a/src/mm-callback-info.c
> +++ b/src/mm-callback-info.c
> @@ -53,13 +53,9 @@ modem_destroyed_cb (gpointer data, GObject
> *destroyed)
>  {
>      MMCallbackInfo *info = data;
>  
> +    /* Just set the modem pointer to NULL, do not further process the
> +     * callback info */
>      info->modem = NULL;
> -    if (!info->pending_id) {
> -        info->error = g_error_new_literal (MM_MODEM_ERROR,
> -                                           MM_MODEM_ERROR_REMOVED,
> -                                           "The modem was removed.");
> -        mm_callback_info_schedule (info);
> -    }
>  }
> 
> Comments?

How does that play into the mm_modem_check_removed() checks which I
think are one of the more common places this error is looked for?  I
think the motivation here was to be *sure* that any callback handler
waiting for the result got an error.  The thing we want to ensure here
is that if a D-Bus client of MM made a request when the modem was pulled
out or crashed (which can happen pretty often) that the client gets a
reply instead of a timed-out D-Bus method return error.  I'm sure
there's room for cleanup here, but that was the original motivation.  Do
we just need to audit the callback handlers to make sure they do the
right thing with a ERROR_REMOVED?

Dan