Disable vnode guard related simulated crashes for WKTR / DRT and WebSQL.
<rdar://problem/40674034>
Created attachment 344149 [details] Patch
Created attachment 344150 [details] Patch
Comment on attachment 344150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344150&action=review > Source/WebCore/platform/posix/FileSystemPOSIX.cpp:454 > +String realPath(const String& filePath) > +{ > + CString fsRep = fileSystemRepresentation(filePath); > + char resolvedName[PATH_MAX]; > + const char* result = realpath(fsRep.data(), resolvedName); > + return result ? String::fromUTF8(result) : filePath; > +} This doesn't look right to me. The idea behind fileSystemRepresentation() is that this is what one passes to POSIX functions, but here, the result is round-tripped through String::fromUTF8() and then utf8(). I don't really have an example of when this would fail, but this is conceptually incorrect and of course wasteful.
Comment on attachment 344150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344150&action=review >> Source/WebCore/platform/posix/FileSystemPOSIX.cpp:454 >> +} > > This doesn't look right to me. The idea behind fileSystemRepresentation() is that this is what one passes to POSIX functions, but here, the result is round-tripped through String::fromUTF8() and then utf8(). I don't really have an example of when this would fail, but this is conceptually incorrect and of course wasteful. I am passing the result of fileSystemRepresentation() to a Posix function: realpath(). realpath() returns a const char* so I have to call String::fromUTF8() to return a String. I do not see what is wrong with my code. Also, if you look at the implementation of directoryName(), it is extremely similar: String directoryName(const String& path) { CString fsRep = fileSystemRepresentation(path); if (!fsRep.data() || fsRep.data()[0] == '\0') return String(); return String::fromUTF8(dirname(fsRep.mutableData())); }
Created attachment 344196 [details] Patch
Comment on attachment 344196 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=344196&action=review > Source/WebCore/Modules/webdatabase/cocoa/DatabaseManagerCocoa.mm:43 > + setenv("SQLITE_EXEMPT_PATH_FROM_VNODE_GUARDS", FileSystem::realPath(databasePath).utf8().data(), 0 /* overwrite */); A minor point, but I prefer this style for giving symbolic names to constant arguments: int overwrite = 0; setenv("SQLITE_EXEMPT_PATH_FROM_VNODE_GUARDS", FileSystem::realPath(databasePath).utf8().data(), overwrite); It's nice when we can document our intent in code. Code has some nice advantages over comments, like compiler validation and debugging and so on.
> The idea behind fileSystemRepresentation() is > that this is what one passes to POSIX functions This function calls fileSystemRepresentation() in order to pass the result to the POSIX function realpath(). So, I think that part of the function matches your expectation. > but here, the result is > round-tripped through String::fromUTF8() and then utf8(). I don't really > have an example of when this would fail, but this is conceptually incorrect > and of course wasteful. I think your comment here is about the return value type? Most functions in FileSystemPOSIX.cpp return String. You could imagine returning CString from this function because that's what POSIX provides, and that's what our caller wants. On the other hand, there's a conflict here between POSIX and Windows. Windows returns a buffer of UChar (i.e. String). So returning CString would require getFinalPathName() in FileSystemWin to convert from UChar buffer to CString. Maybe that's the right tradeoff to make POSIX slightly faster?
Created attachment 344215 [details] Patch
Comment on attachment 344215 [details] Patch I’m going to say r+ to move this fix along — but we can continue to discuss whether there’s a good way to avoid utf8 round tripping.
Comment on attachment 344215 [details] Patch Clearing flags on attachment: 344215 Committed r233487: <https://trac.webkit.org/changeset/233487>
All reviewed patches have been landed. Closing bug.
I like Geoff's suggestion. Alternatively, maybe there is a way to restructure code so that it doesn't even need to be called on Windows. Realpath is a POSIX concept, so adding a Windows function with this name which does nothing is quite misleading. Or maybe we need to actually do something on Windows too, like https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi-getfullpathnamea
(In reply to Alexey Proskuryakov from comment #13) > I like Geoff's suggestion. Alternatively, maybe there is a way to > restructure code so that it doesn't even need to be called on Windows. > Realpath is a POSIX concept, so adding a Windows function with this name > which does nothing is quite misleading. > Or maybe we need to actually do something on Windows too, like > https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi- > getfullpathnamea This is inaccurate, my patch did provide a meaningful Windows implementation. Note that the very first patch you took a look at did not have a review flag and the lack of windows implementation was one of the reason.
> Or maybe we need to actually do something on Windows too, like > https://docs.microsoft.com/en-us/windows/desktop/api/fileapi/nf-fileapi- > getfullpathnamea Right, it seems clear that Windows does have a concept of "~" and similar abbreviations in file names.