Bug 144046

Created attachment 251329 [details] Patch

Comment on attachment 251329 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=251329&action=review Looks OK. > Source/WebKit2/Platform/IPC/Attachment.h:59 > + Attachment(const Attachment&) = default; > + Attachment& operator=(Attachment&) = default; I think you want = delete here, not = default. It doesn’t seem OK to just copy a file descriptor!

(In reply to comment #2) > Comment on attachment 251329 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=251329&action=review > > Looks OK. Thanks for the review. > > Source/WebKit2/Platform/IPC/Attachment.h:59 > > + Attachment(const Attachment&) = default; > > + Attachment& operator=(Attachment&) = default; > > I think you want = delete here, not = default. It doesn’t seem OK to just > copy a file descriptor! Yes, I first tried to make the class non copyable, and release the fd in the destructor to get rid of the dispose() method and the explicit attachment dispose made by the ArgumentEncoder, ArgumentDecoder and ConnectionUnix. But it was not possible, or I didn't find the way. Encoding/decoding attachments is adding them to the encoder/decoder class that take the ownership of the file descriptors until they are passed to the platform specific send/receive IPC methods. The thing is that Attachment::encode() is const and does encoder.addAttachment(*this); Even if we change ArgumentEncoder::addAttachment to receive an Attachment&& we can't WTF::move(*this) in the const encode method. I'm not a C++ expert, so maybe there's a way. In the end I forgot about that part and left the explicit dispose, but avoiding the manual fd = -1 in several places and renaming the confusing adoptFrom, releaseTo methods.

Committed r183176: <http://trac.webkit.org/changeset/183176>

(In reply to comment #3) > (In reply to comment #2) > > Comment on attachment 251329 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=251329&action=review > > > > Looks OK. > > Thanks for the review. > > > > Source/WebKit2/Platform/IPC/Attachment.h:59 > > > + Attachment(const Attachment&) = default; > > > + Attachment& operator=(Attachment&) = default; > > > > I think you want = delete here, not = default. It doesn’t seem OK to just > > copy a file descriptor! > > Yes, I first tried to make the class non copyable, and release the fd in the > destructor to get rid of the dispose() method and the explicit attachment > dispose made by the ArgumentEncoder, ArgumentDecoder and ConnectionUnix. But > it was not possible, or I didn't find the way. Encoding/decoding attachments > is adding them to the encoder/decoder class that take the ownership of the > file descriptors until they are passed to the platform specific send/receive > IPC methods. The thing is that Attachment::encode() is const and does > encoder.addAttachment(*this); Even if we change > ArgumentEncoder::addAttachment to receive an Attachment&& we can't > WTF::move(*this) in the const encode method. I'm not a C++ expert, so maybe > there's a way. In the end I forgot about that part and left the explicit > dispose, but avoiding the manual fd = -1 in several places and renaming the > confusing adoptFrom, releaseTo methods. Zan suggested to make the copy constructor and assignment private and use it only in ::encode() doing encoder.addAttachment(Attachment(*this)); That way we can ensure that any public assignment is a move. We also need to make the file descriptor member mutable and explicitly set to -1 in ::encode() since the ownership is transfered to the encoder. It's tricky, but it works. I'll submit a new bug.

See https://bugs.webkit.org/show_bug.cgi?id=144096