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 338361 ea8c597133e5d952dc92bac75c69b4620c37178b
parent 338360 d9e6ae164b6e06fff75872d02531ad1928038c0e
child 338362 64fcfbf2da25d2563d53fcb518fa355eb72cb2c5
push id6249
push userjlund@mozilla.com
push dateMon, 01 Aug 2016 13:59:36 +0000
treeherdermozilla-beta@bad9d4f5bf7e [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1273307
milestone49.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 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 {