Some of the ASSERTS in GRefPtrGStreamer don't check null pointers, they should. Besides, we should use adoptGRef for the GstElementFactory GRefPtrs in MediaPlayerPrivateGStreamer.cpp.
Created attachment 271425 [details] patch
Comment on attachment 271425 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271425&action=review > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:196 > + priv->task = adoptGRef(gst_task_new(reinterpret_cast<GstTaskFunction>(webKitWebAudioSrcLoop), src, 0)); GstTask is GInitiallyUnowned, so this returns a floating reference that we should consume here, so adoptGRef is not correct. Aren't you hitting the assert in GRefPtr<GstTask> adoptGRef(GstTask* ptr) ?
(In reply to comment #2) > Comment on attachment 271425 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271425&action=review > > > Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:196 > > + priv->task = adoptGRef(gst_task_new(reinterpret_cast<GstTaskFunction>(webKitWebAudioSrcLoop), src, 0)); > > GstTask is GInitiallyUnowned, so this returns a floating reference that we > should consume here, so adoptGRef is not correct. Aren't you hitting the > assert in GRefPtr<GstTask> adoptGRef(GstTask* ptr) ? Heh. No :) But I'll double-check.
Comment on attachment 271425 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271425&action=review >>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:196 >>> + priv->task = adoptGRef(gst_task_new(reinterpret_cast<GstTaskFunction>(webKitWebAudioSrcLoop), src, 0)); >> >> GstTask is GInitiallyUnowned, so this returns a floating reference that we should consume here, so adoptGRef is not correct. Aren't you hitting the assert in GRefPtr<GstTask> adoptGRef(GstTask* ptr) ? > > Heh. No :) > But I'll double-check. This should be adoptGRef(g_object_ref_sink(gst_task_new(...))), correct? I think floating references are not useful in combination with GRefPtr, so why don't we take it always in adoptGRef() (e.g. by calling g_object_is_floating() and then g_object_ref_sink() if that return TRUE)? What is the value in asserting that the ptr is not floating? (Why do we do that only in GRefPtrGStreamer.cpp and not in GRefPtr.cpp?) Am I missing something here?
(In reply to comment #4) > Comment on attachment 271425 [details] > patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=271425&action=review > > >>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:196 > >>> + priv->task = adoptGRef(gst_task_new(reinterpret_cast<GstTaskFunction>(webKitWebAudioSrcLoop), src, 0)); > >> > >> GstTask is GInitiallyUnowned, so this returns a floating reference that we should consume here, so adoptGRef is not correct. Aren't you hitting the assert in GRefPtr<GstTask> adoptGRef(GstTask* ptr) ? > > > > Heh. No :) > > But I'll double-check. > > This should be adoptGRef(g_object_ref_sink(gst_task_new(...))), correct? No. > I think floating references are not useful in combination with GRefPtr, so > why don't we take it always in adoptGRef() (e.g. by calling > g_object_is_floating() and then g_object_ref_sink() if that return TRUE)? Because in that case you are not adopting the ref, but consuming the floating reference. > What is the value in asserting that the ptr is not floating? (Why do we do > that only in GRefPtrGStreamer.cpp and not in GRefPtr.cpp?) Am I missing > something here? Because we want to prevent misuses of GRefPtr with floating references. You should never adopt a floating reference. I think we only do this in GRefPtrGStreamer because GstObject is GInitiallyUnowned, so we know all gst classes are using floating references. Since this seems to be a bit confusing, I've tried to clarify it and written this http://trac.webkit.org/wiki/WebKitGTK/GRefPtrAndFloatingRefs. I hope it helps.
Comment on attachment 271425 [details] patch View in context: https://bugs.webkit.org/attachment.cgi?id=271425&action=review >>>>> Source/WebCore/platform/audio/gstreamer/WebKitWebAudioSourceGStreamer.cpp:196 >>>>> - priv->task = gst_task_new(reinterpret_cast<GstTaskFunction>(webKitWebAudioSrcLoop), src, 0); >>>>> + priv->task = adoptGRef(gst_task_new(reinterpret_cast<GstTaskFunction>(webKitWebAudioSrcLoop), src, 0)); >>>> >>>> GstTask is GInitiallyUnowned, so this returns a floating reference that we should consume here, so adoptGRef is not correct. Aren't you hitting the assert in GRefPtr<GstTask> adoptGRef(GstTask* ptr) ? >>> >>> Heh. No :) >>> But I'll double-check. >> >> This should be adoptGRef(g_object_ref_sink(gst_task_new(...))), correct? >> >> I think floating references are not useful in combination with GRefPtr, so why don't we take it always in adoptGRef() (e.g. by calling g_object_is_floating() and then g_object_ref_sink() if that return TRUE)? What is the value in asserting that the ptr is not floating? (Why do we do that only in GRefPtrGStreamer.cpp and not in GRefPtr.cpp?) Am I missing something here? > > No. So, the thing is that gst_task_init is consuming its own floating ref. This is soooo confusing. Please, add a comment here explaining why we are adopting the ref returned by gst_task_new(). > Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:127 > + ASSERT(!ptr || !g_object_is_floating(G_OBJECT(ptr))); no need to cast to GObject, g_object_is_floating expects a gpointer. > Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:147 > + ASSERT(!ptr || !g_object_is_floating(G_OBJECT(ptr))); ditto. > Source/WebCore/platform/graphics/gstreamer/GRefPtrGStreamer.cpp:167 > + ASSERT(!ptr || !g_object_is_floating(G_OBJECT(ptr))); ditto.
Created attachment 271743 [details] patch
Committed r196809: <http://trac.webkit.org/changeset/196809>
(In reply to comment #5) > Because we want to prevent misuses of GRefPtr with floating references. You > should never adopt a floating reference. I think we only do this in > GRefPtrGStreamer because GstObject is GInitiallyUnowned, so we know all gst > classes are using floating references. Since this seems to be a bit > confusing, I've tried to clarify it and written this > http://trac.webkit.org/wiki/WebKitGTK/GRefPtrAndFloatingRefs. I hope it > helps. Yes, thank you!