Bug 145800

Summary: NetworkCache: Delete old cache versions
Product: WebKit Reporter: Antti Koivisto <koivisto>
Component: Page LoadingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: andersca, cdumez, commit-queue
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch
none
patch
darin: review+
patch none

Description Antti Koivisto 2015-06-09 07:07:15 PDT
We should wipe out any leftover data when incrementing cache version.
Comment 1 Antti Koivisto 2015-06-09 07:22:04 PDT
Created attachment 254567 [details]
patch
Comment 2 WebKit Commit Bot 2015-06-09 07:24:11 PDT
Attachment 254567 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:162:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.h:26:  #ifndef header guard has wrong style, please use: NetworkCacheFileSystem_h  [build/header_guard] [5]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:102:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 10 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Antti Koivisto 2015-06-09 07:35:01 PDT
Created attachment 254568 [details]
patch
Comment 4 WebKit Commit Bot 2015-06-09 07:37:24 PDT
Attachment 254568 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:162:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.h:26:  #ifndef header guard has wrong style, please use: NetworkCacheFileSystem_h  [build/header_guard] [5]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:102:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:57:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 5 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 5 Darin Adler 2015-06-09 09:56:48 PDT
Comment on attachment 254568 [details]
patch

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

Looks like this does not build on GTK yet.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:69
> +        function(String(name), directoryEntryType(dp->d_type));

Doing String(name) here interprets the filename as Latin-1. That’s not right. This needs to be the reverse of WebCore::fileSystemRepresentation. I see some code using String::fromUTF8 for this; openTemporaryFile on the Mac for example. This only works because we are lucky enough to not have any non-ASCII filenames in the directory. Of course, decoding UTF-8 has the inverse problem if there are some file names that we can’t represent as String. I suppose we have to just skip those files. This can’t happen in normal circumstances on OS X, but might happen in other cases on other platforms perhaps.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.h:57
> +        function(String(name));

Same String(name) issue here.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:123
> +        auto partitionPath = WebCore::pathByAppendingComponent(recordsPath, subdirName);
> +        WebCore::deleteEmptyDirectory(partitionPath);

Not sure the local variable makes this easier to read

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:878
> +            String versionString = subdirName.substring(strlen(versionDirectoryPrefix));
> +            bool success;
> +            unsigned directoryVersion = versionString.toUIntStrict(&success);

Should do this with a StringView. No need to copy out a new string just to convert to integer.
Comment 6 Chris Dumez 2015-06-09 09:57:17 PDT
Comment on attachment 254568 [details]
patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:62
> +    struct dirent* dp;

I think the "struct" is superfluous in C++.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:69
> +        function(String(name), directoryEntryType(dp->d_type));

Probably not a big deal for what you're using it for but this likely only works if the name only include latin1 characters. I think String::fromUTF8(name) would be more correct.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:94
> +    struct stat fileInfo;

struct is superfluous I believe.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:121
> +    utimes(WebCore::fileSystemRepresentation(path).data(), 0);

nullptr

> Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.h:2
> + * Copyright (C) 2015 Apple Inc. All rights reserved.

Not commenting on this file as it seems it is the diff being confused.

> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:876
> +            String versionString = subdirName.substring(strlen(versionDirectoryPrefix));

We could use sizeof(versionDirectoryPrefix)-1 to resolve this at compile time instead of iterating over the string at runtime.
Comment 7 Darin Adler 2015-06-09 09:59:57 PDT
Comment on attachment 254568 [details]
patch

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

Setting review+ again.

>> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:876
>> +            String versionString = subdirName.substring(strlen(versionDirectoryPrefix));
> 
> We could use sizeof(versionDirectoryPrefix)-1 to resolve this at compile time instead of iterating over the string at runtime.

Modern compilers like clang constant fold calls to strlen, so this is resolved at compile time.
Comment 8 Chris Dumez 2015-06-09 10:07:38 PDT
Comment on attachment 254568 [details]
patch

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

>>> Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:876
>>> +            String versionString = subdirName.substring(strlen(versionDirectoryPrefix));
>> 
>> We could use sizeof(versionDirectoryPrefix)-1 to resolve this at compile time instead of iterating over the string at runtime.
> 
> Modern compilers like clang constant fold calls to strlen, so this is resolved at compile time.

Ah, I did not know that. This is good to know. Please disregard my comment then, strlen() is a lot more readable :)
Comment 9 Antti Koivisto 2015-06-10 05:24:32 PDT
Created attachment 254646 [details]
patch
Comment 10 WebKit Commit Bot 2015-06-10 05:31:51 PDT
Attachment 254646 [details] did not pass style-queue:


ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:162:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.h:39:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.cpp:102:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/NetworkProcess/cache/NetworkCacheFileSystem.cpp:57:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 4 in 12 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 11 Antti Koivisto 2015-06-10 06:02:30 PDT
https://trac.webkit.org/r185412 

(KaL promised to look into the gtk build issue)
Comment 12 Antti Koivisto 2015-06-10 06:04:29 PDT
> Should do this with a StringView. No need to copy out a new string just to
> convert to integer.

Didn't do this. It looked too ugly for a meaningless performance optimization (or maybe I just don't know how to use StringView correctly).