GNOME Bugzilla – Bug 672063
Crash on hangup in audio-only video call
Last modified: 2012-03-16 10:48:45 UTC
- Kill empathy-call - start a video call from another account - click on the contact list - accept the call but without video from the dialog - hang up the call - empathy crashes as priv->video_preview_sink is uninitialized and empathy_call_window_bus_message() is doing CAST on it
+ Trace 229874
Created attachment 209747 [details] [review] Fix crash on hangup with no video Insufficient refcount on priv->video_preview_sink caused segfaults when accessing freed data.
Review of attachment 209747 [details] [review]: ::: src/empathy-call-window.c @@ +1085,3 @@ + { + g_object_set (priv->video_preview_sink, "texture", preview, NULL); + gst_object_ref (priv->video_preview_sink); Aren't we leaking this reference now? But I don't understand, are we still creating the video preview in audio only calls? Where is the ref going away in audio call but not in video ones? @@ +1090,2 @@ else + g_error ("Missing cluttersink"); According to our coding style the if and else block have to be consistent: if one is using { } then other one should too.
Review of attachment 209747 [details] [review]: ::: src/empathy-call-window.c @@ +1085,3 @@ + { + g_object_set (priv->video_preview_sink, "texture", preview, NULL); + gst_object_ref (priv->video_preview_sink); I dont' think so, we take ownership of the object or empathy_call_window_reset_pipeline() will free the pipeline and leave us with garbled data. For the viedo preview creation, we currently do not instatiate it if the window is created for an audio call, but if we do a video call the video preview is never disabled.
Created attachment 209819 [details] [review] Added a comment for the video_preview_sink ownership and fixed a coding style issue.
(In reply to comment #4) > Review of attachment 209747 [details] [review]: > > ::: src/empathy-call-window.c > @@ +1085,3 @@ > + { > + g_object_set (priv->video_preview_sink, "texture", preview, NULL); > + gst_object_ref (priv->video_preview_sink); > > I dont' think so, we take ownership of the object or > empathy_call_window_reset_pipeline() will free the pipeline and leave us with > garbled data. How I understand it (without your patch): create_video_preview() creates the sink and add_video_preview_to_pipeline() passes the floating ref to the pipeline. So when the pipeline is destroyed the sink is destroyed as well. But with your patch we take an extra ref which is never released -> leaked. Wouldn't be it be cleaner to NULL the sink when the sink is destroyed and make empathy_call_window_bus_message() NULL-safe? > For the viedo preview creation, we currently do not instatiate it if the window > is created for an audio call, but if we do a video call the video preview is > never disabled. Right; so I think making this code NULL-safe is probably a good idea.
Created attachment 209862 [details] [review] Take a weak ref to the sink and clean up when the pipeline is freed
Review of attachment 209862 [details] [review]: ::: src/empathy-call-window.c @@ +1093,3 @@ + g_error ("Missing cluttersink, check your clutter-gst installation"); + g_object_set (priv->video_preview_sink, "texture", preview, NULL); + g_object_weak_ref (G_OBJECT (priv->video_preview_sink), remove_video_preview_sink, self); Thanks, I prefer this approach. You can make it even simpler using g_object_add_weak_pointer().
Created attachment 209912 [details] [review] Fix crash on hangup with no video Patch simplified using g_object_add_weak_pointer
Attachment 209912 [details] pushed as 7b12eea - Fix crash on hangup with no video