[Spice-devel] [PATCH spice-gtk 1/2] session: remove channels on disconnect

[Marc-André Lureau]

Hi

----- Original Message -----
> On Thu, Nov 13, 2014 at 08:40:22AM -0500, Marc-André Lureau wrote:
> > Hi
> > 
> > I already have patches fixing this. And yes, it will crash with newer
> > spice-gtk if virt-viewer doesn't have the fix.
> > 
> > (if we don't have this kind of fix, we will keep crashing in virt-manager,
> > and I yet have to check Boxes behaviour)
> > 
> > In any case, the current channel lifecycle model is broken by design,
> > so we should fix it even if it brings temporarily this kind of runtime
> > crashes.
> 
> If all applications needs changes in order not to crash, I'd call that
> an ABI break. Maybe it's possible to get to some middle ground where
> applications are not crashing, but it's obvious that they need fixing?

Let see. The fix for virt-viewer is (and I believe each client has a particular way of deal with this):

commit d2f4f28ea3f5fdf217d39898011af7f38fe252b5
Author: Marc-André Lureau <marcandre.lureau at gmail.com>
Date:   Wed Nov 12 18:33:02 2014 +0100

    spice: calling VirtViewerSession:close() can destroy self
    
    SpiceSession in spice-gtk v0.27 removes channels from session during
    disconnect (and not when they are actually disposed). When no channels
    are left, session-disconnected is emitted, and the VirtViewerSession
    will be unref from the application. Use a weak reference to self to
    avoid crashing after calling spice_session_disconnect()
    
    (the alternative of calling ref/unref would keep recreating session,
     which is something it can avoid when leaving the application)

diff --git a/src/virt-viewer-session-spice.c b/src/virt-viewer-session-spice.c
index 2eb2224..afeeadd 100644
--- a/src/virt-viewer-session-spice.c
+++ b/src/virt-viewer-session-spice.c
@@ -292,10 +292,15 @@ virt_viewer_session_spice_close(VirtViewerSession *session)
 
     g_return_if_fail(self != NULL);
 
+    g_object_add_weak_pointer(G_OBJECT(self), (gpointer*)&self);
+
     virt_viewer_session_clear_displays(session);
 
     if (self->priv->session) {
         spice_session_disconnect(self->priv->session);
+        if (!self)
+            return;
+
         g_object_unref(self->priv->session);
         self->priv->session = NULL;
         self->priv->gtk_session = NULL;

Furthermore, since the spice session is destroyed when no channels are left we need this in spice-gtk:

commit 1f541743293ba2cf0b735863073d08c4177c3b5f
Author: Marc-André Lureau <marcandre.lureau at redhat.com>
Date:   Wed Nov 12 15:55:23 2014 +0100

    Keep a reference on session disconnect
    
    It is idiomatic for client code to clean up it's reference on channel
    disconnection. Keeping a reference during disconnect help solving
    potential crashes if the session is unref during callbacks.

diff --git a/gtk/spice-session.c b/gtk/spice-session.c
index 5729d8a..b4c2b73 100644
--- a/gtk/spice-session.c
+++ b/gtk/spice-session.c
@@ -1648,6 +1648,7 @@ void spice_session_disconnect(SpiceSession *session)
     if (s->disconnecting)
         return;
 
+    g_object_ref(session);
     s->disconnecting = TRUE;
     s->cmain = NULL;
 
@@ -1666,6 +1667,7 @@ void spice_session_disconnect(SpiceSession *session)
     spice_session_abort_migration(session);
     /* we leave disconnecting = TRUE, so that spice_channel_disconnect()
        is not called multiple times */
+    g_object_unref(session);
 }
 


I think both fixes are legitimate with today API, since there is no guarantee that objects will be alive after callbacks are called. So it's not breaking for me, it's rather ensuring things will work in all cases.

I imagine delaying session_disconnect() in idle (while holding a ref) could work, but will bring additional issues due to its async nature.

Any other idea?