[webkit-reviews] review denied: [Bug 184991] We should cache the compiled sandbox profile in a data vault : [Attachment 344130] Patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Ben Richards
<benton_richards at apple.com>'s request for review:
Bug 184991: We should cache the compiled sandbox profile in a data vault
https://bugs.webkit.org/show_bug.cgi?id=184991

Attachment 344130: Patch

https://bugs.webkit.org/attachment.cgi?id=344130&action=review




--- Comment #96 from Daniel Bates <dbates at webkit.org> ---
Comment on attachment 344130
  --> https://bugs.webkit.org/attachment.cgi?id=344130
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=344130&action=review

I have not finished reviewing this patch. I want to see another iteration.

> Source/WebKit/Shared/ChildProcess.h:49
> +	   WebContentType,

The suffix “Type” is unnecessary on each enumerator given that these are scoped
to ProcessType. Please remove the suffix.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:57
>  #include <HIServices/ProcessesPriv.h>

I know you did not write this code. We should change this line to use #import.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:58
> +#endif // USE(APPLE_INTERNAL_SDK)

Please remove the inline comment as the code block it is annotating is small
and can fit in a single screen. Annotating the end of a macro is most
beneficial when the code block spans more than one screen.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:102
> +    const SandboxInitializationParameters &initializationParameters;

The & should be in the left since this variables has a C++ data type. You also
put the & on the wrong side in the constructor signature below.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:104
> +    SandboxInfo(const SandboxInitializationParameters &parameters)

Do we usually put functions of a struct at the bottom? I was under the
impression that we use the same form as for classes and put functions at the
top and variables at the bottom.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:105
> +	   : initializationParameters(parameters)

Can we use universal initialization syntax?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:110
> +static constexpr uint32_t CachedSandboxVersionNumber = 0;

Please remove the “static”. Constant expressions have internal linkage.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:167
> +    FileSystem::PlatformFileHandle handle = openFile(path,
FileSystem::FileOpenMode::Read);

I suggest that we use the convenience class FileHandle,
<https://trac.webkit.org/browser/webkit/trunk/Source/WebCore/platform/FileHandl
e.h#L36>, instead of FileSystem to get a handle to a file. The former wraps a
platform file handle and will automatically close the file on destruction. Then
we can remove closeFileOnExit below.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:177
> +    if (!FileSystem::getFileSize(handle, fileSize))

Does this provide a measurable performance improvement? If not, I suggest we
remove it and use the file reading logic below handle the case of an empty
file.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:183
> +    int bytesRead = FileSystem::readFromFile(handle, contents.data(),
safeCast<size_t>(fileSize));

Is it necessary to compute the file size in advance to read the entire file?
Can we read fixed size chunks until FileHandle::read() told us there are no
more bytes? The benefit of this approach is that we avoid an unnecessary
stat’ing of the FileSystem as well as simplify the code in this function.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:213
> +    for (size_t i = 0; i < initializationParameters.count(); ++i) {

I take it we cannot use a C++11 ranged-based for-loop to iterate over the
parameters. We should cache count() in a local to avoid re-computing it on each
iteration.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:217
> +	   if (name.isNull() || value.isNull())

How can we ever have a null name or null value? It seems like this would
represent a programming mistake and hence we should use an assertion to catch
this assuming it can even happen based on how the code is structured.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:235
> +	   return std::nullopt;

This line does not conform to the WebKit Code Style Guidelines. A return
statement should not be part of any else clause.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:241
> +	   header.append(piece.data(), piece.length());

Although unlikely to cause an issue on most platforms, this code assumes that
sizeof(uint8_t) == sizeof(char). One way to remove this assumption is to
standardize on uint8_t for the headerPieces.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:244
> +    header.append(bitwise_cast<uint8_t *>(profileContents.data()),
profileContents.size());

Although unlikely to cause an issue on most platforms, this code assumes that
sizeof(uint8_t) == sizeof(char). One way to remove this assumption is to read
the file directly into a uint8_t buffer.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:252
> +    char darwinUserCacheDir[PATH_MAX + 1];

As pointed out before, is the “+ 1” needed? If POSIX? PATH_MAX includes the
null terminator then we should not add “+ 1” here.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:254
> +	   char temp[PATH_MAX + 1];

Ditto

> Source/WebKit/Shared/mac/ChildProcessMac.mm:255
> +	   if (!confstr(_CS_DARWIN_USER_CACHE_DIR, temp, sizeof(temp))) {

We are underutilizing the return value of confstr(). It returns the size of the
value written into the buffer. If this return value > sizeof(temp) then we know
it truncated the value and should handle this case appropriately. Additionally
we can use the knowledge of the size of the buffer to invoke the String()
constructor that takes a buffer and length and avoid having String call
strlen().

> Source/WebKit/Shared/mac/ChildProcessMac.mm:263
> +    return darwinUserCacheDir;

This is suboptimal because String will need to compute the length of the
buffer. The confstr() API returns the size of the buffer. We should pass this
info when constructing the string.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:268
> +    String directory = sandboxParentDirectory();

We should use StringBuilder to build this string efficiently and avoid
unnecessary malloc()s and computing the length of string literals.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:285
> +    // Add .OpenSource suffix so that non-internal builds don't try to
access a data vault used by system Safari

Nit: Missing a period at the end of this sentence.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:294
> +    String sandboxFile = info.directoryPath;

We should use StringBuilder to efficiently build the string in this function.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:308
> +constexpr size_t compileTimeStrlen(const char *str)

This is unnecessary. Please use WTF_ARRAY_SIZE() and remove this function.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:315
> +    constexpr char alphaNum[] =
"ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789";

Maybe a better number for this would be alphaNumericScanSet?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:316
> +    constexpr size_t alphaNumRange = compileTimeStrlen(alphaNum);

The word “range” is typically used to represent a pair of numbers to describe a
section of content. We typically use “length” when talking about the. Inner of
characters in a string.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:318
> +    

I do not see the need for this empty line as I do not feel it improves the
readability of this code. Please remove.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:327
> +    // First we need to ensure that at least the parent directory exists

This comment saids what the code does. Either explain why or remove this
comment.