Bug 1334097 - Avoid busy waiting caused by MaybeUndeferIncall. r=aklotz, a=jcristau
authorBill McCloskey <wmccloskey@mozilla.com>
Fri, 28 Apr 2017 14:41:31 -0700
changeset 641665 fe2a2c7e88cb59f4792ca08f86dccea6508e8730
parent 641664 8191e403fedf0227686b1ab14a4b2fc6372c8ee4
child 641666 8d7dbe5c6587414776d71cced925d25943a8d5a5
push id72608
push userbmo:hskupin@gmail.com
push dateMon, 07 Aug 2017 13:41:29 +0000
reviewersaklotz, jcristau
bugs1334097
milestone52.1.2
Bug 1334097 - Avoid busy waiting caused by MaybeUndeferIncall. r=aklotz, a=jcristau In order to avoid a busy wait where we defer and then immediately un-defer a message, we need to ensure that we only un-defer a message if it's actually ready to be processed. This patch uses the same condition in MaybeUndeferIncall as we use in DispatchInterruptMessage.
ipc/glue/MessageChannel.cpp
ipc/glue/MessageChannel.h
--- a/ipc/glue/MessageChannel.cpp
+++ b/ipc/glue/MessageChannel.cpp
@@ -1740,74 +1740,80 @@ MessageChannel::DispatchAsyncMessage(con
         int nestedLevel = aMsg.nested_level();
         AutoSetValue<bool> async(mDispatchingAsyncMessage, true);
         AutoSetValue<int> nestedLevelSet(mDispatchingAsyncMessageNestedLevel, nestedLevel);
         rv = mListener->OnMessageReceived(aMsg);
     }
     MaybeHandleError(rv, aMsg, "DispatchAsyncMessage");
 }
 
+bool
+MessageChannel::ShouldDeferInterruptMessage(const Message& aMsg, size_t aStackDepth)
+{
+    AssertWorkerThread();
+
+    // We may or may not own the lock in this function, so don't access any
+    // channel state.
+
+    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
+    // the other side.
+    if (aMsg.interrupt_remote_stack_depth_guess() == RemoteViewOfStackDepth(aStackDepth)) {
+        return false;
+    }
+
+    // Interrupt in-calls have raced. The winner, if there is one, gets to defer
+    // processing of the other side's in-call.
+    bool defer;
+    const char* winner;
+    const MessageInfo parentMsgInfo =
+        (mSide == ChildSide) ? MessageInfo(aMsg) : mInterruptStack.top();
+    const MessageInfo childMsgInfo =
+        (mSide == ChildSide) ? mInterruptStack.top() : MessageInfo(aMsg);
+    switch (mListener->MediateInterruptRace(parentMsgInfo, childMsgInfo))
+    {
+        case RIPChildWins:
+            winner = "child";
+            defer = (mSide == ChildSide);
+            break;
+        case RIPParentWins:
+            winner = "parent";
+            defer = (mSide != ChildSide);
+            break;
+        case RIPError:
+            MOZ_CRASH("NYI: 'Error' Interrupt race policy");
+        default:
+            MOZ_CRASH("not reached");
+    }
+
+    return defer;
+}
+
 void
 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
-    // the other side.
-    if (aMsg.interrupt_remote_stack_depth_guess() != RemoteViewOfStackDepth(stackDepth)) {
-        // Interrupt in-calls have raced. The winner, if there is one, gets to defer
-        // processing of the other side's in-call.
-        bool defer;
-        const char* winner;
-        const MessageInfo parentMsgInfo =
-          (mSide == ChildSide) ? MessageInfo(aMsg) : mInterruptStack.top();
-        const MessageInfo childMsgInfo =
-          (mSide == ChildSide) ? mInterruptStack.top() : MessageInfo(aMsg);
-        switch (mListener->MediateInterruptRace(parentMsgInfo, childMsgInfo))
-        {
-          case RIPChildWins:
-            winner = "child";
-            defer = (mSide == ChildSide);
-            break;
-          case RIPParentWins:
-            winner = "parent";
-            defer = (mSide != ChildSide);
-            break;
-          case RIPError:
-            NS_RUNTIMEABORT("NYI: 'Error' Interrupt race policy");
-            return;
-          default:
-            NS_RUNTIMEABORT("not reached");
-            return;
-        }
-
-        if (LoggingEnabled()) {
-            printf_stderr("  (%s: %s won, so we're%sdeferring)\n",
-                          (mSide == ChildSide) ? "child" : "parent",
-                          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(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.
+    if (ShouldDeferInterruptMessage(aMsg, stackDepth)) {
+        // We now know the other side's stack has one more frame
+        // than we thought.
+        ++mRemoteStackDepthGuess; // decremented in MaybeProcessDeferred()
+        mDeferred.push(Move(aMsg));
+        return;
     }
 
+    // If we "lost" a race and need to process the other side's in-call, we
+    // don't need to fix up the mRemoteStackDepthGuess here, because we're just
+    // about to increment it, which will make it correct again.
+
 #ifdef OS_WIN
     SyncStackFrame frame(this, true);
 #endif
 
     nsAutoPtr<Message> reply;
 
     ++mRemoteStackDepthGuess;
     Result rv = mListener->OnCallReceived(aMsg, *getter_Transfers(reply));
@@ -1833,22 +1839,28 @@ MessageChannel::MaybeUndeferIncall()
     AssertWorkerThread();
     mMonitor->AssertCurrentThreadOwns();
 
     if (mDeferred.empty())
         return;
 
     size_t stackDepth = InterruptStackDepth();
 
+    Message& deferred = mDeferred.top();
+
     // the other side can only *under*-estimate our actual stack depth
-    IPC_ASSERT(mDeferred.top().interrupt_remote_stack_depth_guess() <= stackDepth,
+    IPC_ASSERT(deferred.interrupt_remote_stack_depth_guess() <= stackDepth,
                "fatal logic error");
 
+    if (ShouldDeferInterruptMessage(deferred, stackDepth)) {
+        return;
+    }
+
     // maybe time to process this message
-    Message call(Move(mDeferred.top()));
+    Message call(Move(deferred));
     mDeferred.pop();
 
     // fix up fudge factor we added to account for race
     IPC_ASSERT(0 < mRemoteStackDepthGuess, "fatal logic error");
     --mRemoteStackDepthGuess;
 
     MOZ_RELEASE_ASSERT(call.nested_level() == IPC::Message::NOT_NESTED);
     RefPtr<MessageTask> task = new MessageTask(this, Move(call));
--- a/ipc/glue/MessageChannel.h
+++ b/ipc/glue/MessageChannel.h
@@ -418,16 +418,17 @@ class MessageChannel : HasResultCodes
 
     void OnChannelConnected(int32_t peer_id);
 
     // Tell the IO thread to close the channel and wait for it to ACK.
     void SynchronouslyClose();
 
     bool WasTransactionCanceled(int transaction);
     bool ShouldDeferMessage(const Message& aMsg);
+    bool ShouldDeferInterruptMessage(const Message& aMsg, size_t aStackDepth);
     void OnMessageReceivedFromLink(Message&& aMsg);
     void OnChannelErrorFromLink();
 
   private:
     // Run on the not current thread.
     void NotifyChannelClosed();
     void NotifyMaybeChannelError();