GNOME Bugzilla – Bug 671931
Crash receiving a call on a already disconnected empathy call window
Last modified: 2012-03-16 10:21:38 UTC
+ Trace 229855
Created attachment 209519 [details] Log from gabble
Created attachment 209520 [details] Log from empathy-call
I learned how to replicate it while doing some test for bug #671722. - A@gmail.com is set as invisible and starts a call from the web ui for B@gmail.com, who uses empathy - B@gmail.com accept the calls - in the web ui A@gmail.com sees the call as dropped per bug #671722, while empathy still waits with the "hangup" button shown - A@gmail.com starts a new call - B@gmail.com accept the calls - empathy crashes as there already is a video element instantiated and the assertion in create_video_output_widget() fails
*** Bug 671354 has been marked as a duplicate of this bug. ***
Created attachment 209618 [details] [review] Disconnect when a call is ENDED If for some unspecified reason a call go to the TP_CALL_STATE_ENDED state, reinitialize the call window.
Created attachment 209619 [details] [review] Don't restart a call if one already in progress Fixes the crash when calling someone while being already in a call with the same contact.
Review of attachment 209618 [details] [review]: ::: src/empathy-call-window.c @@ +3193,3 @@ + else { + DEBUG ("Call ended: %s", (reason != NULL && reason[0] != '\0') ? reason : "unspecified reason"); + empathy_call_window_disconnected (self, TRUE); Shouldn't we do this in the balance case as well? It's not that different, we just display a specific error for it.
Review of attachment 209619 [details] [review]: ++
Created attachment 209680 [details] [review] Disconnect also on the insufficient balance case.
Review of attachment 209680 [details] [review]: ::: src/empathy-call-window.c @@ +3193,3 @@ + DEBUG ("Call ended: %s", (reason != NULL && reason[0] != '\0') ? reason : "unspecified reason"); + empathy_call_window_disconnected (self, TRUE); + if (state == TP_CALL_STATE_ENDED && no need to re-check the state twice
Created attachment 209682 [details] [review] Removed redundant state check.
Review of attachment 209682 [details] [review]: empathy_call_window_disconnected() is called in a few other place. Did you check if this patch wasn't introducing redundant calls (and so redundant work) when the channel was closed or when manually hanging up?
Created attachment 209783 [details] [review] Create outgoing call windows in RINGING state This commit is needed for 'Don't restart a call if one already in progress', otherwise outgoing calls will not have the call handler attached.
Created attachment 209784 [details] [review] Don't restart a call if one already in progress Fixes the crash when calling someone while being already in a call with the same contact.
Created attachment 209786 [details] [review] Re-spin, disconnect when a call is ENDED, removing a call to empathy_call_window_disconnected(). I can't say if the other calls are still needed, likely they may be replaced by something making the call go to the ENDED state. Also, all calls to empathy_call_window_disconnected() have the 'restart' parameter set to TRUE, it may make sense to remove it altogether.
Review of attachment 209783 [details] [review]: Hum either I don't understand this patch or the commit message is wrong. Shouldn't it be "Create *incoming* call windows in RINGING state" ? Also, so rational about this change in the long commit message would be welcome.
Review of attachment 209784 [details] [review]: ::: src/empathy-call-window.c @@ +2441,3 @@ + + if (self->priv->call_state == CONNECTING || + self->priv->call_state == CONNECTED) What about the ringing state? Do we we really want to start a new call in this case?
Review of attachment 209786 [details] [review]: ::: src/empathy-call-window.c @@ -4008,3 @@ empathy_call_handler_stop_call (self->priv->handler); - - empathy_call_window_disconnected (self, TRUE); Just to be sure; we now rely on stop_call() to change the state of ENDED to call window_disconnected() right? May be worth a comment as this code is getting pretty hairy.
Review of attachment 209783 [details] [review]: Oops, of course you're right, I wrote the wrong commit message.
Created attachment 209820 [details] [review] Added a commend about relying on the ENDED call state signal to clean up on hangup.
Created attachment 209821 [details] [review] Do not restart calls in RINGING state. Also, the outgoing call windows in RINGING state patch is no longer strictly needed, so I've withdrawn it but it may still make sense on its own in a separate bug.
Review of attachment 209820 [details] [review]: ++
Review of attachment 209821 [details] [review]: ::: src/empathy-call-window.c @@ +2432,3 @@ + + if (self->priv->call_state == CONNECTING || + self->priv->call_state == CONNECTED || Actually don't we just want to do this only if state is DISCONNECTED?
Comment on attachment 209820 [details] [review] Added a commend about relying on the ENDED call state signal to I already merged the first patch.
Created attachment 209867 [details] [review] Don't restart a call if one already in progress Re-spin checking only for the DISCONNECTED state
Attachment 209867 [details] pushed as 1e03889 - Don't restart a call if one already in progress