Re: ModemManager: MMCallbackInfo scheduled twice when device removed

[Dan Williams <dcbw redhat com>]

On Tue, 2011-05-17 at 09:57 +0200, Aleksander Morgado wrote:
> 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

All the stuff in the protect-callbacks branch looks good, please push.
I couldn't trigger crashes on a number of devices just pulling them out
in the middle of a tight loop hammering the modem for info.  Good work.

Dan