Bug 1635720 - add diagnostic asserts for outgoing IPC messages; r=jld,nika
authorNathan Froyd <froydnj@mozilla.com>
Fri, 22 May 2020 22:25:27 +0000
changeset 531747 1a717f48e1a260ac33fb9df220337f84b41a5e2f
parent 531746 e549d67814b4839ca45b0023dcc8a008454d1590
child 531748 6e3ec75f2e317dcb826c15a435872cf46f84fd66
push id37442
push userncsoregi@mozilla.com
push dateSat, 23 May 2020 09:21:24 +0000
treeherdermozilla-central@bbcc193fe0f0 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjld, nika
bugs1635720
milestone78.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1635720 - add diagnostic asserts for outgoing IPC messages; r=jld,nika We are seeing crashes on aarch64 Fenix devices that appear to be related to zero-sized messages. But we're seeing the crashes when we're trying to send the messages on the IO thread, and not where we're dispatching them from. Add some asserts so we get errors closer to the source, and add some asserts for other things that we believe to be true and would be useful to know aren't actually true. Differential Revision: https://phabricator.services.mozilla.com/D76496
ipc/chromium/src/chrome/common/ipc_channel_posix.cc
ipc/chromium/src/chrome/common/ipc_message.cc
ipc/chromium/src/chrome/common/ipc_message.h
ipc/glue/MessageLink.cpp
--- a/ipc/chromium/src/chrome/common/ipc_channel_posix.cc
+++ b/ipc/chromium/src/chrome/common/ipc_channel_posix.cc
@@ -483,17 +483,17 @@ bool Channel::ChannelImpl::ProcessIncomi
       bool partial;
       if (incoming_message_.isSome()) {
         // We already have some data for this message stored in
         // incoming_message_. We want to append the new data there.
         Message& m = incoming_message_.ref();
 
         // How much data from this message remains to be added to
         // incoming_message_?
-        MOZ_ASSERT(message_length > m.CurrentSize());
+        MOZ_DIAGNOSTIC_ASSERT(message_length > m.CurrentSize());
         uint32_t remaining = message_length - m.CurrentSize();
 
         // How much data from this message is stored in input_buf_?
         uint32_t in_buf = std::min(remaining, uint32_t(end - p));
 
         m.InputBytes(p, in_buf);
         p += in_buf;
 
@@ -728,18 +728,21 @@ bool Channel::ChannelImpl::ProcessOutgoi
           }
           return false;
       }
     }
 
     if (static_cast<size_t>(bytes_written) != amt_to_write) {
       // If write() fails with EAGAIN then bytes_written will be -1.
       if (bytes_written > 0) {
+        MOZ_DIAGNOSTIC_ASSERT(static_cast<size_t>(bytes_written) < amt_to_write);
         partial_write_iter_.ref().AdvanceAcrossSegments(msg->Buffers(),
                                                         bytes_written);
+        // We should not hit the end of the buffer.
+        MOZ_DIAGNOSTIC_ASSERT(!partial_write_iter_.ref().Done());
       }
 
       // Tell libevent to call us back once things are unblocked.
       is_blocked_on_write_ = true;
       MessageLoopForIO::current()->WatchFileDescriptor(
           pipe_,
           false,  // One shot
           MessageLoopForIO::WATCH_WRITE, &write_watcher_, this);
@@ -839,16 +842,17 @@ void Channel::ChannelImpl::CloseDescript
       return;
     }
   }
   DCHECK(false) << "pending_fd_id not in our list!";
 }
 #endif
 
 void Channel::ChannelImpl::OutputQueuePush(Message* msg) {
+  msg->AssertAsLargeAsHeader();
   output_queue_.push(msg);
   output_queue_length_++;
 }
 
 void Channel::ChannelImpl::OutputQueuePop() {
   output_queue_.pop();
   output_queue_length_--;
 }
--- a/ipc/chromium/src/chrome/common/ipc_message.cc
+++ b/ipc/chromium/src/chrome/common/ipc_message.cc
@@ -179,16 +179,23 @@ void Message::EnsureFileDescriptorSet() 
 }
 
 uint32_t Message::num_fds() const {
   return file_descriptor_set() ? file_descriptor_set()->size() : 0;
 }
 
 #endif
 
+void Message::AssertAsLargeAsHeader() const {
+  MOZ_DIAGNOSTIC_ASSERT(size() >= MSG_HEADER_SZ);
+  MOZ_DIAGNOSTIC_ASSERT(CurrentSize() >= MSG_HEADER_SZ);
+  // Our buffers should agree with what our header specifies.
+  MOZ_DIAGNOSTIC_ASSERT(size() == CurrentSize());
+}
+
 #ifdef MOZ_TASK_TRACER
 void* MessageTask() { return reinterpret_cast<void*>(&MessageTask); }
 
 void Message::TaskTracerDispatch() {
   if (header()->flags.IsTaskTracer()) {
     HeaderTaskTracer* _header = static_cast<HeaderTaskTracer*>(header());
     _header->task_id = GenNewUniqueTaskId();
     uintptr_t* vtab = reinterpret_cast<uintptr_t*>(&MessageTask);
--- a/ipc/chromium/src/chrome/common/ipc_message.h
+++ b/ipc/chromium/src/chrome/common/ipc_message.h
@@ -283,16 +283,19 @@ class Message : public Pickle {
 
   template <class T>
   static bool Dispatch(const Message* msg, T* obj,
                        void (T::*func)(const Message&) const) {
     (obj->*func)(*msg);
     return true;
   }
 
+  // We should not be sending messages that are smaller than our header size.
+  void AssertAsLargeAsHeader() const;
+
   // Used for async messages with no parameters.
   static void Log(const Message* msg, std::wstring* l) {}
 
   static int HeaderSizeFromData(const char* range_start,
                                 const char* range_end) {
 #ifdef MOZ_TASK_TRACER
     return ((static_cast<unsigned int>(range_end - range_start) >=
              sizeof(Header)) &&
--- a/ipc/glue/MessageLink.cpp
+++ b/ipc/glue/MessageLink.cpp
@@ -153,16 +153,18 @@ void ProcessLink::SendMessage(Message* m
     MOZ_CRASH("IPC message size is too large");
   }
 
   if (!mChan->mIsPostponingSends) {
     mChan->AssertWorkerThread();
   }
   mChan->mMonitor->AssertCurrentThreadOwns();
 
+  msg->AssertAsLargeAsHeader();
+
   mIOLoop->PostTask(NewNonOwningRunnableMethod<Message*>(
       "IPC::Channel::Send", mTransport.get(), &Transport::Send, msg));
 }
 
 void ProcessLink::SendClose() {
   mChan->AssertWorkerThread();
   mChan->mMonitor->AssertCurrentThreadOwns();