Comment on attachment 300331
Patch

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

> Source/WebCore/ChangeLog:13
> +        Files cached on disk by MediaPlayerPrivateGStreamer are deleted only when the player is closed. If the
> +        WebProcess crashed, they're just left there in the cache directory. This patch changes the location
> +        of those temporary files to a proper temporary directory (/var/tmp, as those files aren't actually
> +        reusable, so they don't belong to a cache directory, and /tmp is a bad place because it's RAM-based on
> +        some distros), unlinks (deletes) them right after creation and also deletes any other stalled temporary
> +        file on the old legacy cache directory.

Wouldn't it be a problem if /var is in a different partition? I'm not sure if the solution to leaked files is placing them in /tmp to be honest. If we can decide the folder, then we can use a specific one in the user cache dir, and clean it up at startup, for example.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1348
> +    if (!g_strcmp0(className, "GstDownloadBuffer")) {

Early return here instead.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1350
> +        g_signal_handlers_disconnect_by_func(bin, gpointer(uriDecodeBinElementAddedCallback), player);

Use C++ style casts

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1358
> +            processName = "GStreamer";

shouldn't it be WebKitWebProcess by default?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1360
> +        GUniquePtr<gchar> newDownloadTemplate(g_strdup_printf("/var/tmp/%s-XXXXXX", processName));

Use g_build_filename

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1364
> +        player->purgeOldDownloadFiles(oldDownloadTemplate.get());

So, if we purge the old files, then we don't really need to use /var/tmp, no?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1372
> +    g_signal_handlers_disconnect_by_func(player->m_downloadBuffer.get(), gpointer(downloadBufferFileCreatedCallback), player);

Use C++ style cast

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1379
> +    GUniqueOutPtr<gchar> downloadFile;
> +    g_object_get(player->m_downloadBuffer.get(), "temp-location", &downloadFile.outPtr(), nullptr);
> +    if (g_unlink(downloadFile.get()) == -1)
> +        GST_WARNING("Couldn't unlink media temporary file %s after creation", downloadFile.get());
> +    else
> +        GST_TRACE("Unlinked media temporary file %s after creation", downloadFile.get());

Do we need a signal for this? Doesn't gst changes it when g_object_set(element, "temp-template", synchronously?

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1411
> +    GDir* directory = g_dir_open(templatePath.get(), 0, nullptr);
> +
> +    if (!directory)
> +        return;
> +
> +    for (const gchar* fileName = g_dir_read_name(directory); fileName; fileName = g_dir_read_name(directory)) {
> +        if (g_str_has_prefix(fileName, templateFile.get())) {
> +            GUniquePtr<gchar> filePath(g_build_filename(templatePath.get(), fileName, nullptr));
> +            if (g_unlink(filePath.get()) == -1)
> +                GST_WARNING("Couldn't unlink legacy media temporary file: %s", filePath.get());
> +            else
> +                GST_TRACE("Unlinked legacy media temporary file: %s", filePath.get());
> +        }
> +    }

You can simplify this by using FileSystem::listDirectory(), you can pass a regexp to mathc the files directly

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1418
>      m_source.clear();

You should disconnect previous signals here and in the destructor I guess, since you are disconnecting only if the signals was emitted.

> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.h:249
> +    GRefPtr<GstElement> m_downloadBuffer;

Do we really want to keepo this buffer object alive for the whole life of the media platyer? Or can we release it at some point?