[webkit-reviews] review granted: [Bug 239513] [macOS] The function getpwnam can sometimes fail : [Attachment 458276] Patch

[bugzilla-daemon at webkit.org]

Darin Adler <darin at apple.com> has granted Per Arne Vollan <pvollan at apple.com>'s
request for review:
Bug 239513: [macOS] The function getpwnam can sometimes fail
https://bugs.webkit.org/show_bug.cgi?id=239513

Attachment 458276: Patch

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




--- Comment #9 from Darin Adler <darin at apple.com> ---
Comment on attachment 458276
  --> https://bugs.webkit.org/attachment.cgi?id=458276
Patch

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

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:824
> +    sandboxExtension->consume();

Why isn’t there code to do this the first time? Or if there is, why doesn’t
this share code with it. Seems we also need to do this after process startup
time even if the Open Directory cache was not invalidated.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:826
> +    char buffer[4096];

Might need a comment saying why 4096 is the right number to use here.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:827
> +    int bufferSize = sizeof(buffer);

Not sure we need to put this constant into an int. Just call sizeof(buffer)
below.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:829
> +    struct passwd pwd;
> +    struct passwd* result = 0;

This is C++, so I think you can write:

    passwd pwd;
    passwd* result = nullptr;

Don’t think we need struct and we don’t need to use 0.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:830
> +    if (getpwuid_r(getuid(), &pwd, buffer, bufferSize, &result) || !result)

It’s incredibly non-obvious that the whole point of this call is to refill some
internal caches with accurate data as a side effect, not use the result of this
function at all.

I think there needs to be a comment explaining that this is an indirect way to
revalidate caches so that calls done to related functions elsewhere in other
frameworks get the correct result.

In my opinion it’s a bit fragile to rely on this property of the functions, not
documented anywhere I can see, but it may well be necessary to do things this
way. Needs a comment.

> Source/WebKit/Shared/mac/AuxiliaryProcessMac.mm:831
> +	   WTFLogAlways("%s: Couldn't find home directory\n", getprogname());

Do we really need the "\n" here? Doesn’t WTFLog add it?

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:712
> +    m_openDirectoryNotifyTokens.reserveCapacity(WTF_ARRAY_LENGTH(messages));

Can use std::size instead of WTF_ARRAY_LENGTH. We should probably get rid of
WTF_ARRAY_LENGTH.

If this is always done exactly once we can use reserveInitialCapacity.

> Source/WebKit/UIProcess/Cocoa/WebProcessPoolCocoa.mm:713
> +    for (unsigned i = 0; i < WTF_ARRAY_LENGTH(messages); i++) {

Range-based for lop is bette for this:

    for (auto* message : messages) {