Bug 1128457 - Change handling of urgent messages during timeout (r=dvander)

ipc/glue/MessageChannel.cpp

@@ -313,6 +313,7 @@ MessageChannel::MessageChannel(MessageListener *aListener)
     mDispatchingAsyncMessagePriority(0),
     mCurrentTransaction(0),
     mTimedOutMessageSeqno(0),
+    mTimedOutMessagePriority(0),
     mRecvdErrors(0),
     mRemoteStackDepthGuess(false),
     mSawInterruptOutMsg(false),
@@ -784,10 +785,11 @@ MessageChannel::Send(Message* aMsg, Message* aReply)
     msg->set_seqno(NextSeqno());
 
     int32_t seqno = msg->seqno();
+    int prio = msg->priority();
     DebugOnly<msgid_t> replyType = msg->type() + 1;
 
     AutoSetValue<bool> replies(mAwaitingSyncReply, true);
-    AutoSetValue<int> prio(mAwaitingSyncReplyPriority, msg->priority());
+    AutoSetValue<int> prioSet(mAwaitingSyncReplyPriority, prio);
     AutoEnterTransaction transact(this, seqno);
 
     int32_t transaction = mCurrentTransaction;
@@ -836,6 +838,7 @@ MessageChannel::Send(Message* aMsg, Message* aReply)
             }
 
             mTimedOutMessageSeqno = seqno;
+            mTimedOutMessagePriority = prio;
             return false;
         }
     }
@@ -1189,12 +1192,20 @@ MessageChannel::DispatchSyncMessage(const Message& aMsg)
     bool& blockingVar = ShouldBlockScripts() ? gParentIsBlocked : dummy;
 
     Result rv;
-    if (mTimedOutMessageSeqno) {
+    if (mTimedOutMessageSeqno && mTimedOutMessagePriority >= prio) {
         // If the other side sends a message in response to one of our messages
         // that we've timed out, then we reply with an error.
         //
-        // We even reject messages that were sent before the other side even got
-        // to our timed out message.
+        // We do this because want to avoid a situation where we process an
+        // incoming message from the child here while it simultaneously starts
+        // processing our timed-out CPOW. It's very bad for both sides to
+        // be processing sync messages concurrently.
+        //
+        // The only exception is if the incoming message has urgent priority and
+        // our timed-out message had only high priority. In that case it's safe
+        // 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);

ipc/glue/MessageChannel.h

@@ -592,6 +592,7 @@ class MessageChannel : HasResultCodes
     // hitting a lot of corner cases with message nesting that we don't really
     // care about.
     int32_t mTimedOutMessageSeqno;
+    int mTimedOutMessagePriority;
 
     // If waiting for the reply to a sync out-message, it will be saved here
     // on the I/O thread and then read and cleared by the worker thread.

ipc/ipdl/test/cxx/PTestUrgentHangs.ipdl

@@ -1,12 +1,13 @@
 namespace mozilla {
 namespace _ipdltest {
 
-prio(normal upto high) sync protocol PTestUrgentHangs
+prio(normal upto urgent) sync protocol PTestUrgentHangs
 {
 parent:
     prio(high) sync Test1_2();
 
     prio(high) sync TestInner();
+    prio(urgent) sync TestInnerUrgent();
 
 child:
     prio(high) sync Test1_1();
@@ -18,6 +19,9 @@ child:
 
     async Test4();
     prio(high) sync Test4_1();
+
+    async Test5();
+    prio(high) sync Test5_1();
 };
 
 } // namespace _ipdltest

ipc/ipdl/test/cxx/TestUrgentHangs.cpp

@@ -25,6 +25,8 @@ namespace _ipdltest {
 // parent
 
 TestUrgentHangsParent::TestUrgentHangsParent()
+  : mInnerCount(0),
+    mInnerUrgentCount(0)
 {
     MOZ_COUNT_CTOR(TestUrgentHangsParent);
 }
@@ -55,12 +57,12 @@ TestUrgentHangsParent::Main()
     // Do a second round of testing once the reply to Test2 comes back.
     MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
-        NewRunnableMethod(this, &TestUrgentHangsParent::FinishTesting),
+        NewRunnableMethod(this, &TestUrgentHangsParent::SecondStage),
         3000);
 }
 
 void
-TestUrgentHangsParent::FinishTesting()
+TestUrgentHangsParent::SecondStage()
 {
     // Send an async message that waits 2 seconds and then sends a sync message
     // (which should be processed).
@@ -72,7 +74,30 @@ TestUrgentHangsParent::FinishTesting()
     if (SendTest4_1())
         fail("sending Test4_1");
 
-    // Close the channel after the child finishes its work in RecvTest4.
+    MessageLoop::current()->PostDelayedTask(
+        FROM_HERE,
+        NewRunnableMethod(this, &TestUrgentHangsParent::ThirdStage),
+        3000);
+}
+
+void
+TestUrgentHangsParent::ThirdStage()
+{
+    // The third stage does the same thing as the second stage except that the
+    // child sends an urgent message to us. In this case, we actually answer
+    // that message unconditionally.
+
+    // Send an async message that waits 2 seconds and then sends a sync message
+    // (which should be processed).
+    if (!SendTest5())
+        fail("sending Test5");
+
+    // Send a sync message that will time out because the child is waiting
+    // inside RecvTest5.
+    if (SendTest5_1())
+        fail("sending Test5_1");
+
+    // Close the channel after the child finishes its work in RecvTest5.
     MessageLoop::current()->PostDelayedTask(
         FROM_HERE,
         NewRunnableMethod(this, &TestUrgentHangsParent::Close),
@@ -90,7 +115,14 @@ TestUrgentHangsParent::RecvTest1_2()
 bool
 TestUrgentHangsParent::RecvTestInner()
 {
-    fail("TestInner should never be dispatched");
+    mInnerCount++;
+    return true;
+}
+
+bool
+TestUrgentHangsParent::RecvTestInnerUrgent()
+{
+    mInnerUrgentCount++;
     return true;
 }
 
@@ -157,6 +189,30 @@ TestUrgentHangsChild::RecvTest4_1()
     return true;
 }
 
+bool
+TestUrgentHangsChild::RecvTest5()
+{
+    PR_Sleep(PR_SecondsToInterval(2));
+
+    // This message will actually be handled by the parent even though it's in
+    // the timeout state.
+    if (!SendTestInnerUrgent())
+        fail("sending TestInner");
+
+    return true;
+}
+
+bool
+TestUrgentHangsChild::RecvTest5_1()
+{
+    // This message will actually be handled by the parent even though it's in
+    // the timeout state.
+    if (!SendTestInnerUrgent())
+        fail("sending TestInner");
+
+    return true;
+}
+
 TestUrgentHangsChild::TestUrgentHangsChild()
 {
     MOZ_COUNT_CTOR(TestUrgentHangsChild);

ipc/ipdl/test/cxx/TestUrgentHangs.h

@@ -21,10 +21,12 @@ public:
     static bool RunTestInThreads() { return false; }
 
     void Main();
-    void FinishTesting();
+    void SecondStage();
+    void ThirdStage();
 
     bool RecvTest1_2();
     bool RecvTestInner();
+    bool RecvTestInnerUrgent();
 
     bool ShouldContinueFromReplyTimeout() override
     {
@@ -32,9 +34,18 @@ public:
     }
     virtual void ActorDestroy(ActorDestroyReason why) override
     {
+	if (mInnerCount != 1) {
+	    fail("wrong mInnerCount");
+	}
+	if (mInnerUrgentCount != 2) {
+	    fail("wrong mInnerUrgentCount");
+	}
         passed("ok");
         QuitParent();
     }
+
+private:
+    size_t mInnerCount, mInnerUrgentCount;
 };
 
 
@@ -51,6 +62,8 @@ public:
     bool RecvTest3();
     bool RecvTest4();
     bool RecvTest4_1();
+    bool RecvTest5();
+    bool RecvTest5_1();
 
     virtual void ActorDestroy(ActorDestroyReason why) override
     {