bug 533587: process RPC in-calls deferred because of races until "later". in-person r=bent
authorChris Jones <jones.chris.g@gmail.com>
Wed, 09 Dec 2009 17:15:01 -0600
changeset 36171 7efa9bc5cdba64c6b10248e557395290a63728f3
parent 36170 210bdc4eaa46a41ae4af9a9281507a0ce4f5206c
child 36172 f9c0cc34ebe85229eaf87b2003659eac1e91669c
push idunknown
push userunknown
push dateunknown
reviewersbent
bugs533587
milestone1.9.3a1pre
bug 533587: process RPC in-calls deferred because of races until "later". in-person r=bent
ipc/glue/RPCChannel.cpp
ipc/ipdl/ipdl/ast.py
ipc/ipdl/ipdl/type.py
ipc/ipdl/test/cxx/Makefile.in
ipc/ipdl/test/cxx/PTestRPCRaces.ipdl
ipc/ipdl/test/cxx/TestRPCRaces.cpp
ipc/ipdl/test/cxx/TestRPCRaces.h
ipc/ipdl/test/cxx/ipdl.mk
--- a/ipc/glue/RPCChannel.cpp
+++ b/ipc/glue/RPCChannel.cpp
@@ -100,16 +100,20 @@ RPCChannel::Call(Message* msg, Message* 
     msg->set_rpc_remote_stack_depth_guess(mRemoteStackDepthGuess);
     msg->set_rpc_local_stack_depth(StackDepth());
 
     mIOLoop->PostTask(
         FROM_HERE,
         NewRunnableMethod(this, &RPCChannel::OnSend, msg));
 
     while (1) {
+        // now might be the time to process a message deferred because
+        // of race resolution
+        MaybeProcessDeferredIncall();
+
         // here we're waiting for something to happen. see long
         // comment about the queue in RPCChannel.h
         while (Connected() && mPending.empty()) {
             WaitForNotify();
         }
 
         if (!Connected()) {
             ReportConnectionError("RPCChannel");
@@ -149,21 +153,16 @@ RPCChannel::Call(Message* msg, Message* 
             // call.  pop this frame and return the reply
             mStack.pop();
 
             bool isError = recvd.is_reply_error();
             if (!isError) {
                 *reply = recvd;
             }
 
-            // the stack depth just shrunk, so now might be the time
-            // to process a message deferred because of race
-            // resolution
-            MaybeProcessDeferredIncall();
-
             if (0 == StackDepth())
                 // we may have received new messages while waiting for
                 // our reply.  because we were awaiting a reply,
                 // StackDepth > 0, and the IO thread didn't enqueue
                 // OnMaybeDequeueOne() events for us.  so to avoid
                 // "losing" the new messages, we do that now.
                 EnqueuePendingMessages();
 
@@ -215,41 +214,53 @@ RPCChannel::MaybeProcessDeferredIncall()
     MutexAutoUnlock unlock(mMutex);
     fprintf(stderr, "  (processing deferred in-call)\n");
     Incall(call, stackDepth);
 }
 
 void
 RPCChannel::EnqueuePendingMessages()
 {
+    AssertWorkerThread();
+    mMutex.AssertCurrentThreadOwns();
+    RPC_ASSERT(mDeferred.empty() || 1 == mDeferred.size(),
+               "expected mDeferred to have 0 or 1 items");
+
+    if (!mDeferred.empty())
+        mWorkerLoop->PostTask(
+            FROM_HERE,
+            NewRunnableMethod(this, &RPCChannel::OnMaybeDequeueOne));
+
     // XXX performance tuning knob: could process all or k pending
     // messages here, rather than enqueuing for later processing
 
-    AssertWorkerThread();
-    mMutex.AssertCurrentThreadOwns();
-
     for (size_t i = 0; i < mPending.size(); ++i)
         mWorkerLoop->PostTask(
             FROM_HERE,
             NewRunnableMethod(this, &RPCChannel::OnMaybeDequeueOne));
 }
 
 void
 RPCChannel::OnMaybeDequeueOne()
 {
     // XXX performance tuning knob: could process all or k pending
     // messages here
 
     AssertWorkerThread();
     mMutex.AssertNotCurrentThreadOwns();
+    RPC_ASSERT(mDeferred.empty() || 1 == mDeferred.size(),
+               "expected mDeferred to have 0 or 1 items, but it has %lu");
 
     Message recvd;
     {
         MutexAutoLock lock(mMutex);
 
+        if (!mDeferred.empty())
+            return MaybeProcessDeferredIncall();
+
         if (mPending.empty())
             return;
 
         recvd = mPending.front();
         mPending.pop();
     }
 
     if (recvd.is_rpc())
--- a/ipc/ipdl/ipdl/ast.py
+++ b/ipc/ipdl/ipdl/ast.py
@@ -298,16 +298,25 @@ class MessageDecl(Node):
 
 class Transition(Node):
     def __init__(self, loc, trigger, msg, toStates):
         Node.__init__(self, loc)
         self.trigger = trigger
         self.msg = msg
         self.toStates = toStates
 
+    def __cmp__(self, o):
+        c = cmp(self.msg, o.msg)
+        if c: return c
+        c = cmp(self.trigger, o.trigger)
+        if c: return c
+
+    def __hash__(self): return hash(str(self))
+    def __str__(self): return '%s %s'% (self.trigger, self.msg)
+
     @staticmethod
     def nameToTrigger(name):
         return { 'send': SEND, 'recv': RECV, 'call': CALL, 'answer': ANSWER }[name]
 
 Transition.NULL = Transition(Loc.NONE, None, None, [ ])
 
 class TransitionStmt(Node):
     def __init__(self, loc, state, transitions):
--- a/ipc/ipdl/ipdl/type.py
+++ b/ipc/ipdl/ipdl/type.py
@@ -847,22 +847,22 @@ class GatherDecls(TcheckVisitor):
         self.seentriggers = set()
         TcheckVisitor.visitTransitionStmt(self, ts)
 
     def visitTransition(self, t):
         loc = t.loc
 
         # check the trigger message
         mname = t.msg
-        if mname in self.seentriggers:
-            self.error(loc, "trigger `%s' appears multiple times", mname)
-        self.seentriggers.add(mname)
-        
+        if t in self.seentriggers:
+            self.error(loc, "trigger `%s' appears multiple times", t.msg)
+        self.seentriggers.add(t)
+
         mdecl = self.symtab.lookup(mname)
-        if mdecl.type.isIPDL() and mdecl.type.isProtocol():
+        if mdecl is not None and mdecl.type.isIPDL() and mdecl.type.isProtocol():
             mdecl = self.symtab.lookup(mname +'Constructor')
         
         if mdecl is None:
             self.error(loc, "message `%s' has not been declared", mname)
         elif not mdecl.type.isMessage():
             self.error(
                 loc,
                 "`%s' should have message type, but instead has type `%s'",
--- a/ipc/ipdl/test/cxx/Makefile.in
+++ b/ipc/ipdl/test/cxx/Makefile.in
@@ -56,16 +56,17 @@ LIBRARY_NAME = $(MODULE)_s
 LIBXUL_LIBRARY = 1
 FORCE_STATIC_LIB = 1
 EXPORT_LIBRARY = 1
 
 # Please keep these organized in the order "easy"-to-"hard"
 IPDLTESTS = \
   TestSanity  \
   TestLatency \
+  TestRPCRaces \
   TestManyChildAllocs  \
   TestDesc \
   TestShmem \
   TestShutdown \
   TestArrays \
   $(NULL)
 
 IPDLTESTSRCS = $(addsuffix .cpp,$(IPDLTESTS))
new file mode 100644
--- /dev/null
+++ b/ipc/ipdl/test/cxx/PTestRPCRaces.ipdl
@@ -0,0 +1,49 @@
+namespace mozilla {
+namespace _ipdltest {
+
+rpc protocol PTestRPCRaces {
+both:
+    rpc Race() returns (bool hasReply);
+    rpc StackFrame() returns ();
+parent:
+    sync StartRace();
+child:
+    Start();
+    Wakeup();
+    __delete__();
+
+state START:
+    send Start goto TEST1;
+
+// First test: race while no other messages are on the RPC stack
+state TEST1:
+    recv StartRace goto RACE1;
+state RACE1:
+    call Race goto DUMMY1_1;
+    answer Race goto DUMMY1_2;
+state DUMMY1_1:
+    answer Race goto TEST2;
+state DUMMY1_2:
+    call Race goto TEST2;
+
+// Second test: race while other messages are on the RPC stack
+state TEST2:
+    call StackFrame goto MORESTACK;
+state MORESTACK:
+    answer StackFrame goto STARTRACE;
+state STARTRACE:
+    send Wakeup goto RACE2;
+state RACE2:
+    call Race goto DUMMY2_1;
+    answer Race goto DUMMY2_2;
+state DUMMY2_1:
+    answer Race goto DYING;
+state DUMMY2_2:
+    call Race goto DYING;
+
+state DYING:
+    send __delete__;
+};
+
+} // namespace _ipdltest
+} // namespace mozilla
new file mode 100644
--- /dev/null
+++ b/ipc/ipdl/test/cxx/TestRPCRaces.cpp
@@ -0,0 +1,148 @@
+#include "TestRPCRaces.h"
+
+#include "IPDLUnitTests.h"      // fail etc.
+
+
+template<>
+struct RunnableMethodTraits<mozilla::_ipdltest::TestRPCRacesParent>
+{
+    static void RetainCallee(mozilla::_ipdltest::TestRPCRacesParent* obj) { }
+    static void ReleaseCallee(mozilla::_ipdltest::TestRPCRacesParent* obj) { }
+};
+
+
+namespace mozilla {
+namespace _ipdltest {
+
+//-----------------------------------------------------------------------------
+// parent
+void
+TestRPCRacesParent::Main()
+{
+    SendStart();
+}
+
+bool
+TestRPCRacesParent::RecvStartRace()
+{
+    MessageLoop::current()->PostTask(
+        FROM_HERE,
+        NewRunnableMethod(this, &TestRPCRacesParent::OnRaceTime));
+    return true;
+}
+
+void
+TestRPCRacesParent::OnRaceTime()
+{
+    if (!CallRace(&mChildHasReply))
+        fail("problem calling Race()");
+
+    if (!mChildHasReply)
+        fail("child should have got a reply already");
+
+    mHasReply = true;
+
+    MessageLoop::current()->PostTask(
+        FROM_HERE,
+        NewRunnableMethod(this, &TestRPCRacesParent::Test2));
+}
+
+bool
+TestRPCRacesParent::AnswerRace(bool* hasReply)
+{
+    if (mHasReply)
+        fail("apparently the parent won the RPC race!");
+    *hasReply = hasReply;
+    return true;
+}
+
+void
+TestRPCRacesParent::Test2()
+{
+    puts("  passed");
+    puts("Test 2");
+
+    mHasReply = false;
+    mChildHasReply = false;
+
+    if (!CallStackFrame())
+        fail("can't set up a stack frame");
+
+    puts("  passed");
+
+    Close();
+}
+
+bool
+TestRPCRacesParent::AnswerStackFrame()
+{
+    if (!SendWakeup())
+        fail("can't wake up the child");
+
+    if (!CallRace(&mChildHasReply))
+        fail("can't set up race condition");
+    mHasReply = true;
+
+    if (!mChildHasReply)
+        fail("child should have got a reply already");
+
+    return true;
+}
+
+//-----------------------------------------------------------------------------
+// child
+bool
+TestRPCRacesChild::RecvStart()
+{
+    puts("Test 1");
+
+    if (!SendStartRace())
+        fail("problem sending StartRace()");
+
+    bool dontcare;
+    if (!CallRace(&dontcare))
+        fail("problem calling Race()");
+
+    mHasReply = true;
+    return true;
+}
+
+bool
+TestRPCRacesChild::AnswerRace(bool* hasReply)
+{
+    if (!mHasReply)
+        fail("apparently the child lost the RPC race!");
+
+    *hasReply = mHasReply;
+
+    return true;
+}
+
+bool
+TestRPCRacesChild::AnswerStackFrame()
+{
+    // reset for the second test
+    mHasReply = false;
+
+    if (!CallStackFrame())
+        fail("can't set up stack frame");
+
+    if (!mHasReply)
+        fail("should have had reply by now");
+
+    return true;
+}
+
+bool
+TestRPCRacesChild::RecvWakeup()
+{
+    bool dontcare;
+    if (!CallRace(&dontcare))
+        fail("can't set up race condition");
+
+    mHasReply = true;
+    return true;
+}
+
+} // namespace _ipdltest
+} // namespace mozilla
new file mode 100644
--- /dev/null
+++ b/ipc/ipdl/test/cxx/TestRPCRaces.h
@@ -0,0 +1,98 @@
+#ifndef mozilla__ipdltest_TestRPCRaces_h
+#define mozilla__ipdltest_TestRPCRaces_h
+
+#include "mozilla/_ipdltest/IPDLUnitTests.h"
+
+#include "mozilla/_ipdltest/PTestRPCRacesParent.h"
+#include "mozilla/_ipdltest/PTestRPCRacesChild.h"
+
+namespace mozilla {
+namespace _ipdltest {
+
+
+class TestRPCRacesParent :
+    public PTestRPCRacesParent
+{
+public:
+    TestRPCRacesParent() : mHasReply(false), mChildHasReply(false)
+    { }
+    virtual ~TestRPCRacesParent() { }
+
+    void Main();
+
+protected:
+    NS_OVERRIDE
+    virtual bool
+    RecvStartRace();
+
+    NS_OVERRIDE
+    virtual bool
+    AnswerRace(bool* hasRace);
+
+    NS_OVERRIDE
+    virtual bool
+    AnswerStackFrame();
+
+    NS_OVERRIDE
+    virtual void ActorDestroy(ActorDestroyReason why)
+    {
+        if (NormalShutdown != why)
+            fail("unexpected destruction!");
+        if (!(mHasReply && mChildHasReply))
+            fail("both sides should have replies!");
+        passed("ok");
+        QuitParent();
+    }
+
+private:
+    void OnRaceTime();
+
+    void Test2();
+
+    bool mHasReply;
+    bool mChildHasReply;
+};
+
+
+class TestRPCRacesChild :
+    public PTestRPCRacesChild
+{
+public:
+    TestRPCRacesChild() : mHasReply(false) { }
+    virtual ~TestRPCRacesChild() { }
+
+protected:
+    NS_OVERRIDE
+    virtual bool
+    RecvStart();
+
+    NS_OVERRIDE
+    virtual bool
+    AnswerRace(bool* hasRace);
+
+    NS_OVERRIDE
+    virtual bool
+    AnswerStackFrame();
+
+    NS_OVERRIDE
+    virtual bool
+    RecvWakeup();
+
+    NS_OVERRIDE
+    virtual void ActorDestroy(ActorDestroyReason why)
+    {
+        if (NormalShutdown != why)
+            fail("unexpected destruction!");
+        QuitChild();
+    }
+
+private:
+    bool mHasReply;
+};
+
+
+} // namespace _ipdltest
+} // namespace mozilla
+
+
+#endif // ifndef mozilla__ipdltest_TestRPCRaces_h
--- a/ipc/ipdl/test/cxx/ipdl.mk
+++ b/ipc/ipdl/test/cxx/ipdl.mk
@@ -2,14 +2,15 @@ IPDLSRCS =					\
   PTestArrays.ipdl				\
   PTestArraysSub.ipdl				\
   PTestDesc.ipdl				\
   PTestDescSub.ipdl				\
   PTestDescSubsub.ipdl				\
   PTestLatency.ipdl				\
   PTestManyChildAllocs.ipdl			\
   PTestManyChildAllocsSub.ipdl			\
+  PTestRPCRaces.ipdl				\
   PTestSanity.ipdl				\
   PTestShmem.ipdl				\
   PTestShutdown.ipdl				\
   PTestShutdownSub.ipdl				\
   PTestShutdownSubsub.ipdl			\
   $(NULL)