Bug 197684 – [Win] Implement NetworkCache::Data by using FileSystem::MappedFileData

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Fujii Hironori]

[Win] Implement NetworkCache::Data by using FileSystem::MappedFileData

We need to implement FileSystem::MappedFileData for Windows first.

[Fujii Hironori]

See also Bug 196289 Comment 28.

[Fujii Hironori]

(In reply to Fujii Hironori from comment #0)
> We need to implement FileSystem::MappedFileData for Windows first.

Filed this in Bug 198269.

[Christopher Reid]

Created attachment 389293 [details]
patch

WinCairo now passes 30/31 disk-cache tests with these changes.

[Christopher Reid]

Created attachment 389299 [details]
patch

Fixing style and GTK/Mac build issues

[Christopher Reid]

Created attachment 389410 [details]
patch

more build fixes and fixing GTK assert

[Christopher Reid]

Created attachment 389411 [details]
patch

style fixes

[Christopher Reid]

Created attachment 389437 [details]
patch

Adding flag to FileSystem::openFile can open with 0600 directly instead of 0666 then fchmod to 0600.

[Christopher Reid]

Created attachment 389438 [details]
patch

fixing mac builds

[Fujii Hironori]

Comment on attachment 389438 [details]
patch

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

> LayoutTests/platform/wincairo/TestExpectations:856
> +

Sort alphabetically. Put http/tests/cache/* after http/tests/blink/sendbeacon before http/tests/cache-storage .

> Source/WTF/wtf/win/FileSystemWin.cpp:439
> +        creationDisposition = CREATE_ALWAYS;

CREATE_ALWAYS always truncates the file. Why is this OK?

> Source/WTF/wtf/win/PathWalker.cpp:35
> +    String path = makeString(directory, "\\"_s, pattern);

'\\' is slightly better than "\\"_s.

> Source/WebKit/NetworkProcess/cache/NetworkCacheDataCurl.cpp:47
> +    return Data(WTF::nullopt);

This is null Data. Data::empty() returns the empty Data in other ports. Is this OK?

[Christopher Reid]

(In reply to Fujii Hironori from comment #9)
> Comment on attachment 389438 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=389438&action=review
> 
> > LayoutTests/platform/wincairo/TestExpectations:856
> > +
> 
> Sort alphabetically. Put http/tests/cache/* after
> http/tests/blink/sendbeacon before http/tests/cache-storage .
> 
will fix this.

> > Source/WTF/wtf/win/FileSystemWin.cpp:439
> > +        creationDisposition = CREATE_ALWAYS;
> 
> CREATE_ALWAYS always truncates the file. Why is this OK?
> 
It seems like this was only working for this case because if the file exists it gets deleted before this call so it's not called with a non-empty file. https://github.com/WebKit/webkit/blob/master/Source/WebKit/NetworkProcess/cache/NetworkCacheBlobStorage.cpp#L111.
O_RDWR doesn't seem needed for this case then.

> > Source/WTF/wtf/win/PathWalker.cpp:35
> > +    String path = makeString(directory, "\\"_s, pattern);
> 
> '\\' is slightly better than "\\"_s.
> 
will fix this.

> > Source/WebKit/NetworkProcess/cache/NetworkCacheDataCurl.cpp:47
> > +    return Data(WTF::nullopt);
> 
> This is null Data. Data::empty() returns the empty Data in other ports. Is
> this OK?
Right, this should use an empty vector instead of null.

[Christopher Reid]

Created attachment 389719 [details]
patch

updated patch

[Brent Fulgham]

Comment on attachment 389719 [details]
patch

I think this patch looks good from the Windows code standpoint, but we need Antti or another cache person to double-check the cache logic.

[Alex Christensen]

Comment on attachment 389719 [details]
patch

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

> Source/WTF/wtf/FileSystem.cpp:336
> +    default:
> +        ASSERT_NOT_REACHED();

Could we remove the default and put EventsOnly?

> Source/WTF/wtf/FileSystem.h:87
> +enum class FileAccessPermission {

: bool

> Source/WTF/wtf/posix/FileSystemPOSIX.cpp:96
>          platformFlag |= O_EVTONLY;

Please also change this to a switch.

> Source/WTF/wtf/win/FileSystemWin.cpp:638
> +    default:
> +        ASSERT_NOT_REACHED();

Again, please remove the default.

> Source/WebKit/NetworkProcess/cache/NetworkCacheDataCurl.cpp:96
> +    memcpy(buffer.data(), map, size);

Do we really want this memcpy here?  Doesn't that defeat the purpose of having mapped memory?

[Adrian Perez]

Comment on attachment 389719 [details]
patch

Overall it's super nice to see common code being cleaned up and operating
system specific guards being removed \o/

Adding to Alex's comments, the changes to the GLib/Soup/Unix bits look good
to me, with the caveat that the API test /webkit/WebKitWebsiteData/databases
is not supposed to be flaky so I would say we need it to keep it passing
before we can land this—please drop me a line (or anyone else) on IRC if
you need a hand figuring out why it might be failing :-]


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

> Source/WTF/wtf/posix/FileSystemPOSIX.cpp:103
> +        permissionFlag |= 0666;

I would probably use (S_IRUSR|S_IWUSR|S_IRGRP|S_IWGRP|S_IROTH|S_IWOTH)
for the sake of consistency, even though 0666 is a common idiom.

(Note: with _BSD_SOURCE there is a DEFFILEMODE macro which expands to
the expression above, but it won't be available in all systems so I'd
rather avoid using this macro.)

[Christopher Reid]

Created attachment 390418 [details]
patch

Updated patch addressing comments from Alex and Adrian.

I updated NetworkCacheDataCurl to use a ref counted Variant<Vector<uint8_t>, FileSystem::MappedFileData> to act like the ref counted buffers for mmap/allocated containers in Soup and Mac ports so the mapped file can be kept around.

I'll be digging in to that GTK failure today.

[Fujii Hironori]

Comment on attachment 390418 [details]
patch

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

> Source/WTF/wtf/win/FileSystemWin.cpp:-438
> -        ASSERT_NOT_REACHED();

Why did you remove this ASSERT_NOT_REACHED? IIRC, MSVC reports compilation warnings.

> Source/WTF/wtf/win/FileSystemWin.cpp:551
> +    if (!GetDiskFreeSpaceExW(path.wideCharacters().data(), &freeBytesAvailableToCaller, &totalNumberOfBytes, &totalNumberOfFreeBytes))

GetDiskFreeSpaceExW can take NULL. Replace totalNumberOfBytes, totalNumberOfFreeBytes with nullptr.

> Source/WTF/wtf/win/FileSystemWin.cpp:554
> +    if (freeBytesAvailableToCaller.QuadPart > static_cast<ULONGLONG>(std::numeric_limits<uint64_t>::max()))

ULONGLONG and uint64_t are same in x64 Windows. Is this code only for 32bit Windows?
If so, how about enclosing it with #if defined(_M_IX86).
https://docs.microsoft.com/en-us/windows/win32/winprog/windows-data-types

> Source/WTF/wtf/win/FileSystemWin.cpp:555
> +        return false;

If you have plenty of storage, it always return false.
What do you think returning std::numeric_limits<uint64_t>::max() in this case?

> Source/WebKit/NetworkProcess/cache/NetworkCacheData.h:102
> +    Box<Variant<Vector<uint8_t>, FileSystem::MappedFileData>> m_buffer;

Box<T> manages T as ThreadSafeRefCounted. Do you need to manage T by thread-safe and ref-counted?
What about using Optional<T> or unique_ptr<T> ?
Variant is already marked by WTF_MAKE_FAST_ALLOCATED.

[Konstantin Tokarev]

(In reply to Fujii Hironori from comment #16)
> Comment on attachment 390418 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=390418&action=review
> 
> > Source/WTF/wtf/win/FileSystemWin.cpp:-438
> > -        ASSERT_NOT_REACHED();
> 
> Why did you remove this ASSERT_NOT_REACHED? IIRC, MSVC reports compilation
> warnings.

If assert is really needed to avoid warning, then release build should have this warning because it's not a RELEASE_ASSERT_NOT_REACHED()

[Christopher Reid]

Comment on attachment 390418 [details]
patch

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

>>> Source/WTF/wtf/win/FileSystemWin.cpp:-438
>>> -        ASSERT_NOT_REACHED();
>> 
>> Why did you remove this ASSERT_NOT_REACHED? IIRC, MSVC reports compilation warnings.
> 
> If assert is really needed to avoid warning, then release build should have this warning because it's not a RELEASE_ASSERT_NOT_REACHED()

Right should be a warning in MSVC if there was an unhandled enum class value. All the FileOpenMode cases for windows are handled here so we shouldn't get a warning.

>> Source/WTF/wtf/win/FileSystemWin.cpp:551
>> +    if (!GetDiskFreeSpaceExW(path.wideCharacters().data(), &freeBytesAvailableToCaller, &totalNumberOfBytes, &totalNumberOfFreeBytes))
> 
> GetDiskFreeSpaceExW can take NULL. Replace totalNumberOfBytes, totalNumberOfFreeBytes with nullptr.

That's good to know, I'll take them out.

>> Source/WTF/wtf/win/FileSystemWin.cpp:554
>> +    if (freeBytesAvailableToCaller.QuadPart > static_cast<ULONGLONG>(std::numeric_limits<uint64_t>::max()))
> 
> ULONGLONG and uint64_t are same in x64 Windows. Is this code only for 32bit Windows?
> If so, how about enclosing it with #if defined(_M_IX86).
> https://docs.microsoft.com/en-us/windows/win32/winprog/windows-data-types

Oh yeah this seems pretty redundant even on 32-bit windows.
On 32-bit ULONGLONG and uint64_t should still both have at least 64-bits of storage.
I'll remove this check.

>> Source/WebKit/NetworkProcess/cache/NetworkCacheData.h:102
>> +    Box<Variant<Vector<uint8_t>, FileSystem::MappedFileData>> m_buffer;
> 
> Box<T> manages T as ThreadSafeRefCounted. Do you need to manage T by thread-safe and ref-counted?
> What about using Optional<T> or unique_ptr<T> ?
> Variant is already marked by WTF_MAKE_FAST_ALLOCATED.

I used this ref-counted container here because there's currently some cases where this class is copied and needs to manage multiple instances of the data.
The case I remember seeing was https://github.com/WebKit/webkit/blob/master/Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.cpp#L99

[Christopher Reid]

Created attachment 390697 [details]
patch

Updated patch

[Christopher Reid]

Created attachment 390698 [details]
patch

Style fix

[Christopher Reid]

This should also fix the GTK failure.

It seemed to be a pre-existing problem where the CacheStorage directory wasn't being created for ports without SandboxExtensions.

Before trying to open CacheStorage/salt was failing silently.
Now with these FileSystem changes GTK's writeToFile fails an assert if the handle isn't valid.

That directory should now be created properly for all ports.

[Fujii Hironori]

Comment on attachment 390698 [details]
patch

LGTM

[Adrian Perez]

(In reply to Christopher Reid from comment #21)
> This should also fix the GTK failure.
> 
> It seemed to be a pre-existing problem where the CacheStorage directory
> wasn't being created for ports without SandboxExtensions.
> 
> Before trying to open CacheStorage/salt was failing silently.
> Now with these FileSystem changes GTK's writeToFile fails an assert if the
> handle isn't valid.
> 
> That directory should now be created properly for all ports.

Nicer find! 💪

[WebKit Commit Bot]

The commit-queue encountered the following flaky tests while processing attachment 390698 [details]:

editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: [e-mail], [e-mail], and [e-mail])
The commit-queue is continuing to process your patch.

[WebKit Commit Bot]

Comment on attachment 390698 [details]
patch

Clearing flags on attachment: 390698

Committed r256633: <https://trac.webkit.org/changeset/256633>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/59467397>

[WebKit Commit Bot]

Re-opened since this is blocked by bug 207807

[Christopher Reid]

Created attachment 391124 [details]
patch

I ended up re-adding FileOpenMode::ReadWrite since mmap was failing on FileSystemPOSIX when the file was created with O_WRONLY. That caused the memory regression in Mac.

I also wanted to check if O_EXCL is needed on this NetworkCacheData mapped file before re-landing. I didn't carry it over to FileSystem::openFile implementations since I don't think most openFile calls using ReadWrite will want to always create a file. Also the parent directory for the caches are created with S_IRWXU access.

If O_EXCL is needed it might make sense then to have something like a FileSystem::createFile call that fails if the file already exists.

[Christopher Reid]

Created attachment 391126 [details]
patch

fixing gtk build

[Yusuke Suzuki]

Note that I already run the patch offered by Chris and ensured that new patch does not cause Membuster regression on macOS.

[Antti Koivisto]

Comment on attachment 391126 [details]
patch

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

> Source/WebKit/NetworkProcess/cache/NetworkCacheData.cpp:45
> -    int fd = open(FileSystem::fileSystemRepresentation(path).data(), O_CREAT | O_EXCL | O_RDWR , S_IRUSR | S_IWUSR);
> -    if (fd < 0)
> -        return { };
> -
> -    if (ftruncate(fd, m_size) < 0) {
> -        close(fd);
> +    auto handle = FileSystem::openFile(path, FileSystem::FileOpenMode::ReadWrite, FileSystem::FileAccessPermission::User);
> +    if (!FileSystem::truncateFile(handle, m_size)) {
> +        FileSystem::closeFile(handle);

Please don't lose O_EXCL (or any other flags). Otherwise in error situation we'l truncate a file backing an existing mapping. This will crash hard.

[Christopher Reid]

Created attachment 391361 [details]
Patch

Added FileSystem::createFile to keep the O_EXCL flag. It will return an invalid handle if the file already exists.

[Christopher Reid]

Created attachment 391362 [details]
patch

mac build fix

[Antti Koivisto]

Comment on attachment 391362 [details]
patch

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

> Source/WTF/wtf/posix/FileSystemPOSIX.cpp:116
> +PlatformFileHandle createFile(const String& path, FileOpenMode mode, FileAccessPermission permission)

Can't exclusivness just be another parameter for openFile? Seems unnecessary to copy the whole function. Also your openFile can still create files and name createFile doesn't really communicate the difference.

[Christopher Reid]

Created attachment 391632 [details]
Patch

Added onlyCreateFile parameter to openFile instead of using a new function.

[Christopher Reid]

Created attachment 391685 [details]
patch

Ensure the salt file is deleted if it exists but doesn't match the needed salt size.

[Yusuke Suzuki]

Comment on attachment 391685 [details]
patch

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

r=me

> Source/WTF/ChangeLog:16
> +        Added onlyCreateFile flag to openFile which causes openFile to fail if the file already exists.

We should name this flag as `failIfFileExists`

> Source/WebKit/NetworkProcess/cache/NetworkCacheData.cpp:43
> +    auto handle = FileSystem::openFile(path, FileSystem::FileOpenMode::ReadWrite, FileSystem::FileAccessPermission::User, true);

Let's not pass boolean directly, it is hard to read what this bool means.

constexpr bool failIfFileExists = true;
auto handle = FileSystem::openFile(path, ..., failIfFileExists);

is better.

[Christopher Reid]

Created attachment 391756 [details]
Patch for landing

[Christopher Reid]

Created attachment 391757 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 391757 [details]
Patch for landing

Rejecting attachment 391757 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-02', 'validate-changelog', '--check-oops', '--non-interactive', 391757, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

ChangeLog entry in LayoutTests/ChangeLog contains OOPS!.

Full output: https://webkit-queues.webkit.org/results/13329377

[Christopher Reid]

Created attachment 391772 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 391772 [details]
Patch for landing

Rejecting attachment 391772 [details] from commit-queue.

Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'land-attachment', '--force-clean', '--non-interactive', '--parent-command=commit-queue', 391772, '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Logging in as [e-mail]...
Fetching: https://bugs.webkit.org/attachment.cgi?id=391772&action=edit
Fetching: https://bugs.webkit.org/show_bug.cgi?id=197684&ctype=xml&excludefield=attachmentdata
Processing 1 patch from 1 bug.
Updating working directory
Processing patch 391772 from bug 197684.
Fetching: https://bugs.webkit.org/attachment.cgi?id=391772
Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/platform/wincairo/TestExpectations
	M	Source/WTF/ChangeLog
	M	Source/WTF/wtf/FileSystem.cpp
	M	Source/WTF/wtf/FileSystem.h
	M	Source/WTF/wtf/glib/FileSystemGlib.cpp
	M	Source/WTF/wtf/posix/FileSystemPOSIX.cpp
	M	Source/WTF/wtf/win/FileSystemWin.cpp
	M	Source/WTF/wtf/win/PathWalker.cpp
	M	Source/WebKit/ChangeLog
	M	Source/WebKit/NetworkProcess/NetworkProcess.cpp
	M	Source/WebKit/NetworkProcess/cache/NetworkCacheData.cpp
	M	Source/WebKit/NetworkProcess/cache/NetworkCacheData.h
	M	Source/WebKit/NetworkProcess/cache/NetworkCacheDataCocoa.mm
	M	Source/WebKit/NetworkProcess/cache/NetworkCacheDataCurl.cpp
	M	Source/WebKit/NetworkProcess/cache/NetworkCacheDataSoup.cpp
	M	Source/WebKit/NetworkProcess/cache/NetworkCacheFileSystem.cpp
	M	Tools/ChangeLog
	M	Tools/TestWebKitAPI/Tests/WTF/FileSystem.cpp

ERROR from SVN:
Merge conflict during commit: Conflict at '/trunk/LayoutTests/ChangeLog'
W: c1bfafc7aabfe735fbd0fdaeed79acbf8a3a31be and refs/remotes/origin/master differ, using rebase:
:040000 040000 3715dea03ca78607882c1c315dd41b063fb3cfd7 148ef925334bff92d196d535b0dac66bb1037bda M	LayoutTests
:040000 040000 82adc380b74dda0797ee354f6e8bade45a6ddd51 8eee6c7b770d71aabbb3cc5bab7b21d43e8dbbeb M	Source
:040000 040000 57674841b68c9058d5d4bbfa659f2fbd7367d9b4 adfcec4e4cae19a6159741b7cffdddced27e95ec M	Tools
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit

Committing to http://svn.webkit.org/repository/webkit/trunk ...
	M	LayoutTests/ChangeLog
	M	LayoutTests/platform/wincairo/TestExpectations
	M	Source/WTF/ChangeLog
	M	Source/WTF/wtf/FileSystem.cpp
	M	Source/WTF/wtf/FileSystem.h
	M	Source/WTF/wtf/glib/FileSystemGlib.cpp
	M	Source/WTF/wtf/posix/FileSystemPOSIX.cpp
	M	Source/WTF/wtf/win/FileSystemWin.cpp
	M	Source/WTF/wtf/win/PathWalker.cpp
	M	Source/WebKit/ChangeLog
	M	Source/WebKit/NetworkProcess/NetworkProcess.cpp
	M	Source/WebKit/NetworkProcess/cache/NetworkCacheData.cpp
	M	Source/WebKit/NetworkProcess/cache/NetworkCacheData.h
	M	Source/WebKit/NetworkProcess/cache/NetworkCacheDataCocoa.mm
	M	Source/WebKit/NetworkProcess/cache/NetworkCacheDataCurl.cpp
	M	Source/WebKit/NetworkProcess/cache/NetworkCacheDataSoup.cpp
	M	Source/WebKit/NetworkProcess/cache/NetworkCacheFileSystem.cpp
	M	Tools/ChangeLog
	M	Tools/TestWebKitAPI/Tests/WTF/FileSystem.cpp

ERROR from SVN:
Merge conflict during commit: Conflict at '/trunk/LayoutTests/ChangeLog'
W: c1bfafc7aabfe735fbd0fdaeed79acbf8a3a31be and refs/remotes/origin/master differ, using rebase:
:040000 040000 3715dea03ca78607882c1c315dd41b063fb3cfd7 148ef925334bff92d196d535b0dac66bb1037bda M	LayoutTests
:040000 040000 82adc380b74dda0797ee354f6e8bade45a6ddd51 8eee6c7b770d71aabbb3cc5bab7b21d43e8dbbeb M	Source
:040000 040000 57674841b68c9058d5d4bbfa659f2fbd7367d9b4 adfcec4e4cae19a6159741b7cffdddced27e95ec M	Tools
Current branch master is up to date.
ERROR: Not all changes have been committed into SVN, however the committed
ones (if any) seem to be successfully integrated into the working tree.
Please see the above messages for details.


Failed to run "['git', 'svn', 'dcommit', '--rmdir']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit
Updating OpenSource
From https://git.webkit.org/git/WebKit
   3e82a3cc5f3..6e9734c7a4b  master     -> origin/master
Partial-rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc ...
Currently at 257504 = 3e82a3cc5f3052c3efd94f1fcd7e5a415cff6081
r257505 = a637395bea617d4b228aee28c27901a985d367d9
r257506 = 527e95c4d5abfe1debc954754e724863585b06b0
r257507 = c79f943807fc8c0263328fe23f54573a3f130024
r257508 = 6e9734c7a4b54261fe14c29ef4c46c599eb2f656
Done rebuilding .git/svn/refs/remotes/origin/master/.rev_map.268f45cc-cd09-0410-ab3c-d52691b4dbfc
First, rewinding head to replay your work on top of it...
Fast-forwarded master to refs/remotes/origin/master.
Total errors found: 0 in 1 files

Full output: https://webkit-queues.webkit.org/results/13329457

[Christopher Reid]

Created attachment 391776 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 391776 [details]
Patch for landing

Clearing flags on attachment: 391776

Committed r257518: <https://trac.webkit.org/changeset/257518>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.