| Summary: | Use more efficient path resolution logic in SandboxExtensions | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Brent Fulgham <bfulgham> | ||||||||||||||||||
| Component: | WebKit2 | Assignee: | Brent Fulgham <bfulgham> | ||||||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||||||
| Severity: | Normal | CC: | bfulgham, commit-queue, darin, ews-watchlist, mjs, rniwa, ryanhaddad, webkit-bot-watchers-bugzilla, webkit-bug-importer | ||||||||||||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||||||
| OS: | Unspecified | ||||||||||||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=197568 | ||||||||||||||||||||
| Bug Depends on: | |||||||||||||||||||||
| Bug Blocks: | 197465, 197568 | ||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||
|
Description
Brent Fulgham
2019-04-29 14:48:19 PDT
Created attachment 368496 [details]
WIP Patch
Comment on attachment 368496 [details]
WIP Patch
Initial patch to see how the various bots like it.
Comment on attachment 368496 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368496&action=review > Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:244 > + if (getattrlist(path.data(), &attrs, &attrBuf, sizeof(attrBuf), FSOPT_ATTR_CMN_EXTENDED) < 0) Found this in documentation: "Not all volumes support getattrlist(). The best way to test whether a volume supports this function is to simply call it and check the error result. getattrlist() will return ENOTSUP if it is not supported on a particular volume." Can we find a higher level API to use that does not have this limitation? Comment on attachment 368496 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368496&action=review >> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:244 >> + if (getattrlist(path.data(), &attrs, &attrBuf, sizeof(attrBuf), FSOPT_ATTR_CMN_EXTENDED) < 0) > > Found this in documentation: > > "Not all volumes support getattrlist(). The best way to test whether a volume supports this function is to simply call it and check the error result. getattrlist() will return ENOTSUP if it is not supported on a particular volume." > > Can we find a higher level API to use that does not have this limitation? I think I have found one, -[NSString stringByResolvingSymlinksInPath] from Foundation. > Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:254 > String stringByResolvingSymlinksInPath(const String& path) This function should probably move out of this file, now that it’s not used by sandbox extension code at all. > Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:281 > + return String::fromUTF8(fileSystemPath); I suggest removing the FIXME that’s above mentioning this specific problem (calling both resolveSymlinksInPath and stringByStandardizingPath). Comment on attachment 368496 [details] WIP Patch Attachment 368496 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12034482 New failing tests: inspector/unit-tests/objectStore/put.html inspector/unit-tests/objectStore/putObject.html inspector/unit-tests/objectStore/delete.html inspector/unit-tests/objectStore/basic.html inspector/unit-tests/objectStore/clear.html inspector/unit-tests/objectStore/deleteObject.html Created attachment 368512 [details]
Archive of layout-test-results from ews105 for mac-highsierra-wk2
The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews105 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment on attachment 368496 [details] WIP Patch Attachment 368496 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12034710 New failing tests: js/dom/custom-constructors.html Created attachment 368519 [details]
Archive of layout-test-results from ews202 for win-future
The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews202 Port: win-future Platform: CYGWIN_NT-6.1-2.10.0-0.325-5-3-x86_64-64bit
Comment on attachment 368496 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368496&action=review >>> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:244 >>> + if (getattrlist(path.data(), &attrs, &attrBuf, sizeof(attrBuf), FSOPT_ATTR_CMN_EXTENDED) < 0) >> >> Found this in documentation: >> >> "Not all volumes support getattrlist(). The best way to test whether a volume supports this function is to simply call it and check the error result. getattrlist() will return ENOTSUP if it is not supported on a particular volume." >> >> Can we find a higher level API to use that does not have this limitation? > > I think I have found one, -[NSString stringByResolvingSymlinksInPath] from Foundation. This does the job but operates on NSString, not C strings. Perhaps the string conversion overhead is acceptable in code that will make a sys call anyway. Comment on attachment 368496 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368496&action=review > Source/WebKit/ChangeLog:16 > + symlinks twice, since NSString's 'stringByStandardizingPath' method does this already. I think we can remove the stringByStandardizingPath call as well, since supposedly the sandbox extension call already resolves symlinks (needs testing with weird symlink setups, but this patch should probably tested that way regardless.) Comment on attachment 368496 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368496&action=review >> Source/WebKit/ChangeLog:16 >> + symlinks twice, since NSString's 'stringByStandardizingPath' method does this already. > > I think we can remove the stringByStandardizingPath call as well, since supposedly the sandbox extension call already resolves symlinks (needs testing with weird symlink setups, but this patch should probably tested that way regardless.) I'm not sure; we serialize the result of this operation across XPC and use that path for various operations. I guess we could just always deal in terms of the non-simplified file path, and know that the sandbox system is granting access to the actual resolved file node (or whatever the underlying representation is), but making that change seems a little scarier than just changing from one API to another. >>>> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:244 >>>> + if (getattrlist(path.data(), &attrs, &attrBuf, sizeof(attrBuf), FSOPT_ATTR_CMN_EXTENDED) < 0) >>> >>> Found this in documentation: >>> >>> "Not all volumes support getattrlist(). The best way to test whether a volume supports this function is to simply call it and check the error result. getattrlist() will return ENOTSUP if it is not supported on a particular volume." >>> >>> Can we find a higher level API to use that does not have this limitation? >> >> I think I have found one, -[NSString stringByResolvingSymlinksInPath] from Foundation. > > This does the job but operates on NSString, not C strings. Perhaps the string conversion overhead is acceptable in code that will make a sys call anyway. Pierre clarified: "getattrlist is supported on all volume types. What may not be is all *attributes*. ATTR_CMN_FULLPATH is supported on all volumes because it is a VFS feature. it is the same as fcntl(fd, F_GETPATH) which is also supported on all volumes." So I think the proposed change is correct as-is. >> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:281 >> + return String::fromUTF8(fileSystemPath); > > I suggest removing the FIXME that’s above mentioning this specific problem (calling both resolveSymlinksInPath and stringByStandardizingPath). Oh, gosh. Silly of me! :-) Created attachment 368624 [details]
Patch
Comment on attachment 368496 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368496&action=review >>>>> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:244 >>>>> + if (getattrlist(path.data(), &attrs, &attrBuf, sizeof(attrBuf), FSOPT_ATTR_CMN_EXTENDED) < 0) >>>> >>>> Found this in documentation: >>>> >>>> "Not all volumes support getattrlist(). The best way to test whether a volume supports this function is to simply call it and check the error result. getattrlist() will return ENOTSUP if it is not supported on a particular volume." >>>> >>>> Can we find a higher level API to use that does not have this limitation? >>> >>> I think I have found one, -[NSString stringByResolvingSymlinksInPath] from Foundation. >> >> This does the job but operates on NSString, not C strings. Perhaps the string conversion overhead is acceptable in code that will make a sys call anyway. > > Pierre clarified: "getattrlist is supported on all volume types. What may not be is all *attributes*. ATTR_CMN_FULLPATH is supported on all volumes because it is a VFS feature. it is the same as fcntl(fd, F_GETPATH) which is also supported on all volumes." > > So I think the proposed change is correct as-is. So the documentation is incorrect. And should be fixed. And also, I think calling -[NSString stringByResolvingSymlinksInPath] would be better than writing this code to call getattrlist. Comment on attachment 368496 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368496&action=review >> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:254 >> String stringByResolvingSymlinksInPath(const String& path) > > This function should probably move out of this file, now that it’s not used by sandbox extension code at all. Why is this code staying in SandboxExtensionCocoa.mm now that it’s not used at all in sandbox extension code? Comment on attachment 368496 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368496&action=review >>> Source/WebKit/ChangeLog:16 >>> + symlinks twice, since NSString's 'stringByStandardizingPath' method does this already. >> >> I think we can remove the stringByStandardizingPath call as well, since supposedly the sandbox extension call already resolves symlinks (needs testing with weird symlink setups, but this patch should probably tested that way regardless.) > > I'm not sure; we serialize the result of this operation across XPC and use that path for various operations. I guess we could just always deal in terms of the non-simplified file path, and know that the sandbox system is granting access to the actual resolved file node (or whatever the underlying representation is), but making that change seems a little scarier than just changing from one API to another. It's potentially a bigger perf win though, so as long as we are changing this code anyway and are able to test properly, might as well make the bigger improvement. >>>>>> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:244 >>>>>> + if (getattrlist(path.data(), &attrs, &attrBuf, sizeof(attrBuf), FSOPT_ATTR_CMN_EXTENDED) < 0) >>>>> >>>>> Found this in documentation: >>>>> >>>>> "Not all volumes support getattrlist(). The best way to test whether a volume supports this function is to simply call it and check the error result. getattrlist() will return ENOTSUP if it is not supported on a particular volume." >>>>> >>>>> Can we find a higher level API to use that does not have this limitation? >>>> >>>> I think I have found one, -[NSString stringByResolvingSymlinksInPath] from Foundation. >>> >>> This does the job but operates on NSString, not C strings. Perhaps the string conversion overhead is acceptable in code that will make a sys call anyway. >> >> Pierre clarified: "getattrlist is supported on all volume types. What may not be is all *attributes*. ATTR_CMN_FULLPATH is supported on all volumes because it is a VFS feature. it is the same as fcntl(fd, F_GETPATH) which is also supported on all volumes." >> >> So I think the proposed change is correct as-is. > > So the documentation is incorrect. And should be fixed. And also, I think calling -[NSString stringByResolvingSymlinksInPath] would be better than writing this code to call getattrlist. We need to file a bug on the getattrlist man page which is at least one source of wrong info. Darin, is that where you found the warning or is it replicated elsewhere? (In reply to Darin Adler from comment #15) > Comment on attachment 368496 [details] > WIP Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=368496&action=review > > >> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:254 > >> String stringByResolvingSymlinksInPath(const String& path) > > > > This function should probably move out of this file, now that it’s not used by sandbox extension code at all. > > Why is this code staying in SandboxExtensionCocoa.mm now that it’s not used > at all in sandbox extension code? I didn't want to involve moving this code to WTF (or wherever) in this performance-related patch. Why don't I file a separate bugzilla to do the cleanup of moving it to a more logical location (see Bug 197465). Comment on attachment 368496 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368496&action=review >>>> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:254 >>>> String stringByResolvingSymlinksInPath(const String& path) >>> >>> This function should probably move out of this file, now that it’s not used by sandbox extension code at all. >> >> Why is this code staying in SandboxExtensionCocoa.mm now that it’s not used at all in sandbox extension code? > > I didn't want to involve moving this code to WTF (or wherever) in this performance-related patch. Why don't I file a separate bugzilla to do the cleanup of moving it to a more logical location (see Bug 197465). Somewhere in WebKit/Platform/cocoa would do, doesn't have to be all the way to WTF if it's not used below the WebKit level. Comment on attachment 368496 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368496&action=review >>>> Source/WebKit/ChangeLog:16 >>>> + symlinks twice, since NSString's 'stringByStandardizingPath' method does this already. >>> >>> I think we can remove the stringByStandardizingPath call as well, since supposedly the sandbox extension call already resolves symlinks (needs testing with weird symlink setups, but this patch should probably tested that way regardless.) >> >> I'm not sure; we serialize the result of this operation across XPC and use that path for various operations. I guess we could just always deal in terms of the non-simplified file path, and know that the sandbox system is granting access to the actual resolved file node (or whatever the underlying representation is), but making that change seems a little scarier than just changing from one API to another. > > It's potentially a bigger perf win though, so as long as we are changing this code anyway and are able to test properly, might as well make the bigger improvement. That makes sense to me, and I think that splitting this into two separate patches, one that focuses on resolvePathForSandboxExtension, and another that focuses on other call sites, would be helpful (see below). >>>>>>> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:244 >>>>>>> + if (getattrlist(path.data(), &attrs, &attrBuf, sizeof(attrBuf), FSOPT_ATTR_CMN_EXTENDED) < 0) >>>>>> >>>>>> Found this in documentation: >>>>>> >>>>>> "Not all volumes support getattrlist(). The best way to test whether a volume supports this function is to simply call it and check the error result. getattrlist() will return ENOTSUP if it is not supported on a particular volume." >>>>>> >>>>>> Can we find a higher level API to use that does not have this limitation? >>>>> >>>>> I think I have found one, -[NSString stringByResolvingSymlinksInPath] from Foundation. >>>> >>>> This does the job but operates on NSString, not C strings. Perhaps the string conversion overhead is acceptable in code that will make a sys call anyway. >>> >>> Pierre clarified: "getattrlist is supported on all volume types. What may not be is all *attributes*. ATTR_CMN_FULLPATH is supported on all volumes because it is a VFS feature. it is the same as fcntl(fd, F_GETPATH) which is also supported on all volumes." >>> >>> So I think the proposed change is correct as-is. >> >> So the documentation is incorrect. And should be fixed. And also, I think calling -[NSString stringByResolvingSymlinksInPath] would be better than writing this code to call getattrlist. > > We need to file a bug on the getattrlist man page which is at least one source of wrong info. Darin, is that where you found the warning or is it replicated elsewhere? I found it on the man page. >>>>> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:254 >>>>> String stringByResolvingSymlinksInPath(const String& path) >>>> >>>> This function should probably move out of this file, now that it’s not used by sandbox extension code at all. >>> >>> Why is this code staying in SandboxExtensionCocoa.mm now that it’s not used at all in sandbox extension code? >> >> I didn't want to involve moving this code to WTF (or wherever) in this performance-related patch. Why don't I file a separate bugzilla to do the cleanup of moving it to a more logical location (see Bug 197465). > > Somewhere in WebKit/Platform/cocoa would do, doesn't have to be all the way to WTF if it's not used below the WebKit level. Agreed. But also, since the mission here was presumably to improve the performance of resolvePathForSandboxExtension I think we should land a first, simpler patch that just removes the call to resolveSymlinksInPath. If the performance of stringByResolvingSymlinksInPath is an issue at other call sites, I’d like to understand better which ones. And, since all other callers are calling stringByResolvingSymlinksInPath and not calling resolveSymlinksInPath directly, we should just change stringByResolvingSymlinksInPath to call -[NSString stringByResolvingSymlinksInPath] and not write our own getattrlist code unless there is a compelling reason to. That’s unrelated to speeding up resolvePathForSandboxExtension! Created attachment 368806 [details]
Patch
Comment on attachment 368806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368806&action=review > Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:230 > String stringByResolvingSymlinksInPath(const String& path) > { > - return String::fromUTF8(resolveSymlinksInPath(path.utf8())); > + return resolvePathForSandboxExtension(path); > } Is this right? Do we want to use stringByStandardizingPath in all the places where we call stringByResolvingSymlinksInPath? If so, then why does the name of the function exactly match a different NSString function? > Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:253 > - // FIXME: Do we need both resolveSymlinksInPath() and -stringByStandardizingPath? > CString fileSystemPath = FileSystem::fileSystemRepresentation([(NSString *)path stringByStandardizingPath]); > if (fileSystemPath.isNull()) { > LOG_ERROR("Could not create a valid file system representation for the string '%s' of length %lu", fileSystemPath.data(), fileSystemPath.length()); > return { }; > } > > - CString standardizedPath = resolveSymlinksInPath(fileSystemPath); > - return String::fromUTF8(standardizedPath); > + return String::fromUTF8(fileSystemPath); Why the round trip through FileSystem::fileSystemRepresentation and String::fromUTF8? What cases does that do something different for? Can we add test cases that cover this? Comment on attachment 368806 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368806&action=review >> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:230 >> } > > Is this right? Do we want to use stringByStandardizingPath in all the places where we call stringByResolvingSymlinksInPath? If so, then why does the name of the function exactly match a different NSString function? I've now read through the implementations of 'stringByStandardizingPath' and 'stringByResolvingSymlinksInPath', and I think for equivalent behavior to our old code, we want to replace both cases with 'stringByResolvingSymlinksInPath', and do away with 'stringByStandardizingPath'. 'stringByResolvingSymlinksInPath' does the following: 1. Expands tilde (if present) to yield a full absolute path. 2. Resolves the symlinks in the path to their full path. 3. Standardizes the path. 'stringByStandardizingPath' does: 1. Expands the tilde to give an absolute path. 2. Standardizes the path. Where 'Standardizes the path' is described in <https://developer.apple.com/documentation/foundation/nsstring/1407194-stringbystandardizingpath?language=objc>, but basically is about removing extraneous path components. >> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:253 >> + return String::fromUTF8(fileSystemPath); > > Why the round trip through FileSystem::fileSystemRepresentation and String::fromUTF8? What cases does that do something different for? Can we add test cases that cover this? 'fileSystemRepresentation' ensures that the string contains only valid POSIX filesystem characters, but I now see that we have that same guarantee from 'stringByStandardizingPath' (and 'stringByResolvingSymlinksInPath') as long as the original string is a file path. So the answer to the original question, "Do we need both resolveSymlinksInPath' and 'stringByStandardizingPath'?" is actually YES, except that we can just use 'stringByResolvingSymlinksInPath', which does both things (efficiently) in one method call. I'll switch to reflect that. Created attachment 368872 [details]
Patch
(In reply to Brent Fulgham from comment #22) > > So the answer to the original question, "Do we need both > resolveSymlinksInPath' and 'stringByStandardizingPath'?" is actually YES, > except that we can just use 'stringByResolvingSymlinksInPath', which does > both things (efficiently) in one method call. > Based on your findings, do we actually need the symlink resolution (or for that matter other standardization) at all for sandbox calls? Would be better to remove it if we are able, but if not, we should close our internal bug that suggests no symlink resolution is needed. (In reply to Maciej Stachowiak from comment #24) > (In reply to Brent Fulgham from comment #22) > > > > So the answer to the original question, "Do we need both > > resolveSymlinksInPath' and 'stringByStandardizingPath'?" is actually YES, > > except that we can just use 'stringByResolvingSymlinksInPath', which does > > both things (efficiently) in one method call. > > > > Based on your findings, do we actually need the symlink resolution (or for > that matter other standardization) at all for sandbox calls? Would be better > to remove it if we are able, but if not, we should close our internal bug > that suggests no symlink resolution is needed. I would like to do this in two steps. First, the simple change I am proposing in this patch that does not modify behavior (just uses a more efficient API). Then I would like to try the more aggressive change to completely remove the symlink resolution step. I’m not confident it is safe to do that, and would prefer to have a revision for this change that could be used to help troubleshoot problems that might be discovered later if the symlink logic is indeed important. Benchmarks indicate that the current patch is about a 3% perf improvement for Safari launch. Comment on attachment 368872 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368872&action=review r=me The suggestion to entirely remove the symlink resolution here, and to move the stringByResolvingSymlinksInPath() function to another file, should be considered as follow-up changes. > Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:247 > + String resolvedPath = [(NSString *)path stringByResolvingSymlinksInPath]; Optional suggestion: It might actually make sense to call the new stringByResolvingSymlinksInPath() function here, since it does the same NSString/String conversion. Created attachment 368961 [details]
Patch for landing
Comment on attachment 368872 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=368872&action=review >> Source/WebKit/Shared/Cocoa/SandboxExtensionCocoa.mm:247 >> + String resolvedPath = [(NSString *)path stringByResolvingSymlinksInPath]; > > Optional suggestion: It might actually make sense to call the new stringByResolvingSymlinksInPath() function here, since it does the same NSString/String conversion. Good idea! (In reply to Maciej Stachowiak from comment #27) > Comment on attachment 368872 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=368872&action=review > > r=me > > The suggestion to entirely remove the symlink resolution here, and to move > the stringByResolvingSymlinksInPath() function to another file, should be > considered as follow-up changes. Please see Bug 197465 and Bug 197568, which I hope to close soon. Comment on attachment 368961 [details] Patch for landing Clearing flags on attachment: 368961 Committed r244917: <https://trac.webkit.org/changeset/244917> All reviewed patches have been landed. Closing bug. This change has caused an API test failure due to unexpected logging:
TestWebKitAPI.WKWebView.InitializingWebViewWithEphemeralStorageDoesNotLog
Saw unexpected logging: 'ERROR: Could not create a valid file system representation for the string '' of length 0
'
/Volumes/Data/slave/highsierra-debug/build/Tools/TestWebKitAPI/Tests/WebKitCocoa/WKWebViewDoesNotLogDuringInitialization.mm:55
Failed
https://build.webkit.org/builders/Apple%20High%20Sierra%20Debug%20WK2%20%28Tests%29/builds/7664/steps/run-api-tests/logs/stdio
API tests were green on EWS before the last iteration of the patch (which didn't get results), so perhaps the diff between the two introduced the issue.
(In reply to Ryan Haddad from comment #33) > API tests were green on EWS before the last iteration of the patch (which > didn't get results), so perhaps the diff between the two introduced the > issue. Actually, this is only happening for debug queues, and EWS runs release. (In reply to Ryan Haddad from comment #34) > (In reply to Ryan Haddad from comment #33) > > API tests were green on EWS before the last iteration of the patch (which > > didn't get results), so perhaps the diff between the two introduced the > > issue. > Actually, this is only happening for debug queues, and EWS runs release. This looks like a believable result of my change. It's an additional logging message that wasn't happening before. Likely the original code bailed out earlier if it was passed an empty string, and the new code logs the empty string. I'll see if I can restore the original quieter behavior. Maybe we don’t need logging in this function at all. (In reply to Darin Adler from comment #36) > Maybe we don’t need logging in this function at all. I think you are right. I'm not sure what we would do with the log information if we were investigating some kind of failure, and it looks like the original resolveSymlinks code would silently return if the string resolved to an empty string. And the original code didn't log on an empty string; it only logged on a Null string. Let's fix the failure ASAP or else roll out the change. Is there a bug for the new failure? (In reply to Maciej Stachowiak from comment #38) > Let's fix the failure ASAP or else roll out the change. Is there a bug for > the new failure? I will fix it today, but my development machine is updating at the moment. I guess Ryan can roll it out if one or two hours of downtime will impact people. Reverted r244917 for reason: Caused TestWebKitAPI.WKWebView.InitializingWebViewWithEphemeralStorageDoesNotLog failure on debug bots. Committed r244964: <https://trac.webkit.org/changeset/244964> Created attachment 369156 [details]
Patch for landing
Comment on attachment 369156 [details] Patch for landing Clearing flags on attachment: 369156 Committed r244969: <https://trac.webkit.org/changeset/244969> All reviewed patches have been landed. Closing bug. |