Comment 24 for bug 1203240

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?