Comment 28 for bug 1203240

(In reply to comment #22)
> Comment on attachment 300331 [details]
> 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.

Ok, my fault, I tried to help reviewing this, but I didn't read the discussion first, there's even a comment by me suggesting not to use home cache dir :-P

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

Yes, sure, but it's more unlikely. Anyway, I only asked. I know, for example, that we write the partial download files in the same download directory instead of /tmp to avoid copies between partitions.

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

Because gst can't know in which process is running, but we know it.

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

Right, ok.

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

Ah, I see, I wonder if we should purge also leaked files from previous sessions too. That would solve the problem for systems where /var/tmp is not cleaned, or people who hardly ever reboot. Btw, I have old files in my /var/tmp so I'm not sure systemd is cleaning it up, and I reboot every day. All of them are actually empty dirs though.

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

Ok, understood. I was wondering if setting the template would set the location too, but it doesn't.

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