Bug 197389 – Use more efficient path resolution logic in SandboxExtensions

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

[Brent Fulgham]

The code in SandboxExtensions 'resolveSymlinksInPath' is pretty inefficient, and tries to recreate (badly) stuff that is already provided by the operating system.

Let's stop doing that!

[Brent Fulgham]

<rdar://problem/50268491>

[Brent Fulgham]

Created attachment 368496 [details]
WIP Patch

[Brent Fulgham]

Comment on attachment 368496 [details]
WIP Patch

Initial patch to see how the various bots like it.

[Darin Adler]

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?

[Darin Adler]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Maciej Stachowiak]

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.

[Maciej Stachowiak]

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

[Brent Fulgham]

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! :-)

[Brent Fulgham]

Created attachment 368624 [details]
Patch

[Darin Adler]

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.

[Darin Adler]

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?

[Maciej Stachowiak]

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?

[Brent Fulgham]

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

[Maciej Stachowiak]

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.

[Darin Adler]

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!

[Brent Fulgham]

Created attachment 368806 [details]
Patch

[Darin Adler]

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?

[Brent Fulgham]

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.

[Brent Fulgham]

Created attachment 368872 [details]
Patch

[Maciej Stachowiak]

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

[Brent Fulgham]

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

[Brent Fulgham]

Benchmarks indicate that the current patch is about a 3% perf improvement for Safari launch.

[Maciej Stachowiak]

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.

[Brent Fulgham]

Created attachment 368961 [details]
Patch for landing

[Brent Fulgham]

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!

[Brent Fulgham]

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

[WebKit Commit Bot]

Comment on attachment 368961 [details]
Patch for landing

Clearing flags on attachment: 368961

Committed r244917: <https://trac.webkit.org/changeset/244917>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

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.

[Ryan Haddad]

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

[Brent Fulgham]

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

[Darin Adler]

Maybe we don’t need logging in this function at all.

[Brent Fulgham]

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

[Maciej Stachowiak]

Let's fix the failure ASAP or else roll out the change. Is there a bug for the new failure?

[Brent Fulgham]

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

[Ryan Haddad]

Reverted r244917 for reason:

Caused TestWebKitAPI.WKWebView.InitializingWebViewWithEphemeralStorageDoesNotLog failure on debug bots.

Committed r244964: <https://trac.webkit.org/changeset/244964>

[Brent Fulgham]

Created attachment 369156 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 369156 [details]
Patch for landing

Clearing flags on attachment: 369156

Committed r244969: <https://trac.webkit.org/changeset/244969>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.