Bug 145800 – NetworkCache: Delete old cache versions

• Component: Page Loading
• Status: RESOLVED FIXED

[Antti Koivisto]

We should wipe out any leftover data when incrementing cache version.

[Antti Koivisto]

Created attachment 254567 [details]
patch

[WebKit Commit Bot]

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.

[Antti Koivisto]

Created attachment 254568 [details]
patch

[WebKit Commit Bot]

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.

[Darin Adler]

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.

[Chris Dumez]

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.

[Darin Adler]

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.

[Chris Dumez]

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

[Antti Koivisto]

Created attachment 254646 [details]
patch

[WebKit Commit Bot]

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.

[Antti Koivisto]

https://trac.webkit.org/r185412 

(KaL promised to look into the gtk build issue)

[Antti Koivisto]

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