The patch was missing a second case where channels are removed from
the session, see attached patch.
Fundamentally, it doesn't change much from my patch, I guess removing
g_clear_object(channel->priv->session) and ignoring the case where the
channel isn't in the session (like you do here) would have been the
same.
I still think it's a better idea to clear the channel->session if the
channel has been disconnect from the session, although I agree that it
requires a bit more testing and fixing for some warnings/criticals.
On Fri, Nov 21, 2014 at 11:06 AM, Marc-André Lureau
<marcandre.lureau@xxxxxxxxx> wrote:
> It doesn't work well, I get a crash with pretty poor backtrace when
> migrating with virt-manager (looks like an "extra" unref from python
> GC).
>
> On Thu, Nov 20, 2014 at 3:52 PM, Christophe Fergeau <cfergeau@xxxxxxxxxx> wrote:
>> spice_session_disconnect gets rid of the reference it owns on
>> the SpiceChannels listed in SpiceSession::channels. However, it does
>> not remove them from that list, which would cause multiple calls to
>> spice_session_disconnect to remove references it doesn't own, which
>> will cause memory management issues later on.
>>
>> This patch removes the channels from SpiceSession::channels after they
>> are unref'ed to ensure the reference SpiceSession owns will not be
>> removed multiple times.
>>
>> It would be cleaner to clear the SpiceChannel::session when it's removed
>> from the session, but client applications (virt-viewer)/other spice-gtk
>> code do not deal well with SpiceChannel::session getting set to NULL
>> when a disconnect event happens.
>> ---
>> Hey,
>>
>> I've experimented with that as well, and it seems that this patch would solve
>> that issue with using spice-gtk from python/... (haven't tested virt-manager at
>> all), and would cause less issues with existing code, what do you think?
>>
>> Christophe
>>
>>
>> gtk/spice-session.c | 15 ++++++++++-----
>> 1 file changed, 10 insertions(+), 5 deletions(-)
>>
>> diff --git a/gtk/spice-session.c b/gtk/spice-session.c
>> index c44a3e1..df71eb7 100644
>> --- a/gtk/spice-session.c
>> +++ b/gtk/spice-session.c
>> @@ -1650,8 +1650,12 @@ void spice_session_disconnect(SpiceSession *session)
>> for (ring = ring_get_head(&s->channels); ring != NULL; ring = next) {
>> next = ring_next(&s->channels, ring);
>> item = SPICE_CONTAINEROF(ring, struct channel, link);
>> - spice_channel_destroy(item->channel); /* /!\ item and channel are destroy() after this call */
>> + ring_remove(&item->link);
>> + spice_channel_disconnect(item->channel, SPICE_CHANNEL_NONE);
>> + g_object_unref(item->channel);
>> + free(item);
>> }
>> + g_warn_if_fail(ring_is_empty(&s->channels));
>>
>> s->connection_id = 0;
>>
>> @@ -1940,7 +1944,7 @@ void spice_session_channel_destroy(SpiceSession *session, SpiceChannel *channel)
>> {
>> SpiceSessionPrivate *s = session->priv;
>> struct channel *item = NULL;
>> - RingItem *ring;
>> + RingItem *ring = NULL;
>>
>> g_return_if_fail(s != NULL);
>> g_return_if_fail(channel != NULL);
>> @@ -1955,15 +1959,16 @@ void spice_session_channel_destroy(SpiceSession *session, SpiceChannel *channel)
>> break;
>> }
>>
>> - g_return_if_fail(ring != NULL);
>>
>> if (channel == s->cmain) {
>> CHANNEL_DEBUG(channel, "the session lost the main channel");
>> s->cmain = NULL;
>> }
>>
>> - ring_remove(&item->link);
>> - free(item);
>> + if (ring != NULL) {
>> + ring_remove(&item->link);
>> + free(item);
>> + }
>>
>> g_signal_emit(session, signals[SPICE_SESSION_CHANNEL_DESTROY], 0, channel);
>> }
>> --
>> 2.1.0
>>
>> _______________________________________________
>> Spice-devel mailing list
>> Spice-devel@xxxxxxxxxxxxxxxxxxxxx
>> http://lists.freedesktop.org/mailman/listinfo/spice-devel
>
>
>
> --
> Marc-André Lureau
--
Marc-André Lureau
From 24bc3c895f590cd369a05dad87a56e074ce07fff Mon Sep 17 00:00:00 2001
From: Christophe Fergeau <cfergeau@xxxxxxxxxx>
Date: Thu, 20 Nov 2014 15:52:19 +0100
Subject: [PATCH spice-gtk] Remove channels from SpiceSession::channels on
session disconnect
spice_session_disconnect gets rid of the reference it owns on
the SpiceChannels listed in SpiceSession::channels. However, it does
not remove them from that list, which would cause multiple calls to
spice_session_disconnect to remove references it doesn't own, which
will cause memory management issues later on.
This patch removes the channels from SpiceSession::channels after they
are unref'ed to ensure the reference SpiceSession owns will not be
removed multiple times.
It would be cleaner to clear the SpiceChannel::session when it's removed
from the session, but client applications (virt-viewer)/other spice-gtk
code do not deal well with SpiceChannel::session getting set to NULL
when a disconnect event happens.
---
gtk/spice-session.c | 27 +++++++++++++++++++--------
1 file changed, 19 insertions(+), 8 deletions(-)
diff --git a/gtk/spice-session.c b/gtk/spice-session.c
index a628b7f..3a5098f 100644
--- a/gtk/spice-session.c
+++ b/gtk/spice-session.c
@@ -1360,6 +1360,15 @@ static void cache_clear_all(SpiceSession *self)
glz_decoder_window_clear(s->glz_window);
}
+static void
+session_ring_remove(struct channel *item)
+{
+ ring_remove(&item->link);
+ spice_channel_disconnect(item->channel, SPICE_CHANNEL_NONE);
+ g_object_unref(item->channel);
+ free(item);
+}
+
G_GNUC_INTERNAL
void spice_session_switching_disconnect(SpiceSession *self)
{
@@ -1375,8 +1384,9 @@ void spice_session_switching_disconnect(SpiceSession *self)
for (ring = ring_get_head(&s->channels); ring != NULL; ring = next) {
next = ring_next(&s->channels, ring);
item = SPICE_CONTAINEROF(ring, struct channel, link);
- if (item->channel != s->cmain)
- spice_channel_destroy(item->channel); /* /!\ item and channel are destroy() after this call */
+ if (item->channel == s->cmain)
+ continue;
+ session_ring_remove(item);
}
g_warn_if_fail(!ring_is_empty(&s->channels)); /* ring_get_length() == 1 */
@@ -1649,8 +1659,9 @@ void spice_session_disconnect(SpiceSession *session)
for (ring = ring_get_head(&s->channels); ring != NULL; ring = next) {
next = ring_next(&s->channels, ring);
item = SPICE_CONTAINEROF(ring, struct channel, link);
- spice_channel_destroy(item->channel); /* /!\ item and channel are destroy() after this call */
+ session_ring_remove(item);
}
+ g_warn_if_fail(ring_is_empty(&s->channels));
s->connection_id = 0;
@@ -1938,7 +1949,7 @@ void spice_session_channel_destroy(SpiceSession *session, SpiceChannel *channel)
{
SpiceSessionPrivate *s = session->priv;
struct channel *item = NULL;
- RingItem *ring;
+ RingItem *ring = NULL;
g_return_if_fail(s != NULL);
g_return_if_fail(channel != NULL);
@@ -1953,15 +1964,15 @@ void spice_session_channel_destroy(SpiceSession *session, SpiceChannel *channel)
break;
}
- g_return_if_fail(ring != NULL);
-
if (channel == s->cmain) {
CHANNEL_DEBUG(channel, "the session lost the main channel");
s->cmain = NULL;
}
- ring_remove(&item->link);
- free(item);
+ if (ring != NULL) {
+ ring_remove(&item->link);
+ free(item);
+ }
g_signal_emit(session, signals[SPICE_SESSION_CHANNEL_DESTROY], 0, channel);
}
--
2.1.0
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/spice-devel