Bug 37079

Summary:	tp_base_connection_change_status can have a side-effect before hitting a return_if_fail
Product:	Telepathy	Reporter:	Simon McVittie <smcv>
Component:	tp-glib	Assignee:	Simon McVittie <smcv>
Status:	RESOLVED FIXED	QA Contact:	Telepathy bugs list <telepathy-bugs>
Severity:	normal
Priority:	medium	CC:	marco.barisione
Version:	0.13	Keywords:	patch
Hardware:	Other
OS:	All
URL:	http://cgit.collabora.co.uk/git/user/smcv/telepathy-glib-smcv.git/commit/?h=014-am-i-critical-or-not
Whiteboard:	NB#254027, review+
i915 platform:		i915 features:
Attachments:	tp_base_connection_change_status: delay side-effects until all preconditions are checked

Description Simon McVittie 2011-05-10 10:53:25 UTC

+++ This bug was initially created as a clone of Bug #37078 +++

We've seen some Gabble crash-dumps with these criticals logged (in the configuration where they were reported, they're non-fatal):

May  5 13:04:07 (none) telepathy-gabble[23476]: GLIB CRITICAL ** tp-glib -
tp_base_connection_add_interfaces: assertion `self->status !=
TP_CONNECTION_STATUS_DISCONNECTED' failed
May  5 13:04:07 (none) telepathy-gabble[23476]: GLIB CRITICAL ** tp-glib -
tp_base_connection_change_status: assertion `prev_status ==
TP_CONNECTION_STATUS_CONNECTING' failed

and then an abort caused by this assertion failure:

"(base->status == TP_CONNECTION_STATUS_DISCONNECTED) ||
(base->status == TP_INTERNAL_CONNECTION_STATUS_NEW)"

The reason that assertion should be OK is that GabbleConnectionManager holds a
ref to every GabbleConnection, from before the time it moves to CONNECTED until
after the time it moves to DISCONNECTED. The life-cycle is NEW -> CONNECTING ->
CONNECTED -> DISCONNECTED, except that you can also become DISCONNECTED from
any earlier state.

It looks as though the two criticals I quoted can only happen if:

* set_status_to_connected is called when we already DISCONNECTED
* which requires decrement_waiting_connected when we already DISCONNECTED

s_s_t_c should bail out early if we already DISCONNECTED; that's a Gabble bug (Bug #37078).

Meanwhile, telepathy-glib's tp_base_connection_change_status() can critical()
but already have had its side-effect of changing the status to CONNECTED:
that's a telepathy-glib bug (this one).

Comment 1 Simon McVittie 2011-05-10 11:13:43 UTC

Created attachment 46558 [details] [review]
tp_base_connection_change_status: delay side-effects until  all preconditions are checked

If the caller makes an invalid state change, we shouldn't allow the
status to be updated, for instance from DISCONNECTED to CONNECTED if
success and failure race with each other and failure wins, as seen in
fd.o #37078; we should just emit the critical warning and leave it
DISCONNECTED.

Comment 2 Vivek Dasmohapatra 2011-05-13 07:51:23 UTC

ship it

Comment 3 Simon McVittie 2011-05-16 02:19:51 UTC

Shipped it: 0.14.6, 0.15.0.

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.