[webkit-reviews] review granted: [Bug 207972] Make support for bytecode caching more robust against file corruption. : [Attachment 391377] proposed patch.

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <ysuzuki at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 207972: Make support for bytecode caching more robust against file
corruption.
https://bugs.webkit.org/show_bug.cgi?id=207972

Attachment 391377: proposed patch.

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




--- Comment #9 from Yusuke Suzuki <ysuzuki at apple.com> ---
Comment on attachment 391377
  --> https://bugs.webkit.org/attachment.cgi?id=391377
proposed patch.

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

r=me with sync fix.

> Source/JavaScriptCore/ChangeLog:28
> +	   Manually tested with the following scenarios and ensuring that the
client recovers
> +	   with no crashes:

Can you also ensure that we are successfully using bytecode cache if it is
correct?

> Source/JavaScriptCore/API/JSScript.mm:184
> +    SHA1::Digest fileHash;
> +    memcpy(&fileHash, fileData + fileDataSize, sizeof(SHA1::Digest));

Maybe, doing this after calculating the hash is better (at that time, page
should be populated), but it is a small nit.

> Source/JavaScriptCore/API/JSScript.mm:312
> +	   close(fd);

I think writing makeScopeExit twice is cleaner and non-error-prone.

auto closeFD = makeScopeExit([&] {
    close(fd);
});

int tempFD = ...
if (tempFD == -1) {
    ...
}

auto closeTempFD = makeScopeExit([&] {
    close(tempFD);
});

> Source/JavaScriptCore/API/JSScript.mm:346
> +    rename(tempFileName, cacheFileName);

Before renaming, let's do fsync to ensure the content is in disk.

> Source/JavaScriptCore/API/JSScript.mm:347
>      return YES;

To make this actually sync, we need to emit fsync onto the directory of this to
make "renaming" in directory-entry sync in the disk. But since this is cache,
we don't need to ensure it, I think.
If directory misses the rename operation, it is still fine.