[webkit-reviews] review denied: [Bug 236540] [GStreamer] Initial import of the GstWebRTC backend : [Attachment 454531] 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 236540: [GStreamer] Initial import of the GstWebRTC backend
https://bugs.webkit.org/show_bug.cgi?id=236540

Attachment 454531: Patch

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




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

View in context: https://bugs.webkit.org/attachment.cgi?id=454531&action=review

There are a couple of memory issues in this patch that need to be addressed.
After this, I'll give a lgtm and I think Carlos should give the r+ as he did
most of this review iterations.

>
Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.cpp:14
0
> +	       if (stateChange.error) {

Question, is letting this out of here to change the ready state on error legit
or should we return?

> Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.h:54
> +    bool sendStringData(const CString&) final;
> +    bool sendRawData(const uint8_t*, size_t) final;

Not strong opinion, you could rename this to sendData and trust the language to
do the overload for you.

> Source/WebCore/Modules/mediastream/gstreamer/GStreamerDataChannelHandler.h:58
> +    void onMessageData(GBytes*);
> +    void onMessageString(const char*);

Ditto, onMessage.

>
Source/WebCore/Modules/mediastream/gstreamer/GStreamerIceTransportBackend.cpp:5
4
> +    g_object_get(m_backend.get(), "transport", &m_iceTransport.outPtr(),
nullptr);

I guess there is a possibility that m_iceTransport has a value. Calling
outPtr() will get it overwritten without releasing the former value, hence
leaking. Please issue a .clear() before this.

> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:226
> +		   bool result = false;

gboolean?

> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:412
> +		   errorHolder.emplace(g_error_copy(error.get()));

I would try something like:

errorHolder = GUniquePtr<GError>(error.release());

That way you transform GUniqueOutPtr into GUniquePtr, then the r-value logic
should put that into the optional without copying the error.

In fact, it would be nice to have something like releaseIntoGUniquePtr in
GUniqueOutPtr to avoid this convolution.

> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:545
> +    RELEASE_ASSERT(!gst_pad_is_linked(sinkPad.get()));

Wouldn't it be better in this case to let it fail with just an ASSERT and leave
the assertion for debug mode?

> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:665
> +	       GUniquePtr<GError> error(promiseError.release());
> +	       data->endPoint->createSessionDescriptionFailed(WTFMove(error));

See? Nicely done here.

> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:756
> +	   RELEASE_ASSERT_NOT_REACHED();

Just ASSERT?

> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:837
> +	   GValue value = G_VALUE_INIT;
> +	   g_value_init(&value, G_TYPE_STRING);
> +	   g_value_set_string(&value, stream->id().utf8().data());
> +	   gst_value_list_append_value(&streamIdsValue, &value);
> +	   g_value_unset(&value);

It might be interesting to create the GValue in the heap and use
gst_value_list_append_and_take_value instead. That way you don't need to unset
later and you can save some copy.

> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:839
> +    gst_structure_take_value(initData.get(), "stream-ids", &streamIdsValue);

This seems to be wrong, since you are telling the structure to take that
GValue*, which is stack memory.

I hope I didn't miss this pattern anywhere else, please ensure it.

> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:846
> +	       GValue value = G_VALUE_INIT;

Ditto

> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:855
> +	       GValue value = G_VALUE_INIT;

Ditto

> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:862
> +    gst_structure_take_value(initData.get(), "encodings", &encodingsValue);

Ditto.

> Source/WebCore/Modules/mediastream/gstreamer/GStreamerMediaEndpoint.cpp:1144
> +	   std::unique_ptr<GStreamerMediaEndpointHolder>
data(static_cast<GStreamerMediaEndpointHolder*>(userData));

You need to do this at the beginning of the function or you'll be leaking the
userData.

Another thing is that this will only work once because the user data will be
destroyed the second time it gets called or you'll be leaking if the signal is
never emitted. I think the best is passing the GDestroyNotify.

I hope I did not miss this pattern anywhere else, please ensure it.

> Source/WebCore/Modules/mediastream/gstreamer/GStreamerWebRTCUtils.cpp:406
> +    Vector<uint8_t> buffer;
> +    buffer.reserveInitialCapacity(8);
> +    WTF::cryptographicallyRandomValues(buffer.data(), 8);

I don't think this is ok. After you do this, the data is populated and it uses
legit memory but the size is not updated, only the capacity, which is what you
do when you reserve it initially.

explicit Vector(size_t size) might be what you need.

This said, since you don't use the size, just as a holder for data that does
not need anything special (like calling destructors later), it won't cause
trouble.

>
Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingMediaSourceGStrea
mer.cpp:40
> +

Extra line

>
Source/WebCore/platform/mediastream/gstreamer/RealtimeOutgoingMediaSourceGStrea
mer.cpp:148
> +    g_object_get(transceiver.get(), "sender", &m_sender.outPtr(), nullptr);

I guess there is a possibility that m_sender has a value. Calling outPtr() will
get it overwritten without releasing the former value, hence leaking. Please
issue a .clear() before this.