Re: ModemManager: MMCallbackInfo scheduled twice when device removed

[Aleksander Morgado <aleksander lanedo com>]

Hi Dan,

> > > 
> > > 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.
> > > 


Got the new approach developed in the 'protect-callbacks' branch in the
following Gitorious repo, ready for review:
git://gitorious.org/lanedo/modemmanager.git
 
It includes the following fixes:

 * A new mm_callback_info_check_modem_removed() to be used instead of
mm_modem_check_removed(). All response callbacks should use this new
method to check whether info->modem is NULL and just return if so,
without scheduling the info.
https://gitorious.org/lanedo/modemmanager/commit/a3d9298391b39445755b230144b1fc5cda5c60ed

 * A fix in modem_destroyed_cb() to ensure that ERROR_REMOVED is always
set.
https://gitorious.org/lanedo/modemmanager/commit/3acce64ad591a1a436f2ea29c85f706a2da24e4c

 * A fix in several plugins to avoid using a custom DisableInfo
structure, and use a MMCallbackInfo instead. I ended up doing this using
a custom invoke method, which calls parent disable passing the callback,
instead of the set_data()/get_data() approach. Not sure if you'll like
this :-).
https://gitorious.org/lanedo/modemmanager/commit/19395ffa8f5a5927bca2e441cd135b72248c9a15


Cheers!

-- 
Aleksander