Bug 238726 – [Unix] Add UnixFileDescriptor, use it in IPC::Semaphore

• Component: New Bugs
• Status: RESOLVED FIXED

[Zan Dobersek]

[Unix] Add UnixFileDescriptor, use it in IPC::Semaphore

[Zan Dobersek]

Created attachment 456544 [details]
Patch

[Adrian Perez]

Comment on attachment 456544 [details]
Patch

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

> Source/WTF/wtf/unix/UnixFileDescriptor.h:51
> +    UnixFileDescriptor& operator=(const UnixFileDescriptor&) = delete;

You could replace these two with WTF_MAKE_NONCOPYABLE(UnixFileDescriptor)

> Source/WTF/wtf/unix/UnixFileDescriptor.h:56
> +        o.value = -1;

These two lines could be replaced with:

  value = std::exchange(o.value, -1);

> Source/WebKit/Platform/IPC/unix/IPCSemaphoreUnix.cpp:129
> +    encoder << m_fd.duplicate();

I think this is duplicating the UnixFileDescriptor twice unneedlessly,
because the ArgumentCoder<…>::encode() function below will take another
duplicate itself.

> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:3298
> +    auto duplicate = fd.duplicate();

…here is where the second duplicate is made during encoding.
I think this one here should be the only one needed.

[Zan Dobersek]

Comment on attachment 456544 [details]
Patch

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

>> Source/WTF/wtf/unix/UnixFileDescriptor.h:51
>> +    UnixFileDescriptor& operator=(const UnixFileDescriptor&) = delete;
> 
> You could replace these two with WTF_MAKE_NONCOPYABLE(UnixFileDescriptor)

They are manually deleted just like the move constructor and assignment operators are manually defined. No need to reach out for additional macros.

>> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:3298
>> +    auto duplicate = fd.duplicate();
> 
> …here is where the second duplicate is made during encoding.
> I think this one here should be the only one needed.

No, cause the `m_fd.duplicate()` in Semaphore::encode() returns an rvalue that's then passed to ArgumentCoder<>::encode(Encoder&, WTF::UnixFileDescriptor&&), which handles the object like the temporary that it is.

[Carlos Garcia Campos]

Comment on attachment 456544 [details]
Patch

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

I like the idea, but since the destructor is closing the fd, it's somehow like a smart pointer class, so I would follow the same patterns we are already familiar with. I would make it a class instead of a struct with get method

> Source/WTF/wtf/unix/UnixFileDescriptor.h:35
> +struct UnixFileDescriptor {

WTF_MAKE_NONCOPYABLE

> Source/WTF/wtf/unix/UnixFileDescriptor.h:44
> +    enum AdoptionTag { Adopt };
> +    UnixFileDescriptor(int fd, AdoptionTag)
> +        : value(fd)
> +    { }
> +
> +    enum DuplicationTag { Duplicate };
> +    UnixFileDescriptor(int fd, DuplicationTag)

Why not using a single enum and the same constructor that adopts or duplicates depending on the enum value. We could also make them private and add adoptFileDescriptor/duplicateFileDescritor.

>> Source/WTF/wtf/unix/UnixFileDescriptor.h:56
>> +        o.value = -1;
> 
> These two lines could be replaced with:
> 
>   value = std::exchange(o.value, -1);

that's release()

value = o.release();

> Source/WTF/wtf/unix/UnixFileDescriptor.h:82
> +    int value { -1 };

Being this a public member of the struct you can do foo.value = fd and mess it up.

> Source/WebKit/Platform/IPC/IPCSemaphore.h:84
> +    WTF::UnixFileDescriptor m_fd { };

We don't need the initialization now.

>> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:3298
>> +    auto duplicate = fd.duplicate();
> 
> …here is where the second duplicate is made during encoding.
> I think this one here should be the only one needed.

I think it would be easier of IPC::Attachment used UnixFileDescriptor instead of int.

[Zan Dobersek]

Comment on attachment 456544 [details]
Patch

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

>> Source/WTF/wtf/unix/UnixFileDescriptor.h:44
>> +    UnixFileDescriptor(int fd, DuplicationTag)
> 
> Why not using a single enum and the same constructor that adopts or duplicates depending on the enum value. We could also make them private and add adoptFileDescriptor/duplicateFileDescritor.

Because that means generated branches from testing the enum value.

>>>> Source/WebKit/Shared/WebCoreArgumentCoders.cpp:3298
>>>> +    auto duplicate = fd.duplicate();
>>> 
>>> …here is where the second duplicate is made during encoding.
>>> I think this one here should be the only one needed.
>> 
>> No, cause the `m_fd.duplicate()` in Semaphore::encode() returns an rvalue that's then passed to ArgumentCoder<>::encode(Encoder&, WTF::UnixFileDescriptor&&), which handles the object like the temporary that it is.
> 
> I think it would be easier of IPC::Attachment used UnixFileDescriptor instead of int.

In a different patch, cause it will require additional changes in ConnectionUnix.cpp.

[Zan Dobersek]

Created attachment 456681 [details]
Patch

[Carlos Garcia Campos]

Comment on attachment 456681 [details]
Patch

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

> Source/WTF/wtf/unix/UnixFileDescriptor.h:51
> +    enum DuplicationTag { Duplicate };
> +    UnixFileDescriptor(int fd, DuplicationTag)
> +    {
> +        if (fd >= 0)
> +            m_value = dupCloseOnExec(fd);
> +    }

I think we can keep this private for now, since it's always used from ::duplicate().

> Source/WTF/wtf/unix/UnixFileDescriptor.h:88
> +

using WTF::UnixFileDescriptor

> Source/WebKit/Platform/IPC/unix/IPCSemaphoreUnix.cpp:44
> +    m_fd = { eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK | EFD_SEMAPHORE), WTF::UnixFileDescriptor::Adopt };

I still prefer m_fd = adoptFileDescriptor(eventfd(0, EFD_CLOEXEC | EFD_NONBLOCK | EFD_SEMAPHORE)); but no strong opinion if you really want to expose WTF::UnixFileDescriptor::Adopt and WTF::UnixFileDescriptor::Duplicate

> Source/WebKit/Shared/WebCoreArgumentCoders.h:854
> +#if USE(UNIX_DOMAIN_SOCKETS)
> +
> +template<> struct ArgumentCoder<WTF::UnixFileDescriptor> {
> +    static void encode(Encoder&, const WTF::UnixFileDescriptor&);
> +    static void encode(Encoder&, WTF::UnixFileDescriptor&&);
> +    static std::optional<WTF::UnixFileDescriptor> decode(Decoder&);
> +};
> +
> +#endif

This doesn't really belong here, since UnixFileDescriptor is not a WebCore class. Why don't you add the encode/decode methods to UnixFileDescriptor class?

[Zan Dobersek]

Comment on attachment 456681 [details]
Patch

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

>> Source/WTF/wtf/unix/UnixFileDescriptor.h:51
>> +    }
> 
> I think we can keep this private for now, since it's always used from ::duplicate().

I need it in the next patches.

>> Source/WebKit/Shared/WebCoreArgumentCoders.h:854
>> +#endif
> 
> This doesn't really belong here, since UnixFileDescriptor is not a WebCore class. Why don't you add the encode/decode methods to UnixFileDescriptor class?

I don't have IPC::Attachment in WTF.

[Zan Dobersek]

Comment on attachment 456681 [details]
Patch

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

>>> Source/WebKit/Shared/WebCoreArgumentCoders.h:854
>>> +#endif
>> 
>> This doesn't really belong here, since UnixFileDescriptor is not a WebCore class. Why don't you add the encode/decode methods to UnixFileDescriptor class?
> 
> I don't have IPC::Attachment in WTF.

There's also WTF::MachSendRight in this file, even when the definitions are kept somewhere else.

[Zan Dobersek]

Created attachment 456688 [details]
Patch for landing

[Zan Dobersek (Reviews)]

Comment on attachment 456688 [details]
Patch for landing

Clearing flags on attachment: 456688

Committed r292389 (249254@trunk): <https://commits.webkit.org/249254@trunk>

[Zan Dobersek (Reviews)]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/91286018>