Bug 197684

See also Bug 196289 Comment 28.

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

Created attachment 389293 [details] patch WinCairo now passes 30/31 disk-cache tests with these changes.

Created attachment 389299 [details] patch Fixing style and GTK/Mac build issues

Created attachment 389410 [details] patch more build fixes and fixing GTK assert

Created attachment 389411 [details] patch style fixes

Created attachment 389437 [details] patch Adding flag to FileSystem::openFile can open with 0600 directly instead of 0666 then fchmod to 0600.

Created attachment 389438 [details] patch fixing mac builds

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?

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

Created attachment 389719 [details] patch updated patch

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.

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?

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

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.

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.

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

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

Created attachment 390697 [details] patch Updated patch

Created attachment 390698 [details] patch Style fix

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.

Comment on attachment 390698 [details] patch LGTM

(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! 💪

The commit-queue encountered the following flaky tests while processing attachment 390698 [details]: editing/spelling/spellcheck-async-remove-frame.html bug 158401 (authors: morrita@google.com, rniwa@webkit.org, and tony@chromium.org) The commit-queue is continuing to process your patch.

Comment on attachment 390698 [details] patch Clearing flags on attachment: 390698 Committed r256633: <https://trac.webkit.org/changeset/256633>

All reviewed patches have been landed. Closing bug.

<rdar://problem/59467397>

Re-opened since this is blocked by bug 207807

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.

Created attachment 391126 [details] patch fixing gtk build

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

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.

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.

Created attachment 391362 [details] patch mac build fix

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.

Created attachment 391632 [details] Patch Added onlyCreateFile parameter to openFile instead of using a new function.

Created attachment 391685 [details] patch Ensure the salt file is deleted if it exists but doesn't match the needed salt size.

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.

Created attachment 391756 [details] Patch for landing

Created attachment 391757 [details] Patch for landing

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

Created attachment 391772 [details] Patch for landing

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 commit-queue@webkit.org... 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

Created attachment 391776 [details] Patch for landing

Comment on attachment 391776 [details] Patch for landing Clearing flags on attachment: 391776 Committed r257518: <https://trac.webkit.org/changeset/257518>

All reviewed patches have been landed. Closing bug.