Bug 238726

Created attachment 456544 [details] Patch

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.

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.

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.

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.

Created attachment 456681 [details] Patch

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?

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.

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.

Created attachment 456688 [details] Patch for landing

Comment on attachment 456688 [details] Patch for landing Clearing flags on attachment: 456688 Committed r292389 (249254@trunk): <https://commits.webkit.org/249254@trunk>

All reviewed patches have been landed. Closing bug.

<rdar://problem/91286018>