Bug 154285 – [GStreamer] clean-up various leaks

• Status: RESOLVED
• Resolution: FIXED
• Component: Platform

[Philippe Normand]

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.

[Philippe Normand]

Created attachment 271425 [details]
patch

[Carlos Garcia Campos]

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) ?

[Philippe Normand]

(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.

[Michael Catanzaro]

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?

[Carlos Garcia Campos]

(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.

[Carlos Garcia Campos]

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.

[Philippe Normand]

Created attachment 271743 [details]
patch

[Philippe Normand]

Committed r196809: <http://trac.webkit.org/changeset/196809>

[Michael Catanzaro]

(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!