Bug 1355178 - Remove unnecessary refcounting from MessageChannel::mPending (r=froydnj)
authorBill McCloskey <billm@mozilla.com>
Mon, 10 Apr 2017 20:40:00 -0700
changeset 565452 af3fe1a5be65a7b11b17ea4429a34a95d2418124
parent 565451 5c706be68a0742078cb76c03f58a8e9d01de2dec
child 565453 13895acd7c9de2e4539f96dc37219570ce2093a7
push id54862
push userdtownsend@mozilla.com
push dateWed, 19 Apr 2017 21:37:55 +0000
reviewersfroydnj
bugs1355178
milestone55.0a1
Bug 1355178 - Remove unnecessary refcounting from MessageChannel::mPending (r=froydnj) MozReview-Commit-ID: DWEF1ObNmMa
ipc/glue/MessageChannel.cpp
mfbt/LinkedList.h
--- a/ipc/glue/MessageChannel.cpp
+++ b/ipc/glue/MessageChannel.cpp
@@ -679,17 +679,17 @@ MessageChannel::Clear()
     mOnChannelConnectedTask->Cancel();
 
     if (mChannelErrorTask) {
         mChannelErrorTask->Cancel();
         mChannelErrorTask = nullptr;
     }
 
     // Free up any memory used by pending messages.
-    for (RefPtr<MessageTask> task : mPending) {
+    for (MessageTask* task : mPending) {
         task->Clear();
     }
     mPending.clear();
 
     mOutOfTurnReplies.clear();
     while (!mDeferred.empty()) {
         mDeferred.pop();
     }
@@ -1036,17 +1036,17 @@ MessageChannel::OnMessageReceivedFromLin
             // Replace it with the newer message.
             MOZ_RELEASE_ASSERT(mPending.getLast()->Msg().compress_type() ==
                                IPC::Message::COMPRESSION_ENABLED);
             mPending.getLast()->Msg() = Move(aMsg);
 
             reuseTask = true;
         }
     } else if (aMsg.compress_type() == IPC::Message::COMPRESSION_ALL && !mPending.isEmpty()) {
-        for (RefPtr<MessageTask> p = mPending.getLast(); p; p = p->getPrevious()) {
+        for (MessageTask* p = mPending.getLast(); p; p = p->getPrevious()) {
             if (p->Msg().type() == aMsg.type() &&
                 p->Msg().routing_id() == aMsg.routing_id())
             {
                 // This message type has compression enabled, and the queue
                 // holds a message with the same message type and routed to the
                 // same destination. Erase it. Note that, since we always
                 // compress these redundancies, There Can Be Only One.
                 MOZ_RELEASE_ASSERT(p->Msg().compress_type() == IPC::Message::COMPRESSION_ALL);
@@ -1113,17 +1113,17 @@ MessageChannel::OnMessageReceivedFromLin
 }
 
 void
 MessageChannel::PeekMessages(std::function<bool(const Message& aMsg)> aInvoke)
 {
     // FIXME: We shouldn't be holding the lock for aInvoke!
     MonitorAutoLock lock(*mMonitor);
 
-    for (RefPtr<MessageTask> it : mPending) {
+    for (MessageTask* it : mPending) {
         const Message &msg = it->Msg();
         if (!aInvoke(msg)) {
             break;
         }
     }
 }
 
 void
@@ -1142,17 +1142,17 @@ MessageChannel::ProcessPendingRequests(A
         // cause even NOT_NESTED sync messages to be processed (but not
         // NOT_NESTED async messages), which would break message ordering.
         if (aTransaction.IsCanceled()) {
             return;
         }
 
         mozilla::Vector<Message> toProcess;
 
-        for (RefPtr<MessageTask> p = mPending.getFirst(); p; ) {
+        for (MessageTask* p = mPending.getFirst(); p; ) {
             Message &msg = p->Msg();
 
             MOZ_RELEASE_ASSERT(!aTransaction.IsCanceled(),
                                "Calling ShouldDeferMessage when cancelled");
             bool defer = ShouldDeferMessage(msg);
 
             // Only log the interesting messages.
             if (msg.is_sync() || msg.nested_level() == IPC::Message::NESTED_INSIDE_CPOW) {
@@ -1674,17 +1674,17 @@ MessageChannel::RunMessage(MessageTask& 
 
     if (!Connected()) {
         ReportConnectionError("RunMessage");
         return;
     }
 
     // Check that we're going to run the first message that's valid to run.
 #ifdef DEBUG
-    for (RefPtr<MessageTask> task : mPending) {
+    for (MessageTask* task : mPending) {
         if (task == &aTask) {
             break;
         }
 
         MOZ_ASSERT(!ShouldRunMessage(task->Msg()) ||
                    aTask.Msg().priority() != task->Msg().priority());
 
     }
@@ -2593,17 +2593,17 @@ MessageChannel::EndTimeout()
 
     RepostAllMessages();
 }
 
 void
 MessageChannel::RepostAllMessages()
 {
     bool needRepost = false;
-    for (RefPtr<MessageTask> task : mPending) {
+    for (MessageTask* task : mPending) {
         if (!task->IsScheduled()) {
             needRepost = true;
         }
     }
     if (!needRepost) {
         // If everything is already scheduled to run, do nothing.
         return;
     }
@@ -2655,17 +2655,17 @@ MessageChannel::CancelTransaction(int tr
             mTransactionStack->Cancel();
         }
     } else {
         MOZ_RELEASE_ASSERT(mTransactionStack->TransactionID() == transaction);
         mTransactionStack->Cancel();
     }
 
     bool foundSync = false;
-    for (RefPtr<MessageTask> p = mPending.getFirst(); p; ) {
+    for (MessageTask* p = mPending.getFirst(); p; ) {
         Message &msg = p->Msg();
 
         // If there was a race between the parent and the child, then we may
         // have a queued sync message. We want to drop this message from the
         // queue since if will get cancelled along with the transaction being
         // cancelled. This happens if the message in the queue is NESTED_INSIDE_SYNC.
         if (msg.is_sync() && msg.nested_level() != IPC::Message::NOT_NESTED) {
             MOZ_RELEASE_ASSERT(!foundSync);
--- a/mfbt/LinkedList.h
+++ b/mfbt/LinkedList.h
@@ -248,33 +248,35 @@ public:
     mPrev = this;
 
     Traits::exitList(this);
   }
 
   /*
    * Remove this element from the list containing it.  Returns a pointer to the
    * element that follows this element (before it was removed).  This method
-   * asserts if the element does not belong to a list.
+   * asserts if the element does not belong to a list. Note: In a refcounted list,
+   * |this| may be destroyed.
    */
-  ClientType removeAndGetNext()
+  RawType removeAndGetNext()
   {
-    ClientType r = getNext();
+    RawType r = getNext();
     remove();
     return r;
   }
 
   /*
    * Remove this element from the list containing it.  Returns a pointer to the
    * previous element in the containing list (before the removal).  This method
-   * asserts if the element does not belong to a list.
+   * asserts if the element does not belong to a list. Note: In a refcounted list,
+   * |this| may be destroyed.
    */
-  ClientType removeAndGetPrevious()
+  RawType removeAndGetPrevious()
   {
-    ClientType r = getPrevious();
+    RawType r = getPrevious();
     remove();
     return r;
   }
 
   /*
    * Identical to remove(), but also asserts in debug builds that this element
    * is in aList.
    */