[webkit-reviews] review denied: [Bug 214150] [GStreamer][1.18] mediastreamsrc element hits assert in gst -core : [Attachment 403892] Patch

[bugzilla-daemon at webkit.org]

Xabier Rodríguez Calvar <calvaris at igalia.com> has denied Philippe Normand
<pnormand at igalia.com>'s request for review:
Bug 214150: [GStreamer][1.18] mediastreamsrc element hits assert in gst -core
https://bugs.webkit.org/show_bug.cgi?id=214150

Attachment 403892: Patch

https://bugs.webkit.org/attachment.cgi?id=403892&action=review




--- Comment #5 from Xabier Rodríguez Calvar <calvaris at igalia.com> ---
Comment on attachment 403892
  --> https://bugs.webkit.org/attachment.cgi?id=403892
Patch

For what I studied the issue the whole concept of the observers keeping a
referenced to the element is a bad idea because the finalize method will never
be called. The moment we create the media stream src the are reffing ourselves
in the Constructed method twice and keeping an returning an object that was
refcount 3 (without taking into account other leaks involving GRefPtr). This
means that when you return create an object and unref it just afterwards, it
won't be destroyed (unless I am missing anything). I did a lousy review at the
original bug and didn't realize about this. This needs to be addressed here.
I'm sorry for that.

Then we need to consider that with the unpatched code, we're implicitly calling
the GRefPtr constructor.

Besides, current code with the smart pointers is adding a another extra
reference as the ref is bumped again the second time the GRefPtr constructor is
run but not the first. The first is ref_sinking the reference, not increasing
it, but the second does, so with the current unpatched code we are returning an
object with ref count 4.

As I said the first implicit GRefPtr constructor call is ref_sinking the
reference so in the end of the Constructed method, the element has a sinked
referenced that is causing the warning. The problem is that this warning is
just the tip of the iceberg.

For me, the best solution of this problem would be using weak refs to keep the
element in the observer, but there is no smart pointer for that and it would
just be an aesthetic improvement over the solution I'll just propose.

Considering that the life of the observers is 1-1 with the life of the source,
we can just keep a plain GstElement* inside the observer and forget the smart
ptr and gobject reffing at all. In the Constructed method, we pass self down
and we keep a GstElement*.

Another thing is that from what I see of how the observers are being passed to
the callers, they are always passed as reference (l-value), not as a pointer,
so I think we we could even keep the objects in the struct. The problem of
keeping the objects in the struct is that you have the chicken/egg problem when
constructing so either you add a method to the observer to set the GstElement*
calling it in the Constructed method or you leave it as it is.

Independently of leaving the observers as unique_ptrs or not, there is another
problem with this object and it is that I don't think the destructors of be
objects inside the struct are being called, since this is allocated with C,
right? I think this needs to be checked as well and ensure the destructor is
called as it happens for example in the case of the decryptors.

I think we opened quite a can of worms not related to your patch here!