Bug 154285

Summary: [GStreamer] clean-up various leaks
Product: WebKit Reporter: Philippe Normand <pnormand>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cgarcia, mcatanzaro, pnormand
Priority: P2    
Version: Other   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
cgarcia: review+
patch cgarcia: review+

Description Philippe Normand 2016-02-16 03:57:30 PST
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.
Comment 1 Philippe Normand 2016-02-16 04:48:49 PST
Created attachment 271425 [details]
patch
Comment 2 Carlos Garcia Campos 2016-02-16 04:58:50 PST
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) ?
Comment 3 Philippe Normand 2016-02-16 06:04:02 PST
(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 4 Michael Catanzaro 2016-02-18 12:27:19 PST
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?
Comment 5 Carlos Garcia Campos 2016-02-18 23:19:49 PST
(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 6 Carlos Garcia Campos 2016-02-19 02:13:50 PST
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.
Comment 7 Philippe Normand 2016-02-19 02:24:14 PST
Created attachment 271743 [details]
patch
Comment 8 Philippe Normand 2016-02-19 02:30:26 PST
Committed r196809: <http://trac.webkit.org/changeset/196809>
Comment 9 Michael Catanzaro 2016-02-22 07:49:28 PST
(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!