From efe9ba545ea84ebb9f0b826235b330680df4e5bd Mon Sep 17 00:00:00 2001
From: Bill McCloskey <billm@mozilla.com>
Date: Tue, 30 Jun 2015 14:48:29 -0700
Subject: [PATCH] Bug 1177013 - Change IPC locking to get transaction ID
 correct (r=dvander)

---
 ipc/glue/MessageChannel.cpp | 78 ++++++++++++++++---------------------
 ipc/glue/MessageChannel.h   |  2 +-
 2 files changed, 34 insertions(+), 46 deletions(-)

diff --git a/ipc/glue/MessageChannel.cpp b/ipc/glue/MessageChannel.cpp
index 74cae198da4..812858b277e 100644
--- a/ipc/glue/MessageChannel.cpp
+++ b/ipc/glue/MessageChannel.cpp
@@ -974,12 +974,7 @@ MessageChannel::Call(Message* aMsg, Message* aReply)
 
         // If the message is not Interrupt, we can dispatch it as normal.
         if (!recvd.is_interrupt()) {
-            {
-                AutoEnterTransaction transaction(this, recvd);
-                MonitorAutoUnlock unlock(*mMonitor);
-                CxxStackFrame frame(*this, IN_MESSAGE, &recvd);
-                DispatchMessage(recvd);
-            }
+            DispatchMessage(recvd);
             if (!Connected()) {
                 ReportConnectionError("MessageChannel::DispatchMessage");
                 return false;
@@ -1107,14 +1102,7 @@ MessageChannel::ProcessPendingRequest(const Message &aUrgent)
     // to save the reply.
     nsAutoPtr<Message> savedReply(mRecvd.forget());
 
-    {
-        // In order to send the parent RPC messages and guarantee it will
-        // wake up, we must re-use its transaction.
-        AutoEnterTransaction transaction(this, aUrgent);
-
-        MonitorAutoUnlock unlock(*mMonitor);
-        DispatchMessage(aUrgent);
-    }
+    DispatchMessage(aUrgent);
     if (!Connected()) {
         ReportConnectionError("MessageChannel::ProcessPendingRequest");
         return false;
@@ -1171,16 +1159,10 @@ MessageChannel::OnMaybeDequeueOne()
         return false;
     }
 
-    {
-        // We should not be in a transaction yet if we're not blocked.
-        MOZ_ASSERT(mCurrentTransaction == 0);
-        AutoEnterTransaction transaction(this, recvd);
+    // We should not be in a transaction yet if we're not blocked.
+    MOZ_ASSERT(mCurrentTransaction == 0);
+    DispatchMessage(recvd);
 
-        MonitorAutoUnlock unlock(*mMonitor);
-
-        CxxStackFrame frame(*this, IN_MESSAGE, &recvd);
-        DispatchMessage(recvd);
-    }
     return true;
 }
 
@@ -1190,21 +1172,32 @@ MessageChannel::DispatchMessage(const Message &aMsg)
     Maybe<AutoNoJSAPI> nojsapi;
     if (ScriptSettingsInitialized() && NS_IsMainThread())
         nojsapi.emplace();
-    if (aMsg.is_sync())
-        DispatchSyncMessage(aMsg);
-    else if (aMsg.is_interrupt())
-        DispatchInterruptMessage(aMsg, 0);
-    else
-        DispatchAsyncMessage(aMsg);
+
+    nsAutoPtr<Message> reply;
+
+    {
+        AutoEnterTransaction transaction(this, aMsg);
+        MonitorAutoUnlock unlock(*mMonitor);
+        CxxStackFrame frame(*this, IN_MESSAGE, &aMsg);
+
+        if (aMsg.is_sync())
+            DispatchSyncMessage(aMsg, *getter_Transfers(reply));
+        else if (aMsg.is_interrupt())
+            DispatchInterruptMessage(aMsg, 0);
+        else
+            DispatchAsyncMessage(aMsg);
+    }
+
+    if (reply && ChannelConnected == mChannelState) {
+        mLink->SendMessage(reply.forget());
+    }
 }
 
 void
-MessageChannel::DispatchSyncMessage(const Message& aMsg)
+MessageChannel::DispatchSyncMessage(const Message& aMsg, Message*& aReply)
 {
     AssertWorkerThread();
 
-    nsAutoPtr<Message> reply;
-
     int prio = aMsg.priority();
 
     // We don't want to run any code that might run a nested event loop here, so
@@ -1242,23 +1235,18 @@ MessageChannel::DispatchSyncMessage(const Message& aMsg)
         AutoSetValue<bool> blocked(blockingVar, true);
         AutoSetValue<bool> sync(mDispatchingSyncMessage, true);
         AutoSetValue<int> prioSet(mDispatchingSyncMessagePriority, prio);
-        rv = mListener->OnMessageReceived(aMsg, *getter_Transfers(reply));
+        rv = mListener->OnMessageReceived(aMsg, aReply);
     }
 
     if (!MaybeHandleError(rv, aMsg, "DispatchSyncMessage")) {
-        reply = new Message();
-        reply->set_sync();
-        reply->set_priority(aMsg.priority());
-        reply->set_reply();
-        reply->set_reply_error();
-    }
-    reply->set_seqno(aMsg.seqno());
-    reply->set_transaction_id(aMsg.transaction_id());
-
-    MonitorAutoLock lock(*mMonitor);
-    if (ChannelConnected == mChannelState) {
-        mLink->SendMessage(reply.forget());
+        aReply = new Message();
+        aReply->set_sync();
+        aReply->set_priority(aMsg.priority());
+        aReply->set_reply();
+        aReply->set_reply_error();
     }
+    aReply->set_seqno(aMsg.seqno());
+    aReply->set_transaction_id(aMsg.transaction_id());
 }
 
 void
diff --git a/ipc/glue/MessageChannel.h b/ipc/glue/MessageChannel.h
index dff329add02..a36ab9a6d38 100644
--- a/ipc/glue/MessageChannel.h
+++ b/ipc/glue/MessageChannel.h
@@ -242,7 +242,7 @@ class MessageChannel : HasResultCodes
 
     // DispatchMessage will route to one of these functions depending on the
     // protocol type of the message.
-    void DispatchSyncMessage(const Message &aMsg);
+    void DispatchSyncMessage(const Message &aMsg, Message*& aReply);
     void DispatchUrgentMessage(const Message &aMsg);
     void DispatchAsyncMessage(const Message &aMsg);
     void DispatchRPCMessage(const Message &aMsg);