[Bug 750397] CRITICAL: Race condition in GstBus

[GStreamer (GNOME Bugzilla)]

https://bugzilla.gnome.org/show_bug.cgi?id=750397

Sebastian Dröge (slomo) <slomo at coaxion.net> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
 Attachment #330504|none                        |needs-work
             status|                            |

--- Comment #56 from Sebastian Dröge (slomo) <slomo at coaxion.net> ---
Review of attachment 330504:
 --> (https://bugzilla.gnome.org/review?bug=750397&attachment=330504)

Looks good to me except for minor things

::: gst/gstpoll.c
@@ +172,3 @@
 #else
 #define WAKE_EVENT(s)       (SetLastError (0), SetEvent ((s)->wakeup_event),
errno = GetLastError () == NO_ERROR ? 0 : EACCES, errno == 0 ? 1 : 0)
+#define RELEASE_EVENT(s)    (WaitForSingleObject ((s)->wakeup_event,
INFINITE), ResetEvent ((s)->wakeup_event))

You probably want to check for errors here too, and set errno like in
WAKE_EVENT

@@ +206,3 @@
+  if (set->control_pending > 0) {
+    /* release, only if this was the last pending. */
+    if (set->control_pending-- == 1) {

--set->control_pending == 0 seems more clear (in symmetry with
set->control_pending++ == 0 above)

@@ +211,3 @@
+    }
+    else
+      result = TRUE;

Curly braces here while we're at it :)

@@ +230,3 @@
+    GST_LOG ("%p: releasing %d", set, old);
+    if (!RELEASE_EVENT (set))
+      GST_WARNING ("%p: failed to release event", set);

Before this was a loop to call RELEASE_EVENT() until it succeeded

-- 
You are receiving this mail because:
You are the QA Contact for the bug.
You are the assignee for the bug.