GNOME Bugzilla – Bug 779202
ref counting issue fix for Context in gst-libs/gst/gl/gstglcontext.c
Last modified: 2017-05-29 07:59:16 UTC
Created attachment 346683 [details] [review] patch file > There is a ref counting issue in gst-libs/gst/gl/gstglcontext.c. > _GstGLContextPrivate holds onto GThread's without proper ref > counting. when a thread was released I would get exceptions. > see my previous email "heap corruption in gstreamer when IP camera > with an rtp stream disconnects (losses network)" > > > > --- a/gst-libs/gst/gl/gstglcontext.c > +++ b/gst-libs/gst/gl/gstglcontext.c > @@ -371,7 +371,7 @@ gst_gl_context_new (GstGLDisplay * display) > * @context_type: a #GstGLPlatform specifying the type of context in > @handle > * @available_apis: a #GstGLAPI containing the available OpenGL apis > in @handle > * > - * Wraps an existing OpenGL context into a #GstGLContext. > + * Wraps an existing OpenGL context into a #GstGLContext. > * > * Note: The caller is responsible for ensuring that the OpenGL context > * represented by @handle stays alive while the returned > #GstGLContext is > @@ -668,19 +668,37 @@ gst_gl_context_finalize (GObject * object) > g_cond_wait (&context->priv->destroy_cond, > &context->priv->render_lock); > GST_INFO_OBJECT (context, "gl thread joined"); > > - g_thread_unref (context->priv->gl_thread); > - context->priv->gl_thread = NULL; > + if (context->priv->gl_thread) { > + g_thread_unref (context->priv->gl_thread); > + context->priv->gl_thread = NULL; > + } > } > g_mutex_unlock (&context->priv->render_lock); > > gst_gl_window_set_close_callback (context->window, NULL, NULL, > NULL); > gst_object_unref (context->window); > + context->window = NULL; > } > > - if (context->priv->sharegroup) > + if (context->priv->active_thread) { > + g_thread_unref (context->priv->active_thread); > + context->priv->active_thread = NULL; > + } > + > + if (context->priv->gl_thread) { > + g_thread_unref (context->priv->gl_thread); > + context->priv->gl_thread = NULL; > + } > + > + if (context->priv->sharegroup) { > _context_share_group_unref (context->priv->sharegroup); > + context->priv->sharegroup = NULL; > + } > > - gst_object_unref (context->display); > + if (context->display) { > + gst_object_unref (context->display); > + context->display = NULL; > + } > > if (context->gl_vtable) { > g_slice_free (GstGLFuncs, context->gl_vtable); > @@ -729,10 +747,19 @@ gst_gl_context_activate (GstGLContext * context, > gboolean activate) > result = context_class->activate (context, activate); > > if (result && activate) { > - context->priv->active_thread = g_thread_self (); > + GThread *saveOldThread = context->priv->active_thread; //save the > old thread > + context->priv->active_thread = g_thread_self (); // ref count is > 1, does not increment count > + g_thread_ref (context->priv->active_thread); //add ref count > + if (saveOldThread) { > + g_thread_unref (saveOldThread); // release old thread > + } > + > g_private_set (¤t_context_key, context); > } else { > - context->priv->active_thread = NULL; > + if (context->priv->active_thread) { > + g_thread_unref (context->priv->active_thread); > + context->priv->active_thread = NULL; > + } > g_private_set (¤t_context_key, NULL); > } > GST_OBJECT_UNLOCK (context); > @@ -1002,7 +1029,7 @@ gst_gl_context_create (GstGLContext * context, > _context_share_group_ref (other_context->priv->sharegroup); > > context->priv->gl_thread = g_thread_new ("gstglcontext", > - (GThreadFunc) gst_gl_context_create_thread, context); > + (GThreadFunc) gst_gl_context_create_thread, context); // ref > count 2 > > while (!context->priv->created) > g_cond_wait (&context->priv->create_cond, > &context->priv->render_lock); > @@ -1728,10 +1755,19 @@ gst_gl_wrapped_context_get_gl_platform > (GstGLContext * context) > static gboolean > gst_gl_wrapped_context_activate (GstGLContext * context, gboolean > activate) > { > - if (activate) > - context->priv->gl_thread = g_thread_self (); > - else > - context->priv->gl_thread = NULL; > + if (activate) { > + GThread *saveOldThread = context->priv->gl_thread; // save old > thread > + context->priv->gl_thread = g_thread_self (); // ref count is one > + g_thread_ref (context->priv->gl_thread); // add ref count > + if (saveOldThread) { > + g_thread_unref (saveOldThread); // release old thread > + } > + } else { > + if (context->priv->gl_thread) { > + g_thread_unref (context->priv->gl_thread); > + context->priv->gl_thread = NULL; > + } > + } > > return TRUE; > } >
While the patch seems to make sense (except for code-style, please use snake_case variable_names instead of CamelCase and no C99 //-comments), can you also describe the exact steps that happen for this to crash?
Review of attachment 346683 [details] [review]: Explain the commit message, I mean :) ::: gst-libs/gst/gl/gstglcontext.c @@ +694,2 @@ _context_share_group_unref (context->priv->sharegroup); + context->priv->sharegroup = null; Upper-case NULL @@ +698,3 @@ + if (context->display) { + gst_object_unref (context->display); + context->display = null; And here @@ +749,2 @@ if (result && activate) { + GThread *saveOldThread = context->priv->active_thread; save_old_thread, also below
commit eca91d6772f74e4217d845628bfacbdcddd59603 Author: fvanzile <frank@fvanzile.com> Date: Thu Feb 23 15:42:08 2017 -0800 glcontext: keep a ref to the active thread With the macOS/iOS implementations, the active thread can change multiple times over the life of a pipeline which would expose a race in the thread tracking. Fix by taking a ref on the active thread while the context is active. https://bugzilla.gnome.org/show_bug.cgi?id=779202