Bug 1509591 - Part 1: Maintain an IPCOpen variable in IProtocol, r=mccr8
☠☠ backed out by 9bfe29337ffe ☠ ☠
authorNika Layzell <nika@thelayzells.com>
Fri, 23 Nov 2018 17:13:26 -0500
changeset 507661 123b5d5a36377f409cb5c0a5e691e5eadb31626a
parent 507660 bce195f98895f82bd5ace5fc5ed5a5ca3b9f1afd
child 507662 6494840edc17b976778f057b1edd0b7c03e19d0b
push id1905
push userffxbld-merge
push dateMon, 21 Jan 2019 12:33:13 +0000
treeherdermozilla-release@c2fca1944d8c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmccr8
bugs1509591
milestone65.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1509591 - Part 1: Maintain an IPCOpen variable in IProtocol, r=mccr8 Previously there were many different inconsistent implementations of this variable across many different actors. This adds a unified implementation of this variable inside of IProtocol. Differential Revision: https://phabricator.services.mozilla.com/D12956
ipc/glue/ProtocolUtils.cpp
ipc/glue/ProtocolUtils.h
ipc/ipdl/ipdl/lower.py
--- a/ipc/glue/ProtocolUtils.cpp
+++ b/ipc/glue/ProtocolUtils.cpp
@@ -591,28 +591,34 @@ IProtocol::SetManagerAndRegister(IProtoc
 {
   // Set the manager prior to registering so registering properly inherits
   // the manager's event target.
   SetManager(aManager);
 
   aManager->Register(this);
 
   mState->SetIPCChannel(aManager->GetIPCChannel());
+
+  // Note that our actor has been opened.
+  ActorOpenedInternal();
 }
 
 void
 IProtocol::SetManagerAndRegister(IProtocol* aManager, int32_t aId)
 {
   // Set the manager prior to registering so registering properly inherits
   // the manager's event target.
   SetManager(aManager);
 
   aManager->RegisterID(this, aId);
 
   mState->SetIPCChannel(aManager->GetIPCChannel());
+
+  // Note that our actor has been opened.
+  ActorOpenedInternal();
 }
 
 void
 IProtocol::SetEventTargetForActor(IProtocol* aActor, nsIEventTarget* aEventTarget)
 {
   // Make sure we have a manager for the internal method to access.
   aActor->SetManager(this);
   mState->SetEventTargetForActor(aActor, aEventTarget);
@@ -746,49 +752,54 @@ IToplevelProtocol::TakeMinidump(nsIFile*
 
 bool
 IToplevelProtocol::Open(mozilla::ipc::Transport* aTransport,
                         base::ProcessId aOtherPid,
                         MessageLoop* aThread,
                         mozilla::ipc::Side aSide)
 {
   SetOtherProcessId(aOtherPid);
+  ActorOpenedInternal();
   return GetIPCChannel()->Open(aTransport, aThread, aSide);
 }
 
 bool
 IToplevelProtocol::Open(MessageChannel* aChannel,
                         MessageLoop* aMessageLoop,
                         mozilla::ipc::Side aSide)
 {
   SetOtherProcessId(base::GetCurrentProcId());
+  ActorOpenedInternal();
   return GetIPCChannel()->Open(aChannel, aMessageLoop->SerialEventTarget(), aSide);
 }
 
 bool
 IToplevelProtocol::Open(MessageChannel* aChannel,
                         nsIEventTarget* aEventTarget,
                         mozilla::ipc::Side aSide)
 {
   SetOtherProcessId(base::GetCurrentProcId());
+  ActorOpenedInternal();
   return GetIPCChannel()->Open(aChannel, aEventTarget, aSide);
 }
 
 bool
 IToplevelProtocol::OpenWithAsyncPid(mozilla::ipc::Transport* aTransport,
                                     MessageLoop* aThread,
                                     mozilla::ipc::Side aSide)
 {
+  ActorOpenedInternal();
   return GetIPCChannel()->Open(aTransport, aThread, aSide);
 }
 
 bool
 IToplevelProtocol::OpenOnSameThread(MessageChannel* aChannel, Side aSide)
 {
   SetOtherProcessId(base::GetCurrentProcId());
+  ActorOpenedInternal();
   return GetIPCChannel()->OpenOnSameThread(aChannel, aSide);
 }
 
 void
 IToplevelProtocol::Close()
 {
   GetIPCChannel()->Close();
 }
@@ -811,16 +822,19 @@ IToplevelProtocol::ToplevelState::Regist
   if (aRouted->Id() != kNullActorId && aRouted->Id() != kFreedActorId) {
     // If there's already an ID, just return that.
     return aRouted->Id();
   }
   int32_t id = mProtocol->GetSide() == ParentSide ? ++mLastRouteId : --mLastRouteId;
   mActorMap.AddWithID(aRouted, id);
   aRouted->SetId(id);
 
+  // Inform our actor that it has been opened.
+  aRouted->ActorOpenedInternal();
+
   // Inherit our event target from our manager.
   if (IProtocol* manager = aRouted->Manager()) {
     MutexAutoLock lock(mEventTargetMutex);
     if (nsCOMPtr<nsIEventTarget> target = mEventTargetMap.Lookup(manager->Id())) {
       mEventTargetMap.AddWithID(target, id);
     }
   }
 
@@ -828,16 +842,20 @@ IToplevelProtocol::ToplevelState::Regist
 }
 
 int32_t
 IToplevelProtocol::ToplevelState::RegisterID(IProtocol* aRouted,
                                      int32_t aId)
 {
   mActorMap.AddWithID(aRouted, aId);
   aRouted->SetId(aId);
+
+  // Inform our actor that it has been opened.
+  aRouted->ActorOpenedInternal();
+
   return aId;
 }
 
 IProtocol*
 IToplevelProtocol::ToplevelState::Lookup(int32_t aId)
 {
   return mActorMap.Lookup(aId);
 }
@@ -1042,16 +1060,17 @@ IToplevelProtocol::ToplevelState::SetEve
   // We should only call this function on actors that haven't been used for IPC
   // code yet. Otherwise we'll be posting stuff to the wrong event target before
   // we're called.
   MOZ_RELEASE_ASSERT(aActor->Id() == kNullActorId || aActor->Id() == kFreedActorId);
 
   // Register the actor early. When it's registered again, it will keep the same
   // ID.
   int32_t id = Register(aActor);
+  // XXX(nika): Register already calls SetId?
   aActor->SetId(id);
 
   MutexAutoLock lock(mEventTargetMutex);
   // FIXME bug 1445121 - sometimes the id is already mapped.
   // (IDMap debug-asserts that the existing state is as expected.)
   bool replace = false;
 #ifdef DEBUG
   replace = mEventTargetMap.Lookup(id) != nullptr;
--- a/ipc/glue/ProtocolUtils.h
+++ b/ipc/glue/ProtocolUtils.h
@@ -317,16 +317,25 @@ public:
                                 const char* aActorDescription, int32_t aProtocolTypeId);
 
     virtual Result OnMessageReceived(const Message& aMessage) = 0;
     virtual Result OnMessageReceived(const Message& aMessage, Message *& aReply) = 0;
     virtual Result OnCallReceived(const Message& aMessage, Message *& aReply) = 0;
 
     virtual int32_t GetProtocolTypeId() = 0;
 
+    // Returns |true| if the IPC channel is currently open, and |false|
+    // otherwise.
+    bool IPCOpen() const { return mIPCOpen; }
+
+    // This virtual method is called on actors as they are being destroyed from
+    // IPC's point of view. After ActorDestroy is called, IPC will free its
+    // reference to the actor.
+    virtual void ActorDestroy(ActorDestroyReason aWhy) {}
+
     int32_t Id() const { return mId; }
     IProtocol* Manager() const { return mManager; }
 
     bool AllocShmem(size_t aSize, Shmem::SharedMemory::SharedMemoryType aType, Shmem* aOutMem);
     bool AllocUnsafeShmem(size_t aSize, Shmem::SharedMemory::SharedMemoryType aType, Shmem* aOutMem);
     bool DeallocShmem(Shmem& aMem);
 
     // Sets an event target to which all messages for aActor will be
@@ -345,16 +354,17 @@ public:
 
     nsIEventTarget* GetActorEventTarget();
     already_AddRefed<nsIEventTarget> GetActorEventTarget(IProtocol* aActor);
 
 protected:
     IProtocol(Side aSide, UniquePtr<ProtocolState> aState)
         : mId(0)
         , mSide(aSide)
+        , mIPCOpen(false)
         , mManager(nullptr)
         , mState(std::move(aState))
     {}
 
     friend class IToplevelProtocol;
 
     void SetId(int32_t aId) { mId = aId; }
     void ResetManager() { mManager = nullptr; }
@@ -363,22 +373,38 @@ protected:
     void SetManager(IProtocol* aManager);
 
     // Sets the manager for the protocol and registers the protocol with
     // its manager, setting up channels for the protocol as well.  Not
     // for use outside of IPDL.
     void SetManagerAndRegister(IProtocol* aManager);
     void SetManagerAndRegister(IProtocol* aManager, int32_t aId);
 
+    // This method marks the channel as closed. Actors have this called when
+    // |DestroySubtree| is called due to the underlying channel being closed, or
+    // the actor's __delete__ method being called.
+    void ActorDestroyInternal(ActorDestroyReason aWhy) {
+        mIPCOpen = false;
+        ActorDestroy(aWhy);
+    }
+
+    // This method marks the channel as opened. Managed actors have this set
+    // when they are registered with their manager, and toplevel actors set this
+    // when opening the underlying MessageChannel.
+    void ActorOpenedInternal() {
+        mIPCOpen = true;
+    }
+
     static const int32_t kNullActorId = 0;
     static const int32_t kFreedActorId = 1;
 
 private:
     int32_t mId;
     Side mSide;
+    bool mIPCOpen;
     IProtocol* mManager;
     UniquePtr<ProtocolState> mState;
 };
 
 typedef IPCMessageStart ProtocolId;
 
 #define IPC_OK() mozilla::ipc::IPCResult::Ok()
 #define IPC_FAIL(actor, why) mozilla::ipc::IPCResult::Fail(WrapNotNull(actor), __func__, (why))
--- a/ipc/ipdl/ipdl/lower.py
+++ b/ipc/ipdl/ipdl/lower.py
@@ -482,18 +482,18 @@ def errfnRecv(msg, errcode=_Result.ValuE
 
 
 def errfnSentinel(rvalue=ExprLiteral.FALSE):
     def inner(msg):
         return [_sentinelReadError(msg), StmtReturn(rvalue)]
     return inner
 
 
-def _destroyMethod():
-    return ExprVar('ActorDestroy')
+def _destroyInternalMethod():
+    return ExprVar('ActorDestroyInternal')
 
 
 def errfnUnreachable(msg):
     return [
         _logicError(msg)
     ]
 
 
@@ -3264,31 +3264,16 @@ class _GenerateProtocolActorCode(ipdl.as
                 params=md.makeCxxParams(side=self.side, implicit=0),
                 ret=actortype, methodspec=MethodSpec.PURE)))
 
             self.cls.addstmt(StmtDecl(MethodDecl(
                 _deallocMethod(managed, self.side).name,
                 params=[Decl(actortype, 'aActor')],
                 ret=Type.BOOL, methodspec=MethodSpec.PURE)))
 
-        # ActorDestroy() method; default is no-op
-        if self.side == 'parent':
-            methodspec = MethodSpec.PURE
-        else:
-            methodspec = MethodSpec.VIRTUAL
-
-        self.cls.addstmts([
-            Whitespace.NL,
-            MethodDefn(MethodDecl(
-                _destroyMethod().name,
-                params=[Decl(_DestroyReason.Type(), 'aWhy')],
-                ret=Type.VOID, methodspec=methodspec)),
-            Whitespace.NL
-        ])
-
         if ptype.isToplevel():
             # void ProcessingError(code); default to no-op
             processingerror = MethodDefn(
                 MethodDecl(p.processingErrorVar().name,
                            params=[Param(_Result.Type(), 'aCode'),
                                    Param(Type('char', const=1, ptr=1), 'aReason')],
                            methodspec=MethodSpec.OVERRIDE))
 
@@ -3659,17 +3644,17 @@ class _GenerateProtocolActorCode(ipdl.as
                                             indent=1),
                                  StmtExpr(ExprCall(rejectPendingResponsesMethod,
                                                    args=[ExprVar('this')])),
                                  Whitespace.NL
                                  ])
 
         destroysubtree.addstmts([Whitespace('// Finally, destroy "us".\n',
                                             indent=1),
-                                 StmtExpr(ExprCall(_destroyMethod(),
+                                 StmtExpr(ExprCall(_destroyInternalMethod(),
                                                    args=[whyvar]))
                                  ])
 
         self.cls.addstmts([destroysubtree, Whitespace.NL])
 
         # DeallocSubtree()
         deallocsubtree = MethodDefn(MethodDecl(deallocsubtreevar.name))
         for managed in ptype.manages: