Bug 1748759: ipc cleanup r=ipc-reviewers,nika

Differential Revision: https://phabricator.services.mozilla.com/D135184

dom/ipc/ContentParent.cpp

@@ -478,12 +478,13 @@ ContentParentsMemoryReporter::CollectReports(
     const char* channelStr = "no channel";
     uint32_t numQueuedMessages = 0;
     if (channel) {
-      if (channel->Unsound_IsClosed()) {
+      if (channel->IsClosed()) {
         channelStr = "closed channel";
       } else {
         channelStr = "open channel";
       }
-      numQueuedMessages = channel->Unsound_NumQueuedMessages();
+      numQueuedMessages =
+          0;  // XXX was channel->Unsound_NumQueuedMessages(); Bug 1754876
     }
 
     nsPrintfCString path(

ipc/chromium/src/chrome/common/ipc_channel.h

@@ -133,11 +133,10 @@ class Channel {
   // `-1` until `OnChannelConnected` has been called.
   int32_t OtherPid() const;
 
-  // Unsound_IsClosed() and Unsound_NumQueuedMessages() are safe to call from
-  // any thread, but the value returned may be out of date, because we don't
-  // use any synchronization when reading or writing it.
-  bool Unsound_IsClosed() const;
-  uint32_t Unsound_NumQueuedMessages() const;
+  // IsClosed() is safe to call from any thread, but the value returned may
+  // be out of date, because we don't use any synchronization when reading
+  // or writing it.
+  bool IsClosed() const;
 
 #if defined(OS_POSIX)
   // On POSIX an IPC::Channel wraps a socketpair(), this method returns the

ipc/chromium/src/chrome/common/ipc_channel_posix.cc

@@ -214,7 +214,6 @@ void Channel::ChannelImpl::Init(Mode mode, Listener* listener) {
   last_pending_fd_id_ = 0;
   other_task_ = nullptr;
 #endif
-  output_queue_length_ = 0;
 }
 
 bool Channel::ChannelImpl::CreatePipe(Mode mode) {
@@ -840,7 +839,6 @@ void Channel::ChannelImpl::OutputQueuePush(mozilla::UniquePtr<Message> msg) {
   MOZ_DIAGNOSTIC_ASSERT(!closed_);
   msg->AssertAsLargeAsHeader();
   output_queue_.Push(std::move(msg));
-  output_queue_length_++;
 }
 
 void Channel::ChannelImpl::OutputQueuePop() {
@@ -848,7 +846,6 @@ void Channel::ChannelImpl::OutputQueuePop() {
   partial_write_iter_.reset();
 
   mozilla::UniquePtr<Message> message = output_queue_.Pop();
-  output_queue_length_--;
 }
 
 // Called by libevent when we can write to the pipe without blocking.
@@ -1164,11 +1161,7 @@ bool Channel::ChannelImpl::TransferMachPorts(Message& msg) {
 }
 #endif
 
-bool Channel::ChannelImpl::Unsound_IsClosed() const { return closed_; }
-
-uint32_t Channel::ChannelImpl::Unsound_NumQueuedMessages() const {
-  return output_queue_length_;
-}
+bool Channel::ChannelImpl::IsClosed() const { return closed_; }
 
 //------------------------------------------------------------------------------
 // Channel's methods simply call through to ChannelImpl.
@@ -1213,13 +1206,7 @@ void Channel::CloseClientFileDescriptor() {
 
 int32_t Channel::OtherPid() const { return channel_impl_->OtherPid(); }
 
-bool Channel::Unsound_IsClosed() const {
-  return channel_impl_->Unsound_IsClosed();
-}
-
-uint32_t Channel::Unsound_NumQueuedMessages() const {
-  return channel_impl_->Unsound_NumQueuedMessages();
-}
+bool Channel::IsClosed() const { return channel_impl_->IsClosed(); }
 
 #if defined(OS_MACOSX)
 void Channel::SetOtherMachTask(task_t task) {

ipc/chromium/src/chrome/common/ipc_channel_posix.h

@@ -51,10 +51,8 @@ class Channel::ChannelImpl : public MessageLoopForIO::Watcher {
 
   int32_t OtherPid() const { return other_pid_; }
 
-  // See the comment in ipc_channel.h for info on Unsound_IsClosed() and
-  // Unsound_NumQueuedMessages().
-  bool Unsound_IsClosed() const;
-  uint32_t Unsound_NumQueuedMessages() const;
+  // See the comment in ipc_channel.h for info on IsClosed()
+  bool IsClosed() const;
 
 #if defined(OS_MACOSX)
   void SetOtherMachTask(task_t task);
@@ -178,12 +176,6 @@ class Channel::ChannelImpl : public MessageLoopForIO::Watcher {
   mozilla::UniqueMachSendRight other_task_;
 #endif
 
-  // This variable is updated so it matches output_queue_.Count(), except we can
-  // read output_queue_length_ from any thread (if we're OK getting an
-  // occasional out-of-date or bogus value).  We use output_queue_length_ to
-  // implement Unsound_NumQueuedMessages.
-  std::atomic<size_t> output_queue_length_;
-
   ScopedRunnableMethodFactory<ChannelImpl> factory_;
 
   DISALLOW_COPY_AND_ASSIGN(ChannelImpl);

ipc/chromium/src/chrome/common/ipc_channel_win.cc

@@ -100,7 +100,6 @@ void Channel::ChannelImpl::Init(Mode mode, Listener* listener) {
   waiting_connect_ = (mode == MODE_SERVER);
   processing_incoming_ = false;
   closed_ = false;
-  output_queue_length_ = 0;
   input_buf_offset_ = 0;
   input_buf_ = mozilla::MakeUnique<char[]>(Channel::kReadBufferSize);
   accept_handles_ = false;
@@ -112,12 +111,10 @@ void Channel::ChannelImpl::OutputQueuePush(mozilla::UniquePtr<Message> msg) {
   mozilla::LogIPCMessage::LogDispatchWithPid(msg.get(), other_pid_);
 
   output_queue_.Push(std::move(msg));
-  output_queue_length_++;
 }
 
 void Channel::ChannelImpl::OutputQueuePop() {
   mozilla::UniquePtr<Message> message = output_queue_.Pop();
-  output_queue_length_--;
 }
 
 void Channel::ChannelImpl::Close() {
@@ -744,11 +741,7 @@ bool Channel::ChannelImpl::TransferHandles(Message& msg) {
   return true;
 }
 
-bool Channel::ChannelImpl::Unsound_IsClosed() const { return closed_; }
-
-uint32_t Channel::ChannelImpl::Unsound_NumQueuedMessages() const {
-  return output_queue_length_;
-}
+bool Channel::ChannelImpl::IsClosed() const { return closed_; }
 
 //------------------------------------------------------------------------------
 // Channel's methods simply call through to ChannelImpl.
@@ -785,13 +778,7 @@ bool Channel::Send(mozilla::UniquePtr<Message> message) {
 
 int32_t Channel::OtherPid() const { return channel_impl_->OtherPid(); }
 
-bool Channel::Unsound_IsClosed() const {
-  return channel_impl_->Unsound_IsClosed();
-}
-
-uint32_t Channel::Unsound_NumQueuedMessages() const {
-  return channel_impl_->Unsound_NumQueuedMessages();
-}
+bool Channel::IsClosed() const { return channel_impl_->IsClosed(); }
 
 namespace {
 

ipc/chromium/src/chrome/common/ipc_channel_win.h

@@ -49,10 +49,8 @@ class Channel::ChannelImpl : public MessageLoopForIO::IOHandler {
 
   int32_t OtherPid() const { return other_pid_; }
 
-  // See the comment in ipc_channel.h for info on Unsound_IsClosed() and
-  // Unsound_NumQueuedMessages().
-  bool Unsound_IsClosed() const;
-  uint32_t Unsound_NumQueuedMessages() const;
+  // See the comment in ipc_channel.h for info on IsClosed()
+  bool IsClosed() const;
 
  private:
   void Init(Mode mode, Listener* listener);
@@ -127,12 +125,6 @@ class Channel::ChannelImpl : public MessageLoopForIO::IOHandler {
   // record this when generating logs of IPC messages.
   int32_t other_pid_ = -1;
 
-  // This variable is updated so it matches output_queue_.Count(), except we can
-  // read output_queue_length_ from any thread (if we're OK getting an
-  // occasional out-of-date or bogus value).  We use output_queue_length_ to
-  // implement Unsound_NumQueuedMessages.
-  std::atomic<size_t> output_queue_length_ = 0;
-
   ScopedRunnableMethodFactory<ChannelImpl> factory_;
 
   // This is a unique per-channel value used to authenticate the client end of

ipc/glue/MessageChannel.cpp

@@ -462,7 +462,7 @@ MessageChannel::~MessageChannel() {
   // it was destroyed. We can't properly close the channel at this point, as it
   // would be unsafe to invoke our listener's callbacks, and we may be being
   // destroyed on a thread other than `mWorkerThread`.
-  if (!Unsound_IsClosed()) {
+  if (!IsClosedLocked()) {
     CrashReporter::AnnotateCrashReport(
         CrashReporter::Annotation::IPCFatalErrorProtocol,
         nsDependentCString(mName));
@@ -578,8 +578,7 @@ bool MessageChannel::CanSend() const {
 void MessageChannel::Clear() {
   AssertWorkerThread();
   mMonitor->AssertCurrentThreadOwns();
-  MOZ_DIAGNOSTIC_ASSERT(Unsound_IsClosed(),
-                        "MessageChannel cleared too early?");
+  MOZ_DIAGNOSTIC_ASSERT(IsClosedLocked(), "MessageChannel cleared too early?");
 
   // Don't clear mWorkerThread; we use it in AssertWorkerThread().
   //
@@ -622,6 +621,7 @@ bool MessageChannel::Open(ScopedPort aPort, Side aSide,
     MonitorAutoLock lock(*mMonitor);
     MOZ_RELEASE_ASSERT(!mLink, "Open() called > once");
     MOZ_RELEASE_ASSERT(ChannelClosed == mChannelState, "Not currently closed");
+    MOZ_ASSERT(mSide == UnknownSide);
 
     mWorkerThread = aEventTarget ? aEventTarget : GetCurrentSerialEventTarget();
     MOZ_ASSERT(mWorkerThread, "We should always be on a nsISerialEventTarget");
@@ -729,6 +729,8 @@ bool MessageChannel::Send(UniquePtr<Message> aMsg) {
 }
 
 void MessageChannel::SendMessageToLink(UniquePtr<Message> aMsg) {
+  AssertWorkerThread();
+  mMonitor->AssertCurrentThreadOwns();
   if (mIsPostponingSends) {
     mPostponedSends.push_back(std::move(aMsg));
     return;
@@ -1503,6 +1505,7 @@ nsresult MessageChannel::MessageTask::Cancel() {
 
 void MessageChannel::MessageTask::Post() {
   mMonitor->AssertCurrentThreadOwns();
+  Channel()->mMonitor->AssertCurrentThreadOwns();
   MOZ_RELEASE_ASSERT(!mScheduled);
   MOZ_RELEASE_ASSERT(isInList());
 
@@ -1671,6 +1674,7 @@ void MessageChannel::EnqueuePendingMessages() {
 }
 
 bool MessageChannel::WaitResponse(bool aWaitTimedOut) {
+  AssertWorkerThread();
   if (aWaitTimedOut) {
     if (mInTimeoutSecondHalf) {
       // We've really timed out this time.
@@ -1686,6 +1690,7 @@ bool MessageChannel::WaitResponse(bool aWaitTimedOut) {
 
 #ifndef OS_WIN
 bool MessageChannel::WaitForSyncNotify(bool /* aHandleWindowsMessages */) {
+  AssertWorkerThread();
 #  ifdef DEBUG
   // WARNING: We don't release the lock here. We can't because the link
   // could signal at this time and we would miss it. Instead we require

ipc/glue/MessageChannel.h

@@ -288,17 +288,15 @@ class MessageChannel : HasResultCodes {
   // this, if needed, is to make B send a sync message.
   void StopPostponingSends();
 
-  // Unsound_IsClosed and Unsound_NumQueuedMessages are safe to call from any
-  // thread, but they make no guarantees about whether you'll get an
-  // up-to-date value; the values are written on one thread and read without
-  // locking, on potentially different threads.  Thus you should only use
-  // them when you don't particularly care about getting a recent value (e.g.
-  // in a memory report).
-  bool Unsound_IsClosed() const {
-    return mLink ? mLink->Unsound_IsClosed() : true;
+  // IsClosed and NumQueuedMessages are safe to call from any thread, but
+  // may provide an out-of-date value.
+  bool IsClosed() {
+    MonitorAutoLock lock(*mMonitor);
+    return IsClosedLocked();
   }
-  uint32_t Unsound_NumQueuedMessages() const {
-    return mLink ? mLink->Unsound_NumQueuedMessages() : 0;
+  bool IsClosedLocked() const {
+    mMonitor->AssertCurrentThreadOwns();
+    return mLink ? mLink->IsClosed() : true;
   }
 
   static bool IsPumpingMessages() { return sIsPumpingMessages; }
@@ -546,13 +544,16 @@ class MessageChannel : HasResultCodes {
   // NotifyMaybeChannelError runnable
   RefPtr<CancelableRunnable> mChannelErrorTask;
 
-  // Thread we are allowed to send and receive on.
+  // Thread we are allowed to send and receive on.  Set in Open(); never
+  // changed, and we can only call Open() once.  We shouldn't be accessing
+  // from multiple threads before Open().
   nsCOMPtr<nsISerialEventTarget> mWorkerThread;
 
   // Timeout periods are broken up in two to prevent system suspension from
   // triggering an abort. This method (called by WaitForEvent with a 'did
   // timeout' flag) decides if we should wait again for half of mTimeoutMs
   // or give up.
+  // only accessed on WorkerThread
   int32_t mTimeoutMs = kNoTimeout;
   bool mInTimeoutSecondHalf = false;
 

ipc/glue/MessageLink.cpp

@@ -189,20 +189,12 @@ void PortLink::OnPortStatusChanged() {
   }
 }
 
-bool PortLink::Unsound_IsClosed() const {
+bool PortLink::IsClosed() const {
   if (Maybe<PortStatus> status = mNode->GetStatus(mPort)) {
     return !(status->has_messages || status->receiving_messages);
   }
   return true;
 }
 
-uint32_t PortLink::Unsound_NumQueuedMessages() const {
-  // There is no easy way to see the number of messages which have been sent to
-  // a port but haven't been delivered yet.
-  //
-  // FIXME: If this is important, we'll need to add a mechanism for this.
-  return 0;
-}
-
 }  // namespace ipc
 }  // namespace mozilla

ipc/glue/MessageLink.h

@@ -53,8 +53,7 @@ class MessageLink {
   virtual void SendMessage(mozilla::UniquePtr<Message> msg) = 0;
   virtual void SendClose() = 0;
 
-  virtual bool Unsound_IsClosed() const = 0;
-  virtual uint32_t Unsound_NumQueuedMessages() const = 0;
+  virtual bool IsClosed() const = 0;
 
  protected:
   MessageChannel* mChan;
@@ -73,8 +72,7 @@ class PortLink final : public MessageLink {
   void SendMessage(UniquePtr<Message> aMessage) override;
   void SendClose() override;
 
-  bool Unsound_IsClosed() const override;
-  uint32_t Unsound_NumQueuedMessages() const override;
+  bool IsClosed() const override;
 
  private:
   class PortObserverThunk;