Comment on attachment 300331
Patch

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

Thanks for the rest of the comments I'm not replying, they're really useful.

>> Source/WebCore/ChangeLog:13
>> +        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.

I don't think GstDownloadBuffer is ready to reuse the media files between sessions with its current design, so as per https://bugs.webkit.org/show_bug.cgi?id=119477#c2 and https://bugs.webkit.org/show_bug.cgi?id=119477#c3 the files shouldn't be considered "cache" but temporary files. They can be huge (eg: 2GB or similar movie sizes), so they aren't appropriate for /tmp (because of some distros using tmpfs) but they're appropriate for /var/tmp as per https://bugs.webkit.org/show_bug.cgi?id=119477#c5

I considered /var/tmp as the consensus solution for storing this kind of files. If it's not the consensus solution, then discuss it first among yourselves before requesting an implementation.

Having /var in a different partition is as problematic as having /home or /tmp in a different partition too. If the former hasn't been a problem until now, using /var shouldn't be a problem too.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1358
>> +            processName = "GStreamer";
> 
> shouldn't it be WebKitWebProcess by default?

That's what g_get_prgname() should return, yes. In the very unlikely case that it returns NULL, I'm just defaulting to the same value that GstUriDecodeBin would:

https://github.com/GStreamer/gst-plugins-base/blob/1.8.0/gst/playback/gsturidecodebin.c#L1951

>> 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?

The "$HOME/.cache" vs. "/var/tmp" discussion was about if the files were considered as "cache" or as "temporary". It's the second, so they belong to a temporary directory.

This purging is here just for legacy files generated by WebKit prior to this patch. Michael told me that leaking those legacy files wasn't unacceptable. That's why I added the purge method.

Ideally, when all the users have migrated to a WebKit version having this patch, the purge shouldn't be needed anymore and might be removed.

>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1379
>> +        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?

I don't understand this comment. GstDownloadBuffer was already removing the file when the element was destroyed (when destroying the player). That's not enough for us. We want to delete the file immediately after it's created.

The only way I found to get notified of the file creation is listening to changes on the temp-location property, because it's updated with the final (random) filename right after the file is created. "temp-template" is just a template. The file isn't created when you set it, but when GstDownloadBuffer decides that it's time to create the file.

>> 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?

Right, I should have set it to null in downloadBufferFileCreatedCallback(), when it's not needed anymore.