It would be useful to know why the 'deleteFile' failed so that we could distinguish between multiple processes competing for a file lock, or bad permissions settings on the directories used by WebKit, etc.
<rdar://problem/35008951>
Created attachment 376209 [details] Patch
Comment on attachment 376209 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376209&action=review > Source/WebCore/Modules/webdatabase/DatabaseTracker.cpp:865 > + LOG_ERROR("Error deleting database file %s", file.utf8().data()); What are the privacy implications of logging file names?
Comment on attachment 376209 [details] Patch Just passing by, can talk in person if you want advice.... I think this is your first bug, so good job on with picking up the process 🙂. Here are somethings that struck out at me when I glanced at this patch: 1. Logging is only at select call sites (I cannot tell if you’ve covered all call sites). 2. Logging is not in FileSystem::deleteFile, which I expected from the bug title. 3. You log things if deleteNonEmptyDirectory fails, which I didn’t expect from the bug title. 4. The logging does not include an error status, which I expected from the bug title. 5. The logging includes file names, which I didn’t expect from the bug title. 6. Alexey’s point 7. I think the logging is debug only and I’m unclear if that is a large enough audience for us to see thing. 8. I think this may be using the wrong logging mechanism: why not use log channels? ChangeLog message doesn’t explain. 9. ChangeLog message doesn’t explain how this logging makes things easier/what kinds of bugs we are trying to diagnose.
Created attachment 376314 [details] Patch
Comment on attachment 376314 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376314&action=review > Source/WTF/wtf/posix/FileSystemPOSIX.cpp:75 > + bool unlinkResult = unlink(fsRep.data()); I would propose: int unlinkResult = unlink(fsRep.data()); Or: bool unlinked = !unlink(fsRep.data()); if (!unlinked) LOG_ERROR(.... return unlinked;
Created attachment 376321 [details] Patch
Comment on attachment 376314 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=376314&action=review > Source/WTF/wtf/posix/FileSystemPOSIX.cpp:70 > + LOG_ERROR("Failed to get filesystem representation to create CString from cfString or filesystem representation is a null value"); I suggest prepending "File failed to delete. " or "Could not delete a file. " to this for some context.
Created attachment 376332 [details] Patch
Comment on attachment 376332 [details] Patch Looks good to me. r=me
Comment on attachment 376332 [details] Patch Clearing flags on attachment: 376332 Committed r248702: <https://trac.webkit.org/changeset/248702>
All reviewed patches have been landed. Closing bug.
I'm hitting this a bunch locally... do you want bugs about cases where this logging happens, or what?
(In reply to Tim Horton from comment #13) > I'm hitting this a bunch locally... do you want bugs about cases where this > logging happens, or what? This doesn't need to be reported. It's for future bugs where the error might be meaningful in the context of the bug.
I think we might eventually need to do something different. Sometimes something calls "deleteFile" and knows the file might not exist, and does not want that information logged to the console. If the failure is unexpected the logging is more useful. It would be nice to offer a way to log when the failure is unexpected only, even if this makes the code a tiny bit more complex. Ideally we’d quiet the "hitting it a bunch locally" almost entirely with this technique, then we'd see logs for the unexpected failures. Function calls that report their failures "directly to the user" are something I consider an anti-pattern. But I think we can cancel that out if we distinguish the expected failures from the unexpected ones.
(In reply to Tim Horton from comment #13) > I'm hitting this a bunch locally... do you want bugs about cases where this > logging happens, or what? If this logging is getting spammy we might want to make it an INFO level log entry. It would be interesting to know why the delete is failing, and whether it makes sense to call delete for that call stack, or whether we should be taking some action when the delete fails. We might need to pass a flag indicating that failure is expected/normal in some cases, and not log when that happens.
I could add a parameter to the deleteFile function that defaults to "never log." Then in the places where it is important (like the places I put the logging in the original patch upload, i.e. places where !FileSystem::deleteFile() is included in the code), I could set the parameter to "always log." Then any additional places we find along the way where logging might be helpful can also use the parameter as needed. Does that sound okay?
I think something like that is good. But maybe the default could go the other way. The callers that expect errors could indicate that they expect them. However, I wouldn’t want the logging to ignore *all* errors, just the specific error that is expected. So the goal for the argument would be to say what error is expected.
I agree. So, to summarize, the default would be to always log. Then the caller passes an argument to indicate any errors they expect that then won't be logged. One idea is the argument could be value(s) chosen from an enum of potential error codes of the unlink() function. I could also add in comments next to each code to ensure people know the correct code for the expected error, for ex, ENOENT is the "no such file or directory" error). If anyone has other ideas, I'd be happy to hear them too.
Yes, I think it should be an OptionSet of expected errors, but not literally POSIX errors like ENOENT since the point of FileSystem is to abstract away details of the operating system. So an OptionSet that expresses the types of errors that are expected, probably using an enumeration. We’d add them as we need new concepts. I assume one value in the enumeration would be "it's OK if the file doesn't exist". Not sure which others we’d find we need.
Not exactly "expected errors" but "unsurprising errors, which we should ignore silently without logging".
Now every layout test logs: 12:46:33.026 12287 ERROR: File failed to delete. Error message: No such file or directory 12:46:33.026 12287 /Volumes/Data/Development/system/webkit/OpenSource/Source/WTF/wtf/posix/FileSystemPOSIX.cpp(77) : bool WTF::FileSystemImpl::deleteFile(const WTF::String &) 12:46:33.026 12287 ERROR: File failed to delete. Error message: No such file or directory 12:46:33.026 12287 /Volumes/Data/Development/system/webkit/OpenSource/Source/WTF/wtf/posix/FileSystemPOSIX.cpp(77) : bool WTF::FileSystemImpl::deleteFile(const WTF::String &) 12:46:33.026 12287 ERROR: File failed to delete. Error message: No such file or directory 12:46:33.026 12287 /Volumes/Data/Development/system/webkit/OpenSource/Source/WTF/wtf/posix/FileSystemPOSIX.cpp(77) : bool WTF::FileSystemImpl::deleteFile(const WTF::String &) 12:46:33.026 12287 ERROR: File failed to delete. Error message: No such file or directory 12:46:33.026 12287 /Volumes/Data/Development/system/webkit/OpenSource/Source/WTF/wtf/posix/FileSystemPOSIX.cpp(77) : bool WTF::FileSystemImpl::deleteFile(const WTF::String &) 12:46:33.026 12287 ERROR: File failed to delete. Error message: No such file or directory 12:46:33.026 12287 /Volumes/Data/Development/system/webkit/OpenSource/Source/WTF/wtf/posix/FileSystemPOSIX.cpp(77) : bool WTF::FileSystemImpl::deleteFile(const WTF::String &) 12:46:33.026 12287 ERROR: File failed to delete. Error message: No such file or directory 12:46:33.026 12287 /Volumes/Data/Development/system/webkit/OpenSource/Source/WTF/wtf/posix/FileSystemPOSIX.cpp(77) : bool WTF::FileSystemImpl::deleteFile(const WTF::String &) 12:46:33.026 12287 ERROR: File failed to delete. Error message: No such file or directory 12:46:33.026 12287 /Volumes/Data/Development/system/webkit/OpenSource/Source/WTF/wtf/posix/FileSystemPOSIX.cpp(77) : bool WTF::FileSystemImpl::deleteFile(const WTF::String &)