Recent versions of libsoup remove any file in cache dir not referenced by the index when the cache is loaded to workaround leaked resources when load/dump is unbalanced for whatever reason like a crash. We currently use $XDG_CACHE_HOME/app-name as default disk cache directory, but that directory could be used by apps to cache other things, and the soup cache might end up deleting other stuff. The soup cache assumes the given directory is only for the disk cache, so we should ensure that. See https://bugzilla.gnome.org/show_bug.cgi?id=667682.
Created attachment 251335 [details] Patch
Comment on attachment 251335 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251335&action=review > Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:86 > + String diskCachePath = WebCore::pathByAppendingComponent(parameters.diskCacheDirectory, "webkit"); How do you feel about webkitgtk-disk-cache or something like that?
This patch looks good. It certainly solves the problem going forward. As a bonus, I think we should also check if the original directory $XDG_CACHE_HOME/app-name contains a soup cache, and if so delete the entire directory so that we automatically clean up the original cache rather than leaving it on disk forever. Simply because it's easy to do, and we had users complaining about rather spectacular cache leaks there (60 GB at [1], 12 GB at [2]). This should not be a problem because an application that stores anything under $XDG_CACHE_HOME should be prepared for it to disappear. [1] https://lists.webkit.org/pipermail/webkit-gtk/2014-July/0 [2] https://bugzilla.gnome.org/show_bug.cgi?id=667682
Fixed link: https://lists.webkit.org/pipermail/webkit-gtk/2014-July/001980.html
(In reply to comment #2) > Comment on attachment 251335 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251335&action=review > > > Source/WebKit2/NetworkProcess/soup/NetworkProcessSoup.cpp:86 > > + String diskCachePath = WebCore::pathByAppendingComponent(parameters.diskCacheDirectory, "webkit"); > > How do you feel about webkitgtk-disk-cache or something like that? I avoided using gtk because this code is common among ports using SOUP and adding an #ifdef just for the name is probably too much noise for little benefit. I considered webkitcache, but it sounded redundant because it's in the $XDG_CACHE_HOME directory. Also, the new cache always creates this extra dir, named WebKitCache, so that would conflict with webkitcache in some file systems.
(In reply to comment #3) > This patch looks good. It certainly solves the problem going forward. > > As a bonus, I think we should also check if the original directory > $XDG_CACHE_HOME/app-name contains a soup cache, and if so delete the entire > directory so that we automatically clean up the original cache rather than > leaving it on disk forever. Simply because it's easy to do, and we had users > complaining about rather spectacular cache leaks there (60 GB at [1], 12 GB > at [2]). This should not be a problem because an application that stores > anything under $XDG_CACHE_HOME should be prepared for it to disappear. Yes I agree, we can even try to be more conservative and remove only the files containing only numeric characters and all soup.cache*, and leaving subdirectories. I'll update the patch. We will probably need to do the same when we switch to the new cache. > [1] https://lists.webkit.org/pipermail/webkit-gtk/2014-July/0 > [2] https://bugzilla.gnome.org/show_bug.cgi?id=667682
Created attachment 251420 [details] Updated patch Clear the previous soup cache if exists
Comment on attachment 251420 [details] Updated patch View in context: https://bugs.webkit.org/attachment.cgi?id=251420&action=review > Source/WebCore/platform/network/soup/SoupNetworkSession.cpp:170 > +static inline bool strIsNumeric(const char* str) This should probably be called stringIsInteger.
Committed r183255: <http://trac.webkit.org/changeset/183255>