Bug 1177013 - Change IPC locking to get transaction ID correct. r=dvander, a=ritu
authorBill McCloskey <billm@mozilla.com>
Tue, 30 Jun 2015 14:48:29 -0700
changeset 281675 32692b459f3776088051fe8fadd38917e4ad9f8f
parent 281674 347298a170d45c3d5254aab4ea55d35ebaefa329
child 281676 66de4a3517efe7993d83696a899ed1c6f96639bf
push id4932
push userjlund@mozilla.com
push dateMon, 10 Aug 2015 18:23:06 +0000
treeherdermozilla-beta@6dd5a4f5f745 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdvander, ritu
bugs1177013
milestone41.0a2
Bug 1177013 - Change IPC locking to get transaction ID correct. r=dvander, a=ritu
ipc/glue/MessageChannel.cpp
ipc/glue/MessageChannel.h
--- a/ipc/glue/MessageChannel.cpp
+++ b/ipc/glue/MessageChannel.cpp
@@ -969,22 +969,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()) {
-            {
-                AutoEnterTransaction transaction(this, recvd);
-                MonitorAutoUnlock unlock(*mMonitor);
-                CxxStackFrame frame(*this, IN_MESSAGE, &recvd);
-                DispatchMessage(recvd);
-            }
+            DispatchMessage(recvd);
             if (!Connected()) {
                 ReportConnectionError("MessageChannel::DispatchMessage");
                 return false;
             }
             continue;
         }
 
         // If the message is an Interrupt reply, either process it as a reply to our
@@ -1102,24 +1097,17 @@ MessageChannel::ProcessPendingRequest(co
     // therefore mPendingUrgentRequest is set *and* mRecvd is set as
     // well, because the link thread received both before the worker
     // thread woke up.
     //
     // In this case, we process the urgent message first, but we need
     // 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;
     }
 
     // In between having dispatched our reply to the parent process, and
     // re-acquiring the monitor, the parent process could have already
     // processed that reply and sent the reply to our sync message. If so,
@@ -1166,50 +1154,55 @@ 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;
     }
 
-    {
-        // 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;
 }
 
 void
 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
     // we avoid running event handlers. Once we've sent the response to the
     // urgent message, it's okay to run event handlers again since the parent is
     // no longer blocked.
     MOZ_ASSERT_IF(prio > IPC::Message::PRIORITY_NORMAL, NS_IsMainThread());
     MaybeScriptBlocker scriptBlocker(this, prio > IPC::Message::PRIORITY_NORMAL);
@@ -1237,33 +1230,28 @@ MessageChannel::DispatchSyncMessage(cons
         // to process the incoming message because we know that the child won't
         // process anything (the child will defer incoming messages when waiting
         // for a response to its urgent message).
         rv = MsgNotAllowed;
     } else {
         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();
+        aReply = new Message();
+        aReply->set_sync();
+        aReply->set_priority(aMsg.priority());
+        aReply->set_reply();
+        aReply->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->set_seqno(aMsg.seqno());
+    aReply->set_transaction_id(aMsg.transaction_id());
 }
 
 void
 MessageChannel::DispatchAsyncMessage(const Message& aMsg)
 {
     AssertWorkerThread();
     MOZ_ASSERT(!aMsg.is_interrupt() && !aMsg.is_sync());
 
--- a/ipc/glue/MessageChannel.h
+++ b/ipc/glue/MessageChannel.h
@@ -237,17 +237,17 @@ class MessageChannel : HasResultCodes
     bool OnMaybeDequeueOne();
     bool DequeueOne(Message *recvd);
 
     // Dispatches an incoming message to its appropriate handler.
     void DispatchMessage(const Message &aMsg);
 
     // 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);
     void DispatchInterruptMessage(const 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