Bug 767775 - Viciously and loudly kill any process sending messages that race with RPC __delete__ messages. Test by bsmedberg. r=cjones

ipc/ipdl/ipdl/ast.py

@@ -400,6 +400,7 @@ class State(Node):
 
 State.ANY = State(Loc.NONE, '[any]', start=True)
 State.DEAD = State(Loc.NONE, '[dead]', start=False)
+State.DYING = State(Loc.NONE, '[dying]', start=False)
 
 class Param(Node):
     def __init__(self, loc, typespec, name):

ipc/ipdl/ipdl/lower.py

@@ -147,6 +147,11 @@ def _deadState(proto=None):
     if proto is not None:  pfx = proto.name() +'::'
     return ExprVar(pfx +'__Dead')
 
+def _dyingState(proto=None):
+    pfx = ''
+    if proto is not None:  pfx = proto.name() +'::'
+    return ExprVar(pfx +'__Dying')
+
 def _startState(proto=None, fq=False):
     pfx = ''
     if proto:
@@ -157,6 +162,9 @@ def _startState(proto=None, fq=False):
 def _deleteId():
     return ExprVar('Msg___delete____ID')
 
+def _deleteReplyId():
+    return ExprVar('Reply___delete____ID')
+
 def _lookupListener(idexpr):
     return ExprCall(ExprVar('Lookup'), args=[ idexpr ])
 
@@ -1494,6 +1502,7 @@ class _GenerateProtocolCode(ipdl.ast.Visitor):
         stateenum.addId(_deadState().name)
         stateenum.addId(_nullState().name)
         stateenum.addId(_errorState().name)
+        stateenum.addId(_dyingState().name)
         for ts in p.transitionStmts:
             stateenum.addId(ts.state.decl.cxxname)
         if len(p.transitionStmts):
@@ -1658,8 +1667,12 @@ class _GenerateProtocolCode(ipdl.ast.Visitor):
         nullerrorblock = Block()
         if ptype.hasDelete:
             ifdelete = StmtIf(ExprBinary(_deleteId(), '==', msgexpr))
+            if ptype.hasReentrantDelete:
+                nextState = _dyingState()
+            else:
+                nextState = _deadState()
             ifdelete.addifstmts([
-                StmtExpr(ExprAssn(ExprDeref(nextvar), _deadState())),
+                StmtExpr(ExprAssn(ExprDeref(nextvar), nextState)),
                 StmtReturn(ExprLiteral.TRUE) ])
             nullerrorblock.addstmt(ifdelete)
         nullerrorblock.addstmt(
@@ -1674,6 +1687,21 @@ class _GenerateProtocolCode(ipdl.ast.Visitor):
             StmtReturn(ExprLiteral.FALSE) ])
         fromswitch.addcase(CaseLabel(_deadState().name), deadblock)
 
+        # special case for Dying
+        dyingblock = Block()
+        if ptype.hasReentrantDelete:
+            ifdelete = StmtIf(ExprBinary(_deleteReplyId(), '==', msgexpr))
+            ifdelete.addifstmt(
+                StmtExpr(ExprAssn(ExprDeref(nextvar), _deadState())))
+            dyingblock.addstmt(ifdelete)
+            dyingblock.addstmt(
+                StmtReturn(ExprLiteral.TRUE))
+        else:
+            dyingblock.addstmts([
+                _runtimeAbort('__delete__()d (and unexpectedly dying) actor'),
+                StmtReturn(ExprLiteral.FALSE) ])
+        fromswitch.addcase(CaseLabel(_dyingState().name), dyingblock)
+
         unreachedblock = Block()
         unreachedblock.addstmts([
             _runtimeAbort('corrupted actor state'),
@@ -2912,6 +2940,21 @@ class _GenerateProtocolActorCode(ipdl.ast.Visitor):
 
                 method.addstmts([ routedecl, routeif, Whitespace.NL ])
 
+            # in the event of an RPC delete message, we want to loudly complain about
+            # messages that are received that are not a reply to the original message
+            if ptype.hasReentrantDelete:
+                msgVar = ExprVar(params[0].name)
+                ifdying = StmtIf(ExprBinary(
+                    ExprBinary(ExprVar('mState'), '==', _dyingState(ptype)),
+                    '&&',
+                    ExprBinary(
+                        ExprBinary(ExprCall(ExprSelect(msgVar, '.', 'is_reply')), '!=', ExprLiteral.TRUE),
+                        '||',
+                        ExprBinary(ExprCall(ExprSelect(msgVar, '.', 'is_rpc')), '!=', ExprLiteral.TRUE))))
+                ifdying.addifstmts([_fatalError('incoming message racing with actor deletion'),
+                                    StmtReturn(_Result.Processed)])
+                method.addstmt(ifdying)
+
             # bug 509581: don't generate the switch stmt if there
             # is only the default case; MSVC doesn't like that
             if switch.nr_cases > 1:
@@ -4527,9 +4570,13 @@ class _GenerateProtocolActorCode(ipdl.ast.Visitor):
         ifsendok.addifstmts([ Whitespace.NL,
                               StmtExpr(ExprAssn(sendok, ExprLiteral.FALSE, '&=')) ])
 
+        method.addstmt(ifsendok)
+
+        if self.protocol.decl.type.hasReentrantDelete:
+            method.addstmts(self.transition(md, 'in', actor.var(), reply=True))
+
         method.addstmts(
-            [ ifsendok ]
-            + self.dtorEpilogue(md, actor.var())
+            self.dtorEpilogue(md, actor.var())
             + [ Whitespace.NL, StmtReturn(sendok) ])
 
         return method
@@ -4907,7 +4954,7 @@ class _GenerateProtocolActorCode(ipdl.ast.Visitor):
             saveIdStmts = [ ]
         return idvar, saveIdStmts
 
-    def transition(self, md, direction, actor=None):
+    def transition(self, md, direction, actor=None, reply=False):
         if actor is not None:  stateexpr = _actorState(actor)
         else:                  stateexpr = self.protocol.stateVar()
         
@@ -4919,12 +4966,13 @@ class _GenerateProtocolActorCode(ipdl.ast.Visitor):
             action = ExprVar('Trigger::Recv')
         else: assert 0 and 'unknown combo %s/%s'% (self.side, direction)
 
+        msgid = md.pqMsgId() if not reply else md.pqReplyId()
         ifbad = StmtIf(ExprNot(
             ExprCall(
                 ExprVar(self.protocol.name +'::Transition'),
                 args=[ stateexpr,
                        ExprCall(ExprVar('Trigger'),
-                                args=[ action, ExprVar(md.pqMsgId()) ]),
+                                args=[ action, ExprVar(msgid) ]),
                        ExprAddrOf(stateexpr) ])))
         ifbad.addifstmts(_badTransition())
         return [ ifbad ]

ipc/ipdl/ipdl/type.py

@@ -271,6 +271,7 @@ class ProtocolType(IPDLType):
         self.manages = [ ]
         self.stateless = stateless
         self.hasDelete = False
+        self.hasReentrantDelete = False
     def isProtocol(self): return True
 
     def name(self):
@@ -827,6 +828,8 @@ class GatherDecls(TcheckVisitor):
                 "destructor declaration `%s(...)' required for managed protocol `%s'",
                 _DELETE_MSG, p.name)
 
+        p.decl.type.hasReentrantDelete = p.decl.type.hasDelete and self.symtab.lookup(_DELETE_MSG).type.isRpc()
+
         for managed in p.managesStmts:
             mgdname = managed.name
             ctordecl = self.symtab.lookup(mgdname +'Constructor')
@@ -845,13 +848,17 @@ class GatherDecls(TcheckVisitor):
             if 0 == len(p.startStates):
                 p.startStates = [ p.transitionStmts[0] ]
 
-        # declare implicit "any" and "dead" states
+        # declare implicit "any", "dead", and "dying" states
         self.declare(loc=State.ANY.loc,
                      type=StateType(p.decl.type, State.ANY.name, start=False),
                      progname=State.ANY.name)
         self.declare(loc=State.DEAD.loc,
                      type=StateType(p.decl.type, State.DEAD.name, start=False),
                      progname=State.DEAD.name)
+        if p.decl.type.hasReentrantDelete:
+            self.declare(loc=State.DYING.loc,
+                         type=StateType(p.decl.type, State.DYING.name, start=False),
+                         progname=State.DYING.name)
 
         # declare each state before decorating their mention
         for trans in p.transitionStmts:
@@ -871,6 +878,9 @@ class GatherDecls(TcheckVisitor):
             # add a special state |state DEAD: null goto DEAD;|
             deadtrans = TransitionStmt.makeNullStmt(State.DEAD)
             p.states[State.DEAD] = deadtrans           
+            if p.decl.type.hasReentrantDelete:
+                dyingtrans = TransitionStmt.makeNullStmt(State.DYING)
+                p.states[State.DYING] = dyingtrans
 
         # visit the message decls once more and resolve the state names
         # attached to actor params and returns

ipc/ipdl/test/cxx/IPDLUnitTestProcessChild.cpp

@@ -3,13 +3,13 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
-#include "nsRegion.h"
-
 #include "mozilla/ipc/IOThreadChild.h"
 
 #include "IPDLUnitTestProcessChild.h"
 #include "IPDLUnitTests.h"
 
+#include "nsRegion.h"
+
 using mozilla::ipc::IOThreadChild;
 
 namespace mozilla {

ipc/ipdl/test/cxx/IPDLUnitTests.template.cpp

@@ -111,7 +111,7 @@ ${ENUM_TO_STRINGS}
 IPDLUnitTestType
 IPDLUnitTest()
 {
-    return IPDLUnitTestFromString(mozilla::_ipdltest::IPDLUnitTestName());
+    return IPDLUnitTestFromString(::mozilla::_ipdltest::IPDLUnitTestName());
 }
 
 
@@ -250,7 +250,7 @@ DeleteParentActor()
 //===== TEMPLATED =====
 ${PARENT_DELETE_CASES}
 //-----------------------------------------------------------------------------
-    default:  mozilla::_ipdltest::fail("???");
+    default:  ::mozilla::_ipdltest::fail("???");
     }
 }
 
@@ -353,7 +353,7 @@ DeleteChildActor()
 //===== TEMPLATED =====
 ${CHILD_DELETE_CASES}
 //-----------------------------------------------------------------------------
-    default:  mozilla::_ipdltest::fail("???");
+    default:  ::mozilla::_ipdltest::fail("???");
     }
 }
 

ipc/ipdl/test/cxx/Makefile.in

@@ -54,6 +54,7 @@ IPDLTESTS = \
   TestSyncError \
   TestSyncHang \
   TestSyncWakeup \
+  TestBadActor \
   $(NULL)
 
 ifeq ($(OS_ARCH),Linux)

ipc/ipdl/test/cxx/PTestBadActor.ipdl

@@ -0,0 +1,18 @@
+include protocol PTestBadActorSub;
+
+namespace mozilla {
+namespace _ipdltest {
+
+// Test that a parent sending a reentrant __delete__ message
+// is not killed if a child's message races with the reply.
+
+rpc protocol PTestBadActor {
+  manages PTestBadActorSub;
+
+child:
+  PTestBadActorSub();
+  __delete__();
+};
+
+} // namespace _ipdltest
+} // namespace mozilla

ipc/ipdl/test/cxx/PTestBadActorSub.ipdl

@@ -0,0 +1,17 @@
+include protocol PTestBadActor;
+
+namespace mozilla {
+namespace _ipdltest {
+
+rpc protocol PTestBadActorSub {
+  manager PTestBadActor;
+
+child:
+  rpc __delete__();
+
+parent:
+  Ping();
+};
+
+} // namespace _ipdltest
+} // namespace mozilla

ipc/ipdl/test/cxx/TestBadActor.cpp

@@ -0,0 +1,51 @@
+#include "TestBadActor.h"
+#include "IPDLUnitTests.h"
+#include "mozilla/unused.h"
+
+namespace mozilla {
+namespace _ipdltest {
+
+void
+TestBadActorParent::Main()
+{
+  // This test is designed to test a race condition where the child sends us
+  // a message on an actor that we've already destroyed. The child process
+  // should die, and the parent process should not abort.
+
+  PTestBadActorSubParent* child = SendPTestBadActorSubConstructor();
+  if (!child)
+    fail("Sending constructor");
+
+  unused << child->Call__delete__(child);
+}
+
+PTestBadActorSubParent*
+TestBadActorParent::AllocPTestBadActorSub()
+{
+  return new TestBadActorSubParent();
+}
+
+bool
+TestBadActorSubParent::RecvPing()
+{
+  fail("Shouldn't have received ping.");
+  return false;
+}
+
+PTestBadActorSubChild*
+TestBadActorChild::AllocPTestBadActorSub()
+{
+  return new TestBadActorSubChild();
+}
+
+bool
+TestBadActorChild::RecvPTestBadActorSubConstructor(PTestBadActorSubChild* actor)
+{
+  if (!actor->SendPing()) {
+    fail("Couldn't send ping to an actor which supposedly isn't dead yet.");
+  }
+  return true;
+}
+
+} // namespace _ipdltest
+} // namespace mozilla

ipc/ipdl/test/cxx/TestBadActor.h

@@ -0,0 +1,91 @@
+#ifndef mozilla__ipdltest_TestBadActor_h
+#define mozilla__ipdltest_TestBadActor_h
+
+#include "mozilla/_ipdltest/IPDLUnitTests.h"
+
+#include "mozilla/_ipdltest/PTestBadActorParent.h"
+#include "mozilla/_ipdltest/PTestBadActorChild.h"
+
+#include "mozilla/_ipdltest/PTestBadActorSubParent.h"
+#include "mozilla/_ipdltest/PTestBadActorSubChild.h"
+
+namespace mozilla {
+namespace _ipdltest {
+
+class TestBadActorParent
+  : public PTestBadActorParent
+{
+public:
+  TestBadActorParent() { }
+  virtual ~TestBadActorParent() { }
+
+  static bool RunTestInProcesses() { return true; }
+  static bool RunTestInThreads() { return false; }
+
+  void Main();
+
+protected:
+  NS_OVERRIDE
+  virtual void ActorDestroy(ActorDestroyReason why)
+  {
+    if (AbnormalShutdown != why)
+      fail("unexpected destruction");
+    passed("ok");
+    QuitParent();
+  }
+
+  virtual PTestBadActorSubParent*
+  AllocPTestBadActorSub();
+
+  virtual bool
+  DeallocPTestBadActorSub(PTestBadActorSubParent* actor) {
+    delete actor;
+    return true;
+  }
+};
+
+class TestBadActorSubParent
+  : public PTestBadActorSubParent
+{
+public:
+  TestBadActorSubParent() { }
+  virtual ~TestBadActorSubParent() { }
+
+protected:
+  virtual bool RecvPing();
+};
+
+class TestBadActorChild
+  : public PTestBadActorChild
+{
+public:
+  TestBadActorChild() { }
+  virtual ~TestBadActorChild() { }
+
+protected:
+  virtual PTestBadActorSubChild*
+  AllocPTestBadActorSub();
+
+  virtual bool
+  DeallocPTestBadActorSub(PTestBadActorSubChild* actor)
+  {
+    delete actor;
+    return true;
+  }
+
+  virtual bool
+  RecvPTestBadActorSubConstructor(PTestBadActorSubChild* actor);
+};
+
+class TestBadActorSubChild
+  : public PTestBadActorSubChild
+{
+public:
+  TestBadActorSubChild() { }
+  virtual ~TestBadActorSubChild() { }
+};
+
+} // namespace _ipdltest
+} // namespace mozilla
+
+#endif // mozilla__ipdltest_TestBadActor_h

ipc/ipdl/test/cxx/TestDataStructures.h

@@ -24,7 +24,13 @@ public:
     { }
     virtual ~TestDataStructuresSub()
     { }
-    uint32 mI;
+    NS_OVERRIDE
+    virtual void ActorDestroy(ActorDestroyReason why)
+    {
+      if (Deletion != why)
+        fail("unexpected destruction!");
+    }
+  uint32 mI;
 };
 
 //-----------------------------------------------------------------------------

ipc/ipdl/test/cxx/genIPDLUnitTests.py

@@ -94,9 +94,9 @@ def main(argv):
         reinterpret_cast<%sChild**>(&gChildActor);
         *child = new %sChild();
 
-        mozilla::ipc::AsyncChannel *childChannel = (*child)->GetIPCChannel();
-        mozilla::ipc::AsyncChannel::Side parentSide = 
-            mozilla::ipc::AsyncChannel::Parent;
+        ::mozilla::ipc::AsyncChannel *childChannel = (*child)->GetIPCChannel();
+        ::mozilla::ipc::AsyncChannel::Side parentSide = 
+            ::mozilla::ipc::AsyncChannel::Parent;
 
         (*parent)->Open(childChannel, childMessageLoop, parentSide);
         return (*parent)->Main();

ipc/ipdl/test/cxx/ipdl.mk

@@ -45,4 +45,6 @@ IPDLSRCS =					\
   PTestSyncHang.ipdl                            \
   PTestSyncWakeup.ipdl				\
   PTestSysVShmem.ipdl				\
+  PTestBadActor.ipdl                            \
+  PTestBadActorSub.ipdl                         \
   $(NULL)