[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 bugzilla-daemon at webkit.org
Mon Jan 10 08:42:21 PST 2022

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

Attachment 448697: Patch


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

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.

More information about the webkit-reviews mailing list