Bug 60621

Created attachment 93109 [details] Patch Instead of checking if the handle is null, we can simply check if the size has changed in the plugin process before creating a new backing store.

Comment on attachment 93109 [details] Patch Looks sane to me, but I'd be more comfortable if the original authors of this code took a look as well.

Comment on attachment 93109 [details] Patch This patch is incorrect. Isn't it possible to make SharedMemory::Handle::encode just encode whether the handle is null or not?

(In reply to comment #3) > (From update of attachment 93109 [details]) > This patch is incorrect. Isn't it possible to make SharedMemory::Handle::encode just encode whether the handle is null or not? It does it, when file descriptor is -1 the handle is null, but then it's impossible to send the message because sendmsg always fails.

(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 93109 [details] [details]) > > This patch is incorrect. Isn't it possible to make SharedMemory::Handle::encode just encode whether the handle is null or not? > > It does it, when file descriptor is -1 the handle is null, but then it's impossible to send the message because sendmsg always fails. But in that case, does the file descriptor even need to be passed to sendmsg?

(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (From update of attachment 93109 [details] [details] [details]) > > > This patch is incorrect. Isn't it possible to make SharedMemory::Handle::encode just encode whether the handle is null or not? > > > > It does it, when file descriptor is -1 the handle is null, but then it's impossible to send the message because sendmsg always fails. > > But in that case, does the file descriptor even need to be passed to sendmsg? well, it's part of the GeometryDidChange message, the handle is encoded as an attachment. Attachments are sent using control messages with cmsg_level = SOL_SOCKET, cmsg_type = SCM_RIGHTS which makes sendmsg check the file descriptors passed.

Your are right, we should be able to send null handles. I'm already working on a patch for ConnectionUnix to allow it.

Created attachment 93295 [details] Patch

Comment on attachment 93295 [details] Patch Attachment 93295 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8684816

(In reply to comment #9) > (From update of attachment 93295 [details]) > Attachment 93295 [details] did not pass qt-ews (qt): > Output: http://queues.webkit.org/results/8684816 Qt build failed because this patch depends on patch attached to bug #60629

Comment on attachment 93295 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=93295&action=review Look sane, but I'd like to at least get a nod from the personal who originally wrote this code. > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:104 > + void setType(Attachment::Type type) { m_type = type; } > + > + Attachment::Type getType() { return m_type; } No need for this extra newline. > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:110 > + ASSERT(m_type == Attachment::MappedMemoryType); > + > + m_size = size; Ditto. > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:117 > + ASSERT(m_type == Attachment::MappedMemoryType); > + > + return m_size; Ditto. > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:124 > + void setNull() { m_isNull = true; } > + > + bool isNull() { return m_isNull; } > + Ditto. > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:247 > + size_t attachmentFDCount = 0; Please call this something less similar to attachmentCount like attachmentFileDescriptorCount. I confused the two variables on my first read through. > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:436 > + int* fdptr = 0; Should be fdPtr. > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:442 > + cmsg = CMSG_FIRSTHDR(&message); It seems that you only use this insidde this block, so you should declare it here instead of above. It would be better to rename this to something more readable like firstCMSGHeader. > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:457 > + // Fall trhough. Please expand this. Why should this fall through?

(In reply to comment #11) > (From update of attachment 93295 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=93295&action=review > > Look sane, but I'd like to at least get a nod from the personal who originally wrote this code. Thanks for reviewing. > > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:104 > > + void setType(Attachment::Type type) { m_type = type; } > > + > > + Attachment::Type getType() { return m_type; } > > No need for this extra newline. I added these lines for consistency with MessageInfo class, but I can remove them. > > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:110 > > + ASSERT(m_type == Attachment::MappedMemoryType); > > + > > + m_size = size; > > Ditto. > > > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:117 > > + ASSERT(m_type == Attachment::MappedMemoryType); > > + > > + return m_size; > > Ditto. > > > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:124 > > + void setNull() { m_isNull = true; } > > + > > + bool isNull() { return m_isNull; } > > + > > Ditto. > > > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:247 > > + size_t attachmentFDCount = 0; > > Please call this something less similar to attachmentCount like attachmentFileDescriptorCount. I confused the two variables on my first read through. Ok. > > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:436 > > + int* fdptr = 0; > > Should be fdPtr. I haven't added that variable, it's in the current code, I'll rename it anyway. > > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:442 > > + cmsg = CMSG_FIRSTHDR(&message); > > It seems that you only use this insidde this block, so you should declare it here instead of above. Ok. > It would be better to rename this to something more readable like firstCMSGHeader. This is also current code, I'll rename it too. > > Source/WebKit2/Platform/CoreIPC/unix/ConnectionUnix.cpp:457 > > + // Fall trhough. > > Please expand this. Why should this fall through? Bacause both mapped mem and socket attachments have a file descriptor.

Created attachment 94062 [details] Updated patch according to review

Comment on attachment 94062 [details] Updated patch according to review Attachment 94062 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8719071

A comment as the original implementor: The patch looks a bit too complex, e.g the AttachmentInfo seems a bit redundant. What is the condition when the handle file descriptor is -1? I think that is the bug that should be solved? Does the problem appear as ASSERT Handle::encode? If so, you should be able to solve it like following: Modified Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp diff --git a/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp b/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp index 3dce1c2..9789f81 100644 --- a/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp +++ b/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp @@ -69,9 +69,13 @@ bool SharedMemory::Handle::isNull() const void SharedMemory::Handle::encode(CoreIPC::ArgumentEncoder* encoder) const { - ASSERT(!isNull()); + if (isNull()) + encoder->encode(false); + else { + encoder->encode(true); + encoder->encode(releaseToAttachment()); + } - encoder->encode(releaseToAttachment()); } bool SharedMemory::Handle::decode(CoreIPC::ArgumentDecoder* decoder, Handle& handle) @@ -79,6 +83,13 @@ bool SharedMemory::Handle::decode(CoreIPC::ArgumentDecoder* decoder, Handle& han ASSERT_ARG(handle, !handle.m_size); ASSERT_ARG(handle, handle.isNull()); + bool hasContent = false; + if (!decoder->decode(hasContent)) + return false; + + if (!hasContent) + return true; + CoreIPC::Attachment attachment; if (!decoder->decode(attachment)) return false; Note, that's just pseudocode, I haven't tried it.. In general: also if only possible, it would be great if the bug reports would provide a bit more concrete details about the actual symptom. Eg. if it asserts or crashes, you should provide info about where an which conditions. Jumping to conclusions might be a bit non-productive..

(In reply to comment #15) > A comment as the original implementor: > The patch looks a bit too complex, e.g the AttachmentInfo seems a bit redundant. > > What is the condition when the handle file descriptor is -1? I think that is the bug that should be solved? Does the problem appear as ASSERT Handle::encode? GeometryDidChange message sends a null handle on purpose when there's no need to create a new backing store. Receiver checks whether handle is null to decide whether to create a new backing store or not. I talked to Anders Carlsson and he confirmed that the expected behaviour so we should find a way to send a null handle. As I said before the problem of sending a null handle is that the file descriptor is -1. When sending the message using sendmsg(), it tries to dup the descriptors and when it finds the invalid file descritor, sendmsg fails with errno = EBADF and the message is not called. > If so, you should be able to solve it like following: > > Modified Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp > diff --git a/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp b/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp > index 3dce1c2..9789f81 100644 > --- a/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp > +++ b/Source/WebKit2/Platform/unix/SharedMemoryUnix.cpp > @@ -69,9 +69,13 @@ bool SharedMemory::Handle::isNull() const > > void SharedMemory::Handle::encode(CoreIPC::ArgumentEncoder* encoder) const > { > - ASSERT(!isNull()); > + if (isNull()) > + encoder->encode(false); > + else { > + encoder->encode(true); > + encoder->encode(releaseToAttachment()); > + } > > - encoder->encode(releaseToAttachment()); > } > > bool SharedMemory::Handle::decode(CoreIPC::ArgumentDecoder* decoder, Handle& handle) > @@ -79,6 +83,13 @@ bool SharedMemory::Handle::decode(CoreIPC::ArgumentDecoder* decoder, Handle& han > ASSERT_ARG(handle, !handle.m_size); > ASSERT_ARG(handle, handle.isNull()); > > + bool hasContent = false; > + if (!decoder->decode(hasContent)) > + return false; > + > + if (!hasContent) > + return true; > + > CoreIPC::Attachment attachment; > if (!decoder->decode(attachment)) > return false; > > > Note, that's just pseudocode, I haven't tried it.. > > > In general: also if only possible, it would be great if the bug reports would provide a bit more concrete details about the actual symptom. Eg. if it asserts or crashes, you should provide info about where an which conditions. Jumping to conclusions might be a bit non-productive.. Yes, sorry, this bug report was created when working on other bugs, I explained the problem in bugs #57617 and #60629, and I talked about the issue with apple guys on irc.

Created attachment 96078 [details] Updated patch to current git master ConnectionUnix.cpp file has changed a bit after r88147, so I've updated the patch to apply to current git master.

(In reply to comment #17) > Created an attachment (id=96078) [details] > Updated patch to current git master > > ConnectionUnix.cpp file has changed a bit after r88147, so I've updated the patch to apply to current git master. Is it really depends now on 60629? It seems it can be applied without 60629. Am I right?

(In reply to comment #18) > (In reply to comment #17) > > Created an attachment (id=96078) [details] [details] > > Updated patch to current git master > > > > ConnectionUnix.cpp file has changed a bit after r88147, so I've updated the patch to apply to current git master. > > Is it really depends now on 60629? It seems it can be applied without 60629. Am I right? No, it still requires 60629, because it adds the attachment types (SocketType and MappedType, this one renamed actually) used by this patch.

Created attachment 98330 [details] Updated patch It simply removes a couple of asserts that were wrong and are not actually needed and fixes a memory corruption issue with the file descriptors buffer.

Comment on attachment 98330 [details] Updated patch Attachment 98330 [details] did not pass qt-ews (qt): Output: http://queues.webkit.org/results/8923982

Committed r91016: <http://trac.webkit.org/changeset/91016>