After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 779202 - ref counting issue fix for Context in gst-libs/gst/gl/gstglcontext.c
ref counting issue fix for Context in gst-libs/gst/gl/gstglcontext.c
Status: RESOLVED FIXED
Product: GStreamer
Classification: Platform
Component: gst-plugins-bad
git master
Other Mac OS
: Normal critical
: 1.12.1
Assigned To: GStreamer Maintainers
GStreamer Maintainers
Depends on:
Blocks:
 
 
Reported: 2017-02-24 23:58 UTC by Frank VanZile
Modified: 2017-05-29 07:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
patch file (3.66 KB, patch)
2017-02-24 23:58 UTC, Frank VanZile
needs-work Details | Review

Description Frank VanZile 2017-02-24 23:58:57 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 (&current_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 (&current_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;
>  }
>
Comment 1 Sebastian Dröge (slomo) 2017-02-25 10:43:19 UTC
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?
Comment 2 Sebastian Dröge (slomo) 2017-02-25 10:44:47 UTC
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
Comment 3 Matthew Waters (ystreet00) 2017-05-20 14:21:24 UTC
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