Bug 1177013 - Change IPC locking to get transaction ID correct (r=dvander)
☠☠ backed out by 0d06cc55aed1 ☠ ☠
authorBill McCloskey <billm@mozilla.com>
Tue, 30 Jun 2015 14:48:29 -0700
changeset 251274 bff9f07dad5259c482ae8fa15ac4b977612cb99d
parent 251273 50702da2194ad31a0c4b4a8957fc66889c478a56
child 251275 d0f5a347465905056cd9c9d670dd80a8d84ad396
push id28991
push usercbook@mozilla.com
push dateFri, 03 Jul 2015 10:08:14 +0000
treeherdermozilla-central@b6a79816ee71 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersdvander
bugs1177013
milestone42.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 1177013 - Change IPC locking to get transaction ID correct (r=dvander)
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