Bug 197389 - Use more efficient path resolution logic in SandboxExtensions
Summary: Use more efficient path resolution logic in SandboxExtensions
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Brent Fulgham
URL:
Keywords: InRadar
Depends on:
Blocks: 197465 197568
  Show dependency treegraph
 
Reported: 2019-04-29 14:48 PDT by Brent Fulgham
Modified: 2019-05-06 13:14 PDT (History)
9 users (show)

See Also:


Attachments
WIP Patch (5.12 KB, patch)
2019-04-29 14:54 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews105 for mac-highsierra-wk2 (2.90 MB, application/zip)
2019-04-29 16:24 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews202 for win-future (12.97 MB, application/zip)
2019-04-29 17:04 PDT, Build Bot
no flags Details
Patch (5.57 KB, patch)
2019-04-30 16:22 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (5.09 KB, patch)
2019-05-02 12:33 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch (5.49 KB, patch)
2019-05-02 19:44 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch for landing (5.43 KB, patch)
2019-05-03 12:04 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff
Patch for landing (5.43 KB, patch)
2019-05-06 12:35 PDT, Brent Fulgham
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Brent Fulgham 2019-04-29 14:48:19 PDT
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!
Comment 1 Brent Fulgham 2019-04-29 14:48:40 PDT
<rdar://problem/50268491>
Comment 2 Brent Fulgham 2019-04-29 14:54:22 PDT
Created attachment 368496 [details]
WIP Patch
Comment 3 Brent Fulgham 2019-04-29 14:54:47 PDT
Comment on attachment 368496 [details]
WIP Patch

Initial patch to see how the various bots like it.
Comment 4 Darin Adler 2019-04-29 15:56:42 PDT
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 5 Darin Adler 2019-04-29 16:01:19 PDT
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 6 Build Bot 2019-04-29 16:24:12 PDT
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
Comment 7 Build Bot 2019-04-29 16:24:13 PDT
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 8 Build Bot 2019-04-29 17:04:16 PDT
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
Comment 9 Build Bot 2019-04-29 17:04:28 PDT
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 10 Maciej Stachowiak 2019-04-29 22:06:27 PDT
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 11 Maciej Stachowiak 2019-04-29 22:12:33 PDT
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 12 Brent Fulgham 2019-04-30 09:26:04 PDT
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! :-)
Comment 13 Brent Fulgham 2019-04-30 16:22:26 PDT
Created attachment 368624 [details]
Patch
Comment 14 Darin Adler 2019-04-30 18:20:05 PDT
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 15 Darin Adler 2019-04-30 18:20:55 PDT
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 16 Maciej Stachowiak 2019-04-30 18:49:40 PDT
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?
Comment 17 Brent Fulgham 2019-05-01 10:07:16 PDT
(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 18 Maciej Stachowiak 2019-05-01 10:14:21 PDT
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 19 Darin Adler 2019-05-01 13:01:39 PDT
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!
Comment 20 Brent Fulgham 2019-05-02 12:33:21 PDT
Created attachment 368806 [details]
Patch
Comment 21 Darin Adler 2019-05-02 14:39:27 PDT
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 22 Brent Fulgham 2019-05-02 16:54:29 PDT
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.
Comment 23 Brent Fulgham 2019-05-02 19:44:45 PDT
Created attachment 368872 [details]
Patch
Comment 24 Maciej Stachowiak 2019-05-02 21:52:00 PDT
(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.
Comment 25 Brent Fulgham 2019-05-02 23:03:02 PDT
(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.
Comment 26 Brent Fulgham 2019-05-03 10:26:50 PDT
Benchmarks indicate that the current patch is about a 3% perf improvement for Safari launch.
Comment 27 Maciej Stachowiak 2019-05-03 11:28:34 PDT
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.
Comment 28 Brent Fulgham 2019-05-03 12:04:19 PDT
Created attachment 368961 [details]
Patch for landing
Comment 29 Brent Fulgham 2019-05-03 12:05:06 PDT
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!
Comment 30 Brent Fulgham 2019-05-03 12:05:54 PDT
(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 31 WebKit Commit Bot 2019-05-03 12:42:58 PDT
Comment on attachment 368961 [details]
Patch for landing

Clearing flags on attachment: 368961

Committed r244917: <https://trac.webkit.org/changeset/244917>
Comment 32 WebKit Commit Bot 2019-05-03 12:43:00 PDT
All reviewed patches have been landed.  Closing bug.
Comment 33 Ryan Haddad 2019-05-06 08:41:43 PDT
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.
Comment 34 Ryan Haddad 2019-05-06 08:43:36 PDT
(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.
Comment 35 Brent Fulgham 2019-05-06 09:19:40 PDT
(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.
Comment 36 Darin Adler 2019-05-06 09:25:08 PDT
Maybe we don’t need logging in this function at all.
Comment 37 Brent Fulgham 2019-05-06 09:33:10 PDT
(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.
Comment 38 Maciej Stachowiak 2019-05-06 09:52:29 PDT
Let's fix the failure ASAP or else roll out the change. Is there a bug for the new failure?
Comment 39 Brent Fulgham 2019-05-06 10:21:05 PDT
(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.
Comment 40 Ryan Haddad 2019-05-06 10:23:21 PDT
Reverted r244917 for reason:

Caused TestWebKitAPI.WKWebView.InitializingWebViewWithEphemeralStorageDoesNotLog failure on debug bots.

Committed r244964: <https://trac.webkit.org/changeset/244964>
Comment 41 Brent Fulgham 2019-05-06 12:35:16 PDT
Created attachment 369156 [details]
Patch for landing
Comment 42 WebKit Commit Bot 2019-05-06 13:14:16 PDT
Comment on attachment 369156 [details]
Patch for landing

Clearing flags on attachment: 369156

Committed r244969: <https://trac.webkit.org/changeset/244969>
Comment 43 WebKit Commit Bot 2019-05-06 13:14:18 PDT
All reviewed patches have been landed.  Closing bug.