[webkit-reviews] review denied: [Bug 234084] [GStreamer] tests media/track/audio-track-configuration.html and media/track/video-track-configuration.html fail : [Attachment 448697] 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 234084: [GStreamer] tests media/track/audio-track-configuration.html and
media/track/video-track-configuration.html fail
https://bugs.webkit.org/show_bug.cgi?id=234084

Attachment 448697: Patch

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




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

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

> Source/WebCore/platform/graphics/gstreamer/AudioTrackPrivateGStreamer.cpp:59
> +    auto scopeExit = makeScopeExit([&] {
> +	   setConfiguration(WTFMove(configuration));
> +    });

Why do you need this if I don't see any return?

> Source/WebCore/platform/graphics/gstreamer/VideoTrackPrivateGStreamer.cpp:59
> +    PlatformVideoTrackConfiguration configuration;
> +    auto scopeExit = makeScopeExit([&] {
> +	   setConfiguration(WTFMove(configuration));
> +    });

This is bad.

First, regarding the life cycle of these objects, the compiler is going to
destroy first the scopeExit and then the configuration object. But you're
depending on something obscure.

Second and more dangerous. configuration belongs to the caller, when you create
the lambda, you just copy a reference into the lambda but then inside the
lambda, you move the object inside setConfiguration. configuration is going to
be destructed when the caller dies first when the caller dies and then another
time after setConfiguration finishes dealing with it. Very bad.

I think you can either avoid the scope exit or create the configuration object,
get a pointer to it, move the object inside the lambda and operate with the
pointer outside the lambda.