Comment 30 for bug 1203240

Comment on attachment 300331
Patch

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

>>> Source/WebCore/platform/graphics/gstreamer/MediaPlayerPrivateGStreamer.cpp:1360
>>> + GUniquePtr<gchar> newDownloadTemplate(g_strdup_printf("/var/tmp/%s-XXXXXX", processName));
>>
>> Use g_build_filename
>
> char.
>
> Another thing is that I think we should ensure the files are created in a different directory for each user and ensure that directory has the proper permissions so that it cannot be access by anyone else other than the user. I think media people watch online can vary a lot from cats to other things. I'd write a function to return the template and ensure it is placed in the proper directory.

This is tricky and, IMHO, not needed.

Having and extra directory under /var/tmp means that we would have to maintain (delete) that directory by ourselves. This is problematic because we can't use the elegant trick of deleting the file right after its creation to delete the directory. If several videos are being played at once by different WebProcesses a race condition can happen:

- Process A creates /var/tmp/$USER
- Process A creates WebKit-Media-123456 there
- Process B tries to create /var/tmp/$USER (but it's already created, no problem)
- Process A deletes WebKit-Media-123456 there and /var/tmp/$USER
- Process B creates WebKit-Media-ABCDEF there but... the directory doesn't exist anymore. ERROR!

Anyway, the directory itself isn't needed because the new temporary files are now deleted right after their creation. This means that no other processes can open them anymore, so there shouldn't be any concern regarding privacy. I've checked it by myself with a "while true; do ls /var/tmp; done" script.

>>>>> 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.
>>
>> Right, ok.
>
> Yep, this will purge the old files. I have a concern though for the files that fall under the GST_WARNING of line 1377 of not being able to unlink. Should we assume that is is very unlikely and forget (and trust distros to purge this from time to time)? Btw, if it is so unlikely, we can tag the decission as UNLIKELY().

Michael would say that WebKit should try hard to clean its own files. I don't think distros are going to mess with the contents of the user home dir, even for the .cache dir. Thanks for the UNLIKELY() suggestion, btw.