Bug 1273307 - Remove copy constructor/assignment operator for Message/Pickle (r=froydnj)
authorBill McCloskey <billm@mozilla.com>
Mon, 16 May 2016 14:02:43 -0700
changeset 372282 ea8c597133e5d952dc92bac75c69b4620c37178b
parent 372281 d9e6ae164b6e06fff75872d02531ad1928038c0e
child 372283 64fcfbf2da25d2563d53fcb518fa355eb72cb2c5
push id19496
push userdmitchell@mozilla.com
push dateFri, 27 May 2016 22:17:17 +0000
reviewersfroydnj
bugs1273307
milestone49.0a1
Bug 1273307 - Remove copy constructor/assignment operator for Message/Pickle (r=froydnj)
ipc/chromium/src/base/pickle.cc
ipc/chromium/src/base/pickle.h
ipc/chromium/src/chrome/common/ipc_message.cc
ipc/chromium/src/chrome/common/ipc_message.h
ipc/glue/MessageChannel.cpp
ipc/glue/MessageChannel.h
--- a/ipc/chromium/src/base/pickle.cc
+++ b/ipc/chromium/src/base/pickle.cc
@@ -140,53 +140,31 @@ Pickle::Pickle(const char* data, int dat
   if (header_size_ != AlignInt(header_size_))
     header_size_ = 0;
 
   // If there is anything wrong with the data, we're not going to use it.
   if (!header_size_)
     header_ = nullptr;
 }
 
-Pickle::Pickle(const Pickle& other)
-    : header_(NULL),
-      header_size_(other.header_size_),
-      capacity_(0),
-      variable_buffer_offset_(other.variable_buffer_offset_) {
-  uint32_t payload_size = header_size_ + other.header_->payload_size;
-  Resize(payload_size);
-  memcpy(header_, other.header_, payload_size);
-}
-
 Pickle::Pickle(Pickle&& other)
   : header_(other.header_),
     header_size_(other.header_size_),
     capacity_(other.capacity_),
     variable_buffer_offset_(other.variable_buffer_offset_) {
   other.header_ = NULL;
   other.capacity_ = 0;
   other.variable_buffer_offset_ = 0;
 }
 
 Pickle::~Pickle() {
   if (capacity_ != kCapacityReadOnly)
     free(header_);
 }
 
-Pickle& Pickle::operator=(const Pickle& other) {
-  if (header_size_ != other.header_size_ && capacity_ != kCapacityReadOnly) {
-    free(header_);
-    header_ = NULL;
-    header_size_ = other.header_size_;
-  }
-  Resize(other.header_size_ + other.header_->payload_size);
-  memcpy(header_, other.header_, header_size_ + other.header_->payload_size);
-  variable_buffer_offset_ = other.variable_buffer_offset_;
-  return *this;
-}
-
 Pickle& Pickle::operator=(Pickle&& other) {
   std::swap(header_, other.header_);
   std::swap(header_size_, other.header_size_);
   std::swap(capacity_, other.capacity_);
   std::swap(variable_buffer_offset_, other.variable_buffer_offset_);
   return *this;
 }
 
--- a/ipc/chromium/src/base/pickle.h
+++ b/ipc/chromium/src/base/pickle.h
@@ -52,23 +52,22 @@ class Pickle {
   // Initializes a Pickle from a const block of data. If ownership == BORROWS,
   // the data is not copied; instead the data is merely referenced by this
   // Pickle. Only const methods should be used on the Pickle when initialized
   // this way. The header padding size is deduced from the data length.  If
   // ownership == OWNS, then again no copying takes place. However, the buffer
   // is writable and will be freed when this Pickle is destroyed.
   Pickle(const char* data, int data_len, Ownership ownership = BORROWS);
 
-  // Initializes a Pickle as a deep copy of another Pickle.
-  Pickle(const Pickle& other);
+  Pickle(const Pickle& other) = delete;
 
   Pickle(Pickle&& other);
 
   // Performs a deep copy.
-  Pickle& operator=(const Pickle& other);
+  Pickle& operator=(const Pickle& other) = delete;
 
   Pickle& operator=(Pickle&& other);
 
   // Returns the size of the Pickle's data.
   int size() const { return static_cast<int>(header_size_ +
                                              header_->payload_size); }
 
   // Return the full size of the memory allocated for this Pickle's data.
--- a/ipc/chromium/src/chrome/common/ipc_message.cc
+++ b/ipc/chromium/src/chrome/common/ipc_message.cc
@@ -75,29 +75,16 @@ Message::Message(int32_t routing_id, msg
 
 Message::Message(const char* data, int data_len, Ownership ownership)
   : Pickle(data, data_len, ownership)
 {
   MOZ_COUNT_CTOR(IPC::Message);
   InitLoggingVariables();
 }
 
-Message::Message(const Message& other) : Pickle(other) {
-  MOZ_COUNT_CTOR(IPC::Message);
-  InitLoggingVariables(other.name_);
-#if defined(OS_POSIX)
-  file_descriptor_set_ = other.file_descriptor_set_;
-#endif
-#ifdef MOZ_TASK_TRACER
-  header()->source_event_id = other.header()->source_event_id;
-  header()->parent_task_id = other.header()->parent_task_id;
-  header()->source_event_type = other.header()->source_event_type;
-#endif
-}
-
 Message::Message(Message&& other) : Pickle(mozilla::Move(other)) {
   MOZ_COUNT_CTOR(IPC::Message);
   InitLoggingVariables(other.name_);
 #if defined(OS_POSIX)
   file_descriptor_set_ = other.file_descriptor_set_.forget();
 #endif
 #ifdef MOZ_TASK_TRACER
   header()->source_event_id = other.header()->source_event_id;
@@ -105,30 +92,16 @@ Message::Message(Message&& other) : Pick
   header()->source_event_type = other.header()->source_event_type;
 #endif
 }
 
 void Message::InitLoggingVariables(const char* const aName) {
   name_ = aName;
 }
 
-Message& Message::operator=(const Message& other) {
-  *static_cast<Pickle*>(this) = other;
-  InitLoggingVariables(other.name_);
-#if defined(OS_POSIX)
-  file_descriptor_set_ = other.file_descriptor_set_;
-#endif
-#ifdef MOZ_TASK_TRACER
-  header()->source_event_id = other.header()->source_event_id;
-  header()->parent_task_id = other.header()->parent_task_id;
-  header()->source_event_type = other.header()->source_event_type;
-#endif
-  return *this;
-}
-
 Message& Message::operator=(Message&& other) {
   *static_cast<Pickle*>(this) = mozilla::Move(other);
   InitLoggingVariables(other.name_);
 #if defined(OS_POSIX)
   file_descriptor_set_.swap(other.file_descriptor_set_);
 #endif
 #ifdef MOZ_TASK_TRACER
   std::swap(header()->source_event_id, other.header()->source_event_id);
--- a/ipc/chromium/src/chrome/common/ipc_message.h
+++ b/ipc/chromium/src/chrome/common/ipc_message.h
@@ -74,19 +74,19 @@ class Message : public Pickle {
 
   // Initializes a message from a const block of data. If ownership == BORROWS,
   // the data is not copied; instead the data is merely referenced by this
   // message. Only const methods should be used on the message when initialized
   // this way. If ownership == OWNS, then again no copying takes place. However,
   // the buffer is writable and will be freed when the message is destroyed.
   Message(const char* data, int data_len, Ownership ownership = BORROWS);
 
-  Message(const Message& other);
+  Message(const Message& other) = delete;
   Message(Message&& other);
-  Message& operator=(const Message& other);
+  Message& operator=(const Message& other) = delete;
   Message& operator=(Message&& other);
 
   PriorityValue priority() const {
     return static_cast<PriorityValue>(header()->flags & PRIORITY_MASK);
   }
 
   void set_priority(int prio) {
     DCHECK((prio & ~PRIORITY_MASK) == 0);
--- a/ipc/glue/MessageChannel.cpp
+++ b/ipc/glue/MessageChannel.cpp
@@ -16,16 +16,18 @@
 #include "mozilla/SizePrintfMacros.h"
 #include "mozilla/Snprintf.h"
 #include "mozilla/Telemetry.h"
 #include "mozilla/Logging.h"
 #include "nsDebug.h"
 #include "nsISupportsImpl.h"
 #include "nsContentUtils.h"
 
+using mozilla::Move;
+
 // Undo the damage done by mozzconf.h
 #undef compress
 
 // Logging seems to be somewhat broken on b2g.
 #ifdef MOZ_B2G
 #define IPC_LOG(...)
 #else
 static LazyLogModule sLogModule("ipc");
@@ -165,17 +167,17 @@ public:
     {
         MOZ_RELEASE_ASSERT(mMessageName || mMoved);
     }
 
     InterruptFrame& operator=(InterruptFrame&& aOther)
     {
         MOZ_RELEASE_ASSERT(&aOther != this);
         this->~InterruptFrame();
-        new (this) InterruptFrame(mozilla::Move(aOther));
+        new (this) InterruptFrame(Move(aOther));
         return *this;
     }
 
     bool IsInterruptIncall() const
     {
         return INTR_SEMS == mMesageSemantics && IN_MESSAGE == mDirection;
     }
 
@@ -386,32 +388,32 @@ public:
         return mSeqno;
     }
 
     int32_t TransactionID() const {
         MOZ_RELEASE_ASSERT(mActive);
         return mTransaction;
     }
 
-    void ReceivedReply(const IPC::Message& aMessage) {
+    void ReceivedReply(IPC::Message&& aMessage) {
         MOZ_RELEASE_ASSERT(aMessage.seqno() == mSeqno);
         MOZ_RELEASE_ASSERT(aMessage.transaction_id() == mTransaction);
         MOZ_RELEASE_ASSERT(!mReply);
         IPC_LOG("Reply received on worker thread: seqno=%d", mSeqno);
-        mReply = new IPC::Message(aMessage);
+        mReply = new IPC::Message(Move(aMessage));
         MOZ_RELEASE_ASSERT(IsComplete());
     }
 
-    void HandleReply(const IPC::Message& aMessage) {
+    void HandleReply(IPC::Message&& aMessage) {
         AutoEnterTransaction *cur = mChan->mTransactionStack;
         MOZ_RELEASE_ASSERT(cur == this);
         while (cur) {
             MOZ_RELEASE_ASSERT(cur->mActive);
             if (aMessage.seqno() == cur->mSeqno) {
-                cur->ReceivedReply(aMessage);
+                cur->ReceivedReply(Move(aMessage));
                 break;
             }
             cur = cur->mNext;
             MOZ_RELEASE_ASSERT(cur);
         }
     }
 
     bool IsComplete() {
@@ -894,17 +896,17 @@ MessageChannel::OnMessageReceivedFromLin
             IPC_LOG("Received reply to timedout message; igoring; xid=%d", mTimedOutMessageSeqno);
             EndTimeout();
             return;
         }
 
         MOZ_RELEASE_ASSERT(AwaitingSyncReply());
         MOZ_RELEASE_ASSERT(!mTimedOutMessageSeqno);
 
-        mTransactionStack->HandleReply(aMsg);
+        mTransactionStack->HandleReply(Move(aMsg));
         NotifyWorkerThread();
         return;
     }
 
     // Prioritized messages cannot be compressed.
     MOZ_RELEASE_ASSERT(aMsg.compress_type() == IPC::Message::COMPRESSION_NONE ||
                        aMsg.priority() == IPC::Message::PRIORITY_NORMAL);
 
@@ -1045,17 +1047,17 @@ MessageChannel::ProcessPendingRequests(A
         if (toProcess.empty()) {
             break;
         }
 
         // Processing these messages could result in more messages, so we
         // loop around to check for more afterwards.
 
         for (auto it = toProcess.begin(); it != toProcess.end(); it++) {
-            ProcessPendingRequest(*it);
+            ProcessPendingRequest(Move(*it));
         }
     }
 }
 
 bool
 MessageChannel::Send(Message* aMsg, Message* aReply)
 {
     if (aMsg->capacity() >= kMinTelemetryMessageSize) {
@@ -1350,17 +1352,17 @@ MessageChannel::Call(Message* aMsg, Mess
             // deferred in-call that needs to be processed.  either way, we
             // won't break the inner while loop again until something new
             // happens.
             continue;
         }
 
         // If the message is not Interrupt, we can dispatch it as normal.
         if (!recvd.is_interrupt()) {
-            DispatchMessage(recvd);
+            DispatchMessage(Move(recvd));
             if (!Connected()) {
                 ReportConnectionError("MessageChannel::DispatchMessage");
                 return false;
             }
             continue;
         }
 
         // If the message is an Interrupt reply, either process it as a reply to our
@@ -1408,17 +1410,17 @@ MessageChannel::Call(Message* aMsg, Mess
 
         // Dispatch an Interrupt in-call. Snapshot the current stack depth while we
         // own the monitor.
         size_t stackDepth = InterruptStackDepth();
         {
             MonitorAutoUnlock unlock(*mMonitor);
 
             CxxStackFrame frame(*this, IN_MESSAGE, &recvd);
-            DispatchInterruptMessage(recvd, stackDepth);
+            DispatchInterruptMessage(Move(recvd), stackDepth);
         }
         if (!Connected()) {
             ReportConnectionError("MessageChannel::DispatchInterruptMessage");
             return false;
         }
     }
 
     return true;
@@ -1464,24 +1466,24 @@ MessageChannel::InterruptEventOccurred()
     return (!Connected() ||
             !mPending.empty() ||
             (!mOutOfTurnReplies.empty() &&
              mOutOfTurnReplies.find(mInterruptStack.top().seqno()) !=
              mOutOfTurnReplies.end()));
 }
 
 bool
-MessageChannel::ProcessPendingRequest(const Message &aUrgent)
+MessageChannel::ProcessPendingRequest(Message &&aUrgent)
 {
     AssertWorkerThread();
     mMonitor->AssertCurrentThreadOwns();
 
     IPC_LOG("Process pending: seqno=%d, xid=%d", aUrgent.seqno(), aUrgent.transaction_id());
 
-    DispatchMessage(aUrgent);
+    DispatchMessage(Move(aUrgent));
     if (!Connected()) {
         ReportConnectionError("MessageChannel::ProcessPendingRequest");
         return false;
     }
 
     return true;
 }
 
@@ -1552,23 +1554,23 @@ MessageChannel::OnMaybeDequeueOne()
 
     if (IsOnCxxStack() && recvd.is_interrupt() && recvd.is_reply()) {
         // We probably just received a reply in a nested loop for an
         // Interrupt call sent before entering that loop.
         mOutOfTurnReplies[recvd.seqno()] = Move(recvd);
         return false;
     }
 
-    DispatchMessage(recvd);
+    DispatchMessage(Move(recvd));
 
     return true;
 }
 
 void
-MessageChannel::DispatchMessage(const Message &aMsg)
+MessageChannel::DispatchMessage(Message &&aMsg)
 {
     Maybe<AutoNoJSAPI> nojsapi;
     if (ScriptSettingsInitialized() && NS_IsMainThread())
         nojsapi.emplace();
 
     nsAutoPtr<Message> reply;
 
     IPC_LOG("DispatchMessage: seqno=%d, xid=%d", aMsg.seqno(), aMsg.transaction_id());
@@ -1583,17 +1585,17 @@ MessageChannel::DispatchMessage(const Me
             MonitorAutoUnlock unlock(*mMonitor);
             CxxStackFrame frame(*this, IN_MESSAGE, &aMsg);
 
             mListener->ArtificialSleep();
 
             if (aMsg.is_sync())
                 DispatchSyncMessage(aMsg, *getter_Transfers(reply));
             else if (aMsg.is_interrupt())
-                DispatchInterruptMessage(aMsg, 0);
+                DispatchInterruptMessage(Move(aMsg), 0);
             else
                 DispatchAsyncMessage(aMsg);
 
             mListener->ArtificialSleep();
         }
 
         if (reply && transaction.IsCanceled()) {
             // The transaction has been canceled. Don't send a reply.
@@ -1653,17 +1655,17 @@ MessageChannel::DispatchAsyncMessage(con
         AutoSetValue<bool> async(mDispatchingAsyncMessage, true);
         AutoSetValue<int> prioSet(mDispatchingAsyncMessagePriority, prio);
         rv = mListener->OnMessageReceived(aMsg);
     }
     MaybeHandleError(rv, aMsg, "DispatchAsyncMessage");
 }
 
 void
-MessageChannel::DispatchInterruptMessage(const Message& aMsg, size_t stackDepth)
+MessageChannel::DispatchInterruptMessage(Message&& aMsg, size_t stackDepth)
 {
     AssertWorkerThread();
     mMonitor->AssertNotCurrentThreadOwns();
 
     IPC_ASSERT(aMsg.is_interrupt() && !aMsg.is_reply(), "wrong message type");
 
     // Race detection: see the long comment near mRemoteStackDepthGuess in
     // MessageChannel.h. "Remote" stack depth means our side, and "local" means
@@ -1701,17 +1703,17 @@ MessageChannel::DispatchInterruptMessage
                           winner,
                           defer ? " " : " not ");
         }
 
         if (defer) {
             // We now know the other side's stack has one more frame
             // than we thought.
             ++mRemoteStackDepthGuess; // decremented in MaybeProcessDeferred()
-            mDeferred.push(aMsg);
+            mDeferred.push(Move(aMsg));
             return;
         }
 
         // We "lost" and need to process the other side's in-call. Don't need
         // to fix up the mRemoteStackDepthGuess here, because we're just about
         // to increment it in DispatchCall(), which will make it correct again.
     }
 
@@ -2230,17 +2232,17 @@ MessageChannel::NotifyChannelClosed()
     mListener->OnChannelClose();
 
     Clear();
 }
 
 void
 MessageChannel::DebugAbort(const char* file, int line, const char* cond,
                            const char* why,
-                           bool reply) const
+                           bool reply)
 {
     printf_stderr("###!!! [MessageChannel][%s][%s:%d] "
                   "Assertion (%s) failed.  %s %s\n",
                   mSide == ChildSide ? "Child" : "Parent",
                   file, line, cond,
                   why,
                   reply ? "(reply)" : "");
     // technically we need the mutex for this, but we're dying anyway
@@ -2249,17 +2251,17 @@ MessageChannel::DebugAbort(const char* f
                   mRemoteStackDepthGuess);
     printf_stderr("  deferred stack size: %" PRIuSIZE "\n",
                   mDeferred.size());
     printf_stderr("  out-of-turn Interrupt replies stack size: %" PRIuSIZE "\n",
                   mOutOfTurnReplies.size());
     printf_stderr("  Pending queue size: %" PRIuSIZE ", front to back:\n",
                   mPending.size());
 
-    MessageQueue pending = mPending;
+    MessageQueue pending = Move(mPending);
     while (!pending.empty()) {
         printf_stderr("    [ %s%s ]\n",
                       pending.front().is_interrupt() ? "intr" :
                       (pending.front().is_sync() ? "sync" : "async"),
                       pending.front().is_reply() ? "reply" : "");
         pending.pop_front();
     }
 
--- a/ipc/glue/MessageChannel.h
+++ b/ipc/glue/MessageChannel.h
@@ -268,35 +268,35 @@ class MessageChannel : HasResultCodes
 
     // Send OnChannelConnected notification to listeners.
     void DispatchOnChannelConnected();
 
     bool InterruptEventOccurred();
     bool HasPendingEvents();
 
     void ProcessPendingRequests(AutoEnterTransaction& aTransaction);
-    bool ProcessPendingRequest(const Message &aUrgent);
+    bool ProcessPendingRequest(Message &&aUrgent);
 
     void MaybeUndeferIncall();
     void EnqueuePendingMessages();
 
     // Executed on the worker thread. Dequeues one pending message.
     bool OnMaybeDequeueOne();
     bool DequeueOne(Message *recvd);
 
     // Dispatches an incoming message to its appropriate handler.
-    void DispatchMessage(const Message &aMsg);
+    void DispatchMessage(Message &&aMsg);
 
     // DispatchMessage will route to one of these functions depending on the
     // protocol type of the message.
     void DispatchSyncMessage(const Message &aMsg, Message*& aReply);
     void DispatchUrgentMessage(const Message &aMsg);
     void DispatchAsyncMessage(const Message &aMsg);
     void DispatchRPCMessage(const Message &aMsg);
-    void DispatchInterruptMessage(const Message &aMsg, size_t aStackDepth);
+    void DispatchInterruptMessage(Message &&aMsg, size_t aStackDepth);
 
     // Return true if the wait ended because a notification was received.
     //
     // Return false if the time elapsed from when we started the process of
     // waiting until afterwards exceeded the currently allotted timeout.
     // That *DOES NOT* mean false => "no event" (== timeout); there are many
     // circumstances that could cause the measured elapsed time to exceed the
     // timeout EVEN WHEN we were notified.
@@ -357,17 +357,17 @@ class MessageChannel : HasResultCodes
     }
 
     MessageListener *Listener() const {
         return mListener.get();
     }
 
     void DebugAbort(const char* file, int line, const char* cond,
                     const char* why,
-                    bool reply=false) const;
+                    bool reply=false);
 
     // This method is only safe to call on the worker thread, or in a
     // debugger with all threads paused.
     void DumpInterruptStack(const char* const pfx="") const;
 
   private:
     // Called from both threads
     size_t InterruptStackDepth() const {