Re: [gtk-vnc-devel] crash on disconnect

[Jonh Wendell <jwendell gnome org>]

Em Ter, 2008-04-15 às 03:55 +0100, Daniel P. Berrange escreveu:
> On Mon, Apr 14, 2008 at 10:33:16PM -0300, Jonh Wendell wrote:
> > Em Ter, 2008-04-15 às 01:47 +0100, Daniel P. Berrange escreveu:
> > > On Mon, Apr 14, 2008 at 07:45:06PM -0500, Anthony Liguori wrote:
> > > > Jonh Wendell wrote:
> > > > >Yes, in my case, I do not call vnc_close() or similar, I just destroy
> > > > >the widget.
> > > > >  
> > > > 
> > > > That's not the best of things to do.  You should really explicitly close 
> > > > the connection before destroying the widget if you can.
> > 
> > I agree with Dan here, since we have vnc_display_close() in destroy().
> > Anyway, I modified vinagre to call vnc_display_close() before destroy
> > the widget and the result was the same.
> > 
> > > Actually that should be OK since we increment the ref-count when the
> > > coroutine is running so that the vncdisplay widget is not actually
> > > free'd until it exits. What's odd though is that the debug trace does
> > > not show the vncdisplay widget cleanup code being called which I would
> > > definitely expect if it were being destroyed indirectly by its container.
> > > 
> > > Dan.
> > 
> > OK, Dan, you're right. I managed to get a decent output this time:
> > 
> > **********************
> > gtk-vnc: FramebufferUpdate(16, 0, 0, 1024, 768)
> > gtk-vnc: Expose 0x0 @ 12,21
> > gtk-vnc: Expose 0x0 @ 1013,581
> > gtk-vnc: Expose 224x213 @ 340,71
> > gtk-vnc: Requesting that VNC close
> > gtk-vnc: Requesting graceful shutdown of connection
> > gtk-vnc: Waking up couroutine to shutdown gracefully
> > gtk-vnc: Closing the connection: gvnc_read() - ret=-1
> > Segmentation fault (core dumped)
> > **********************
> > 
> > The backtrace is exactly the same as before.
> 
> Ahhhhh, I see what is going on.
> 
>  - You destroy the container
>  - This calls vnc_display_destroy()
>  - Which calls vnc_display_close()
>  - Which requests that the co-routine shuts down asynchronously
> 
> So the vnc_display object still exists at this point, and will continue
> to exist until the co-routine actually exits  and releases the last
> reference.
> 
> Unfortunately....
> 
> ....just after asking the coroutine todo async shutdown, vnc_display_close()
> goes ahead and free's the XImage / OpenGL texture.
> 
> But if the co-routine is in middle of an update it may well try to memcpy()
> data into the image we just free'd before checking its shutdown flag...
> 
> ... kaboom !
> 
> 
> Basically, the code in vnc_display_close()  that free's the image / texture
> needs to be changed so that it is only run after the coroutine has actually
> exited.
> 
> Dan.

Thanks Dan!

In this patch, I just moved the free() calls in vnc_close() to
vnc_delayed_unref(). It has fixed the crash to me.

Comments?
-- 
Jonh Wendell
www.bani.com.br

diff -r 53a42c599c77 src/vncdisplay.c
--- a/src/vncdisplay.c	Fri Apr 11 09:15:48 2008 -0300
+++ b/src/vncdisplay.c	Tue Apr 15 10:09:20 2008 -0300
@@ -1434,6 +1434,18 @@ static gboolean delayed_unref_object(gpo
 	VncDisplay *obj = VNC_DISPLAY(data);
 
 	g_assert(obj->priv->coroutine.exited == TRUE);
+
+	if (obj->priv->image) {
+		g_object_unref(obj->priv->image);
+		obj->priv->image = NULL;
+	}
+
+#if WITH_GTKGLEXT
+	g_free(obj->priv->gl_tex_data);
+	obj->priv->gl_tex_data = NULL;
+	obj->priv->gl_enabled = 0;
+#endif
+
 	g_object_unref(G_OBJECT(data));
 	return FALSE;
 }
@@ -1608,20 +1620,12 @@ void vnc_display_close(VncDisplay *obj)
 		gvnc_shutdown(priv->gvnc);
 	}
 
-	if (priv->image) {
-		g_object_unref(priv->image);
-		priv->image = NULL;
-	}
-
 #if WITH_GTKGLEXT
 	if (priv->gl_tex_data) {
 		gdk_gl_drawable_gl_begin(priv->gl_drawable,
 					 priv->gl_context);
 		glDeleteTextures(1, &priv->gl_tex);
 		gdk_gl_drawable_gl_end(priv->gl_drawable);
-		g_free(priv->gl_tex_data);
-		priv->gl_tex_data = NULL;
-		priv->gl_enabled = 0;
 	}
 #endif