diff --git a/ipc/ipdl/ipdl/ast.py b/ipc/ipdl/ipdl/ast.py index ab0d16d49a62..62c423ae2677 100644 --- a/ipc/ipdl/ipdl/ast.py +++ b/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): diff --git a/ipc/ipdl/ipdl/lower.py b/ipc/ipdl/ipdl/lower.py index ef47fc7e0263..0d8c1c973ece 100644 --- a/ipc/ipdl/ipdl/lower.py +++ b/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 ] diff --git a/ipc/ipdl/ipdl/type.py b/ipc/ipdl/ipdl/type.py index 20737bf79e77..8ed6c214843f 100644 --- a/ipc/ipdl/ipdl/type.py +++ b/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 diff --git a/ipc/ipdl/test/cxx/IPDLUnitTestProcessChild.cpp b/ipc/ipdl/test/cxx/IPDLUnitTestProcessChild.cpp index a5c7b47eced1..24479226420c 100644 --- a/ipc/ipdl/test/cxx/IPDLUnitTestProcessChild.cpp +++ b/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 { diff --git a/ipc/ipdl/test/cxx/IPDLUnitTests.template.cpp b/ipc/ipdl/test/cxx/IPDLUnitTests.template.cpp index 4663e5a64fb7..8c4958c3a839 100644 --- a/ipc/ipdl/test/cxx/IPDLUnitTests.template.cpp +++ b/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("???"); } } diff --git a/ipc/ipdl/test/cxx/Makefile.in b/ipc/ipdl/test/cxx/Makefile.in index 676542caa03d..68613cdce212 100644 --- a/ipc/ipdl/test/cxx/Makefile.in +++ b/ipc/ipdl/test/cxx/Makefile.in @@ -54,6 +54,7 @@ IPDLTESTS = \ TestSyncError \ TestSyncHang \ TestSyncWakeup \ + TestBadActor \ $(NULL) ifeq ($(OS_ARCH),Linux) diff --git a/ipc/ipdl/test/cxx/PTestBadActor.ipdl b/ipc/ipdl/test/cxx/PTestBadActor.ipdl new file mode 100644 index 000000000000..626943c170b8 --- /dev/null +++ b/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 diff --git a/ipc/ipdl/test/cxx/PTestBadActorSub.ipdl b/ipc/ipdl/test/cxx/PTestBadActorSub.ipdl new file mode 100644 index 000000000000..597b8129da21 --- /dev/null +++ b/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 diff --git a/ipc/ipdl/test/cxx/TestBadActor.cpp b/ipc/ipdl/test/cxx/TestBadActor.cpp new file mode 100644 index 000000000000..14b9e18290b9 --- /dev/null +++ b/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 diff --git a/ipc/ipdl/test/cxx/TestBadActor.h b/ipc/ipdl/test/cxx/TestBadActor.h new file mode 100644 index 000000000000..783d69b04e74 --- /dev/null +++ b/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 diff --git a/ipc/ipdl/test/cxx/TestDataStructures.h b/ipc/ipdl/test/cxx/TestDataStructures.h index 0de9883930e2..90b6f4ec7f92 100644 --- a/ipc/ipdl/test/cxx/TestDataStructures.h +++ b/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; }; //----------------------------------------------------------------------------- diff --git a/ipc/ipdl/test/cxx/genIPDLUnitTests.py b/ipc/ipdl/test/cxx/genIPDLUnitTests.py index 8354c501bea7..f8112c6cd3f7 100644 --- a/ipc/ipdl/test/cxx/genIPDLUnitTests.py +++ b/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(); diff --git a/ipc/ipdl/test/cxx/ipdl.mk b/ipc/ipdl/test/cxx/ipdl.mk index ff362cbbfd89..978f43d207f9 100644 --- a/ipc/ipdl/test/cxx/ipdl.mk +++ b/ipc/ipdl/test/cxx/ipdl.mk @@ -45,4 +45,6 @@ IPDLSRCS = \ PTestSyncHang.ipdl \ PTestSyncWakeup.ipdl \ PTestSysVShmem.ipdl \ + PTestBadActor.ipdl \ + PTestBadActorSub.ipdl \ $(NULL)