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

[bugzilla-daemon at webkit.org]

Ryosuke Niwa <rniwa at webkit.org> has granted 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 345311: Patch

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




--- Comment #117 from Ryosuke Niwa <rniwa at webkit.org> ---
Comment on attachment 345311
  --> https://bugs.webkit.org/attachment.cgi?id=345311
Patch

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

> Source/WebCore/platform/FileSystem.cpp:374
> +bool deleteNonEmptyDirectory(const String& path)
> +{

Alex says we have a method to do this in NSFileManager. Let's use that and put
this in FileSystemCocoa.mm

> Source/WebKit/Shared/mac/ChildProcessMac.mm:311
> +    auto crypto =
PAL::CryptoDigest::create(PAL::CryptoDigest::Algorithm::SHA_224);

Use SHA_256?

> Source/WebKit/Shared/mac/ChildProcessMac.mm:315
> +    String readableHash = base64Encode(hash, WTF::Base64URLPolicy);
> +    readableHash.replace('/', '_');

I think we wanna use base64URLEncode so that we don't have to do manual
replacement.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:394
> +    if (file.write(cacheFile.data(), cacheFile.size()) !=
safeCast<int>(cacheFile.size()))

Just return file.write(~) == safeCast... here without a if statement.

> Source/WebKit/Shared/mac/ChildProcessMac.mm:451
> +    if (auto contents = fileContents(info.filePath, true,
FileSystem::FileLockMode::Shared))
> +	   cachedSandboxContents = WTFMove(*contents);

Better to early return when contents is falsey as in:
auto contents =~
if (!contents || !contents->isEmpty())
    return false;

> Source/WebKit/Shared/mac/ChildProcessMac.mm:480
> +	   // Header and cached header do not have the same contents so sandbox
parameters must have changed and we need to recompile.

I don't think we need this comment. The code makes it clear what we're
checking.