Bug 1724083 - Part 1: Rework ActorDestroy logic to be more resilient to nested event loops, r=ipc-reviewers,mccr8

Differential Revision: https://phabricator.services.mozilla.com/D198840

dom/ipc/ContentParent.cpp

@@ -2021,11 +2021,6 @@ void ContentParent::MarkAsDead() {
   mLifecycleState = LifecycleState::DEAD;
 }
 
-void ContentParent::OnChannelError() {
-  RefPtr<ContentParent> kungFuDeathGrip(this);
-  PContentParent::OnChannelError();
-}
-
 void ContentParent::ProcessingError(Result aCode, const char* aReason) {
   if (MsgDropped == aCode) {
     return;

dom/ipc/ContentParent.h

@@ -422,8 +422,6 @@ class ContentParent final : public PContentParent,
    */
   void FriendlyName(nsAString& aName, bool aAnonymize = false);
 
-  virtual void OnChannelError() override;
-
   mozilla::ipc::IPCResult RecvInitCrashReporter(
       const NativeThreadId& aThreadId);
 

ipc/glue/Endpoint.cpp

@@ -88,7 +88,7 @@ bool UntypedManagedEndpoint::BindCommon(IProtocol* aActor,
   mInner.reset();
 
   // Our typed caller will insert the actor into the managed container.
-  aActor->SetManagerAndRegister(aManager, id);
+  MOZ_ALWAYS_TRUE(aActor->SetManagerAndRegister(aManager, id));
 
   aManager->GetIPCChannel()->Send(
       MakeUnique<IPC::Message>(id, MANAGED_ENDPOINT_BOUND_MESSAGE_TYPE));

ipc/glue/ProtocolUtils.cpp

@@ -315,6 +315,7 @@ IProtocol::~IProtocol() {
   // Gecko, simply emit a warning and clear the weak backreference from our
   // LifecycleProxy back to us.
   if (mLifecycleProxy) {
+    MOZ_ASSERT(mLinkStatus != LinkStatus::Inactive);
     NS_WARNING(
         nsPrintfCString("Actor destructor for '%s%s' called before IPC "
                         "lifecycle complete!\n"
@@ -323,34 +324,13 @@ IProtocol::~IProtocol() {
             .get());
 
     mLifecycleProxy->mActor = nullptr;
-
-    // If we are somehow being destroyed while active, make sure that the
-    // existing IPC reference has been freed. If the status of the actor is
-    // `Destroyed`, the reference has already been freed, and we shouldn't free
-    // it a second time.
-    MOZ_ASSERT(mLinkStatus != LinkStatus::Inactive);
-    if (mLinkStatus != LinkStatus::Destroyed) {
-      NS_IF_RELEASE(mLifecycleProxy);
-    }
     mLifecycleProxy = nullptr;
   }
 }
 
 // The following methods either directly forward to the toplevel protocol, or
 // almost directly do.
-int32_t IProtocol::Register(IProtocol* aRouted) {
-  return mToplevel->Register(aRouted);
-}
-int32_t IProtocol::RegisterID(IProtocol* aRouted, int32_t aId) {
-  return mToplevel->RegisterID(aRouted, aId);
-}
 IProtocol* IProtocol::Lookup(int32_t aId) { return mToplevel->Lookup(aId); }
-void IProtocol::Unregister(int32_t aId) {
-  if (aId == mId) {
-    mId = kFreedActorId;
-  }
-  return mToplevel->Unregister(aId);
-}
 
 Shmem::SharedMemory* IProtocol::CreateSharedMemory(size_t aSize, bool aUnsafe,
                                                    int32_t* aId) {
@@ -487,21 +467,26 @@ void IProtocol::SetManager(IRefCountedProtocol* aManager) {
   mToplevel = aManager->mToplevel;
 }
 
-void IProtocol::SetManagerAndRegister(IRefCountedProtocol* aManager) {
-  // Set the manager prior to registering so registering properly inherits
-  // the manager's event target.
-  SetManager(aManager);
-
-  aManager->Register(this);
-}
-
-void IProtocol::SetManagerAndRegister(IRefCountedProtocol* aManager,
+bool IProtocol::SetManagerAndRegister(IRefCountedProtocol* 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);
+  mToplevel->RegisterID(this, aId);
+
+  // If our manager is already dying, mark ourselves as doomed as well.
+  RefPtr<ActorLifecycleProxy> proxy(mLifecycleProxy);
+  if (aManager && aManager->mLinkStatus != LinkStatus::Connected) {
+    MOZ_ASSERT(proxy->Get() == this);
+    proxy->Get()->mLinkStatus = LinkStatus::Doomed;
+    if (aManager->mLinkStatus != LinkStatus::Doomed) {
+      ActorDisconnected(FailedConstructor);
+      MOZ_ASSERT(mLinkStatus == LinkStatus::Destroyed);
+      return false;
+    }
+  }
+  return true;
 }
 
 void IProtocol::UnlinkManager() {
@@ -540,9 +525,9 @@ void IProtocol::WarnMessageDiscarded(IPC::Message* aMsg) {
 }
 #endif
 
-void IProtocol::ActorConnected() {
+already_AddRefed<ActorLifecycleProxy> IProtocol::ActorConnected() {
   if (mLinkStatus != LinkStatus::Inactive) {
-    return;
+    return nullptr;
   }
 
 #ifdef FUZZING_SNAPSHOT
@@ -552,73 +537,78 @@ void IProtocol::ActorConnected() {
   mLinkStatus = LinkStatus::Connected;
 
   MOZ_ASSERT(!mLifecycleProxy, "double-connecting live actor");
-  mLifecycleProxy = new ActorLifecycleProxy(this);
-  NS_ADDREF(mLifecycleProxy);  // Reference freed in DestroySubtree();
+  RefPtr<ActorLifecycleProxy> proxy = new ActorLifecycleProxy(this);
+  mLifecycleProxy = proxy;
+  return proxy.forget();
 }
 
-void IProtocol::DoomSubtree() {
-  MOZ_ASSERT(CanSend(), "dooming non-connected actor");
-  MOZ_ASSERT(mLifecycleProxy, "dooming zombie actor");
-
-  nsTArray<RefPtr<ActorLifecycleProxy>> managed;
-  AllManagedActors(managed);
-  for (ActorLifecycleProxy* proxy : managed) {
-    // Guard against actor being disconnected or destroyed during previous Doom
-    IProtocol* actor = proxy->Get();
-    if (actor && actor->CanSend()) {
-      actor->DoomSubtree();
-    }
+void IProtocol::ActorDisconnected(ActorDestroyReason aWhy) {
+  MOZ_ASSERT(mLifecycleProxy, "destroying zombie actor");
+  // If the actor has already been marked as `Destroyed`, there's nothing to do.
+  if (mLinkStatus != LinkStatus::Connected &&
+      mLinkStatus != LinkStatus::Doomed) {
+    return;
   }
 
-  // ActorDoom is called immediately before changing state, this allows messages
-  // to be sent during ActorDoom immediately before the channel is closed and
-  // sending messages is disabled.
-  ActorDoom();
-  mLinkStatus = LinkStatus::Doomed;
-}
+  // Mark the entire subtree as doomed so that no further messages can be
+  // sent/recieved, and newly created managed actors are immediately marked as
+  // doomed on creation.
+  DoomSubtree();
 
-void IProtocol::DestroySubtree(ActorDestroyReason aWhy) {
-  MOZ_ASSERT(CanRecv(), "destroying non-connected actor");
-  MOZ_ASSERT(mLifecycleProxy, "destroying zombie actor");
+  // Perform the steps to fully destroy an actor after it has been unregistered
+  // from its manager.
+  auto doActorDestroy = [toplevel = mToplevel, ipcChannel = GetIPCChannel()](
+                            IProtocol* actor, ActorDestroyReason why) {
+    MOZ_ASSERT(actor->mLinkStatus == LinkStatus::Doomed,
+               "Actor must be doomed when calling doActorDestroy");
+    MOZ_ASSERT(actor->AllManagedActorsCount() == 0,
+               "All managed actors must have been destroyed first");
+
+    // Mark the actor as Destroyed, ensuring we can't re-enter `ActorDestroy`,
+    // even if an callback spins a nested event loop.
+    actor->mLinkStatus = LinkStatus::Destroyed;
 
 #ifdef FUZZING_SNAPSHOT
-  fuzzing::IPCFuzzController::instance().OnActorDestroyed(this);
+    fuzzing::IPCFuzzController::instance().OnActorDestroyed(actor);
 #endif
 
-  int32_t id = Id();
+    int32_t id = actor->mId;
+    if (IProtocol* manager = actor->Manager()) {
+      actor->mId = kFreedActorId;
+      toplevel->Unregister(id);
+      manager->RemoveManagee(actor->GetProtocolId(), actor);
+    }
 
-  // If we're a managed actor, unregister from our manager
-  if (Manager()) {
-    Unregister(id);
-  }
+    ipcChannel->RejectPendingResponsesForActor(id);
+    actor->ActorDestroy(why);
+  };
 
-  // Destroy subtree
+  // Hold all ActorLifecycleProxy instances for managed actors until we return.
+  nsTArray<RefPtr<ActorLifecycleProxy>> proxyHolder;
+  proxyHolder.AppendElement(GetLifecycleProxy());
+
+  // Invoke `ActorDestroy` for all managed actors in the subtree. These are
+  // handled one at a time, so that new actors which are potentially registered
+  // during `ActorDestroy` callbacks are not missed.
   ActorDestroyReason subtreeWhy = aWhy;
   if (aWhy == Deletion || aWhy == FailedConstructor) {
     subtreeWhy = AncestorDeletion;
   }
-
-  nsTArray<RefPtr<ActorLifecycleProxy>> managed;
-  AllManagedActors(managed);
-  for (ActorLifecycleProxy* proxy : managed) {
-    // Guard against actor being disconnected or destroyed during previous
-    // Destroy
-    IProtocol* actor = proxy->Get();
-    if (actor && actor->CanRecv()) {
-      actor->DestroySubtree(subtreeWhy);
+  while (IProtocol* actor = PeekManagedActor()) {
+    // If the selected actor manages other actors, destroy those first.
+    while (IProtocol* inner = actor->PeekManagedActor()) {
+      actor = inner;
     }
+
+    proxyHolder.AppendElement(actor->GetLifecycleProxy());
+    doActorDestroy(actor, subtreeWhy);
   }
 
-  // Ensure that we don't send any messages while we're calling `ActorDestroy`
-  // by setting our state to `Doomed`.
-  mLinkStatus = LinkStatus::Doomed;
-
-  // The actor is being destroyed, reject any pending responses, invoke
-  // `ActorDestroy` to destroy it, and then clear our status to
-  // `LinkStatus::Destroyed`.
-  GetIPCChannel()->RejectPendingResponsesForActor(id);
-  ActorDestroy(aWhy);
-  mLinkStatus = LinkStatus::Destroyed;
+  // Destroy ourselves if we were not not otherwise destroyed while destroying
+  // managed actors.
+  if (mLinkStatus == LinkStatus::Doomed) {
+    doActorDestroy(this, aWhy);
+  }
 }
 
 IToplevelProtocol::IToplevelProtocol(const char* aName, ProtocolId aProtoId,
@@ -689,23 +679,21 @@ int32_t IToplevelProtocol::NextId() {
   return (++mLastLocalId << 2) | tag;
 }
 
-int32_t IToplevelProtocol::Register(IProtocol* aRouted) {
-  if (aRouted->Id() != kNullActorId && aRouted->Id() != kFreedActorId) {
-    // If there's already an ID, just return that.
-    return aRouted->Id();
-  }
-  return RegisterID(aRouted, NextId());
-}
-
 int32_t IToplevelProtocol::RegisterID(IProtocol* aRouted, int32_t aId) {
-  aRouted->SetId(aId);
-  aRouted->ActorConnected();
-  MOZ_ASSERT(!mActorMap.Contains(aId), "Don't insert with an existing ID");
-  mActorMap.InsertOrUpdate(aId, aRouted);
-  return aId;
+  int32_t id = aId == kNullActorId ? NextId() : aId;
+  aRouted->SetId(id);
+  RefPtr<ActorLifecycleProxy> proxy = aRouted->ActorConnected();
+  MOZ_ASSERT(!mActorMap.Contains(id), "Don't insert with an existing ID");
+  mActorMap.InsertOrUpdate(id, std::move(proxy));
+  return id;
 }
 
-IProtocol* IToplevelProtocol::Lookup(int32_t aId) { return mActorMap.Get(aId); }
+IProtocol* IToplevelProtocol::Lookup(int32_t aId) {
+  if (auto entry = mActorMap.Lookup(aId)) {
+    return entry.Data()->Get();
+  }
+  return nullptr;
+}
 
 void IToplevelProtocol::Unregister(int32_t aId) {
   MOZ_ASSERT(mActorMap.Contains(aId),

ipc/glue/ProtocolUtils.h

@@ -124,12 +124,16 @@ enum class LinkStatus : uint8_t {
   // A live link is connected to the other side of this actor.
   Connected,
 
-  // The link has begun being destroyed. Messages may still be received, but
-  // cannot be sent. (exception: sync replies may be sent while Doomed).
+  // The link has begun being destroyed. Messages may no longer be sent. The
+  // ActorDestroy method is queued to be called, but has not been invoked yet,
+  // as managed actors still need to be destroyed first.
+  //
+  // NOTE: While no new IPC can be received at this point, `CanRecv` will still
+  // be true until `LinkStatus::Destroyed`.
   Doomed,
 
-  // The link has been destroyed, and messages will no longer be sent or
-  // received.
+  // The actor has been destroyed, and ActorDestroy has been called, however an
+  // ActorLifecycleProxy still holds a reference to the actor.
   Destroyed,
 };
 
@@ -171,12 +175,8 @@ class IProtocol : public HasResultCodes {
   IToplevelProtocol* ToplevelProtocol() { return mToplevel; }
   const IToplevelProtocol* ToplevelProtocol() const { return mToplevel; }
 
-  // The following methods either directly forward to the toplevel protocol, or
-  // almost directly do.
-  int32_t Register(IProtocol* aRouted);
-  int32_t RegisterID(IProtocol* aRouted, int32_t aId);
+  // Lookup() is forwarded directly to the toplevel protocol.
   IProtocol* Lookup(int32_t aId);
-  void Unregister(int32_t aId);
 
   Shmem::SharedMemory* CreateSharedMemory(size_t aSize, bool aUnsafe,
                                           int32_t* aId);
@@ -204,6 +204,9 @@ class IProtocol : public HasResultCodes {
 
   Side GetSide() const { return mSide; }
   bool CanSend() const { return mLinkStatus == LinkStatus::Connected; }
+
+  // Returns `true` for an active actor until the actor's `ActorDestroy` method
+  // has been called.
   bool CanRecv() const {
     return mLinkStatus == LinkStatus::Connected ||
            mLinkStatus == LinkStatus::Doomed;
@@ -249,8 +252,8 @@ class IProtocol : public HasResultCodes {
   // 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(IRefCountedProtocol* aManager);
-  void SetManagerAndRegister(IRefCountedProtocol* aManager, int32_t aId);
+  bool SetManagerAndRegister(IRefCountedProtocol* aManager,
+                             int32_t aId = kNullActorId);
 
   // Helpers for calling `Send` on our underlying IPC channel.
   bool ChannelSend(UniquePtr<IPC::Message> aMsg);
@@ -270,28 +273,31 @@ class IProtocol : public HasResultCodes {
     }
   }
 
-  // Collect all actors managed by this object in an array. To make this safer
-  // to iterate, `ActorLifecycleProxy` references are returned rather than raw
-  // actor pointers.
-  virtual void AllManagedActors(
-      nsTArray<RefPtr<ActorLifecycleProxy>>& aActors) const = 0;
-
   virtual uint32_t AllManagedActorsCount() const = 0;
 
   // Internal method called when the actor becomes connected.
-  void ActorConnected();
+  already_AddRefed<ActorLifecycleProxy> ActorConnected();
 
-  // Called immediately before setting the actor state to doomed, and triggering
-  // async actor destruction. Messages may be sent from this callback, but no
-  // later.
-  // FIXME(nika): This is currently unused!
-  virtual void ActorDoom() {}
-  void DoomSubtree();
+  // Internal method called when actor becomes disconnected.
+  void ActorDisconnected(ActorDestroyReason aWhy);
+
+  // Called by DoomSubtree on each managed actor to mark it as Doomed and
+  // prevent further IPC.
+  void SetDoomed() {
+    MOZ_ASSERT(mLinkStatus == LinkStatus::Connected ||
+                   mLinkStatus == LinkStatus::Doomed,
+               "Invalid link status for SetDoomed");
+    mLinkStatus = LinkStatus::Doomed;
+  }
+  virtual void DoomSubtree() = 0;
+
+  // Internal function returning an arbitrary directly managed actor. Used to
+  // identify managed actors to destroy when tearing down an actor tree.
+  virtual IProtocol* PeekManagedActor() = 0;
 
   // Called when the actor has been destroyed due to an error, a __delete__
   // message, or a __doom__ reply.
   virtual void ActorDestroy(ActorDestroyReason aWhy) {}
-  void DestroySubtree(ActorDestroyReason aWhy);
 
   // Called when IPC has acquired its first reference to the actor. This method
   // may take references which will later be freed by `ActorDealloc`.
@@ -428,11 +434,10 @@ class IToplevelProtocol : public IRefCountedProtocol {
   ~IToplevelProtocol() = default;
 
  public:
-  // Shadow methods on IProtocol which are implemented directly on toplevel
-  // actors.
-  int32_t Register(IProtocol* aRouted);
-  int32_t RegisterID(IProtocol* aRouted, int32_t aId);
+  // Shadows the method on IProtocol, which will forward to the top.
   IProtocol* Lookup(int32_t aId);
+
+  int32_t RegisterID(IProtocol* aRouted, int32_t aId);
   void Unregister(int32_t aId);
 
   Shmem::SharedMemory* CreateSharedMemory(size_t aSize, bool aUnsafe,
@@ -446,8 +451,6 @@ class IToplevelProtocol : public IRefCountedProtocol {
 
   void SetOtherProcessId(base::ProcessId aOtherPid);
 
-  virtual void OnChannelClose() = 0;
-  virtual void OnChannelError() = 0;
   virtual void ProcessingError(Result aError, const char* aMsgName) {}
 
   bool Open(ScopedPort aPort, const nsID& aMessageChannelId,
@@ -513,7 +516,26 @@ class IToplevelProtocol : public IRefCountedProtocol {
 
   virtual void OnChannelReceivedMessage(const Message& aMsg) {}
 
-  void OnIPCChannelOpened() { ActorConnected(); }
+  // MessageChannel lifecycle callbacks.
+  void OnIPCChannelOpened() {
+    // Leak the returned ActorLifecycleProxy reference. It will be destroyed in
+    // `OnChannelClose` or `OnChannelError`.
+    Unused << ActorConnected();
+  }
+  void OnChannelClose() {
+    // Re-acquire the ActorLifecycleProxy reference acquired in
+    // OnIPCChannelOpened.
+    RefPtr<ActorLifecycleProxy> proxy = dont_AddRef(GetLifecycleProxy());
+    ActorDisconnected(NormalShutdown);
+    DeallocShmems();
+  }
+  void OnChannelError() {
+    // Re-acquire the ActorLifecycleProxy reference acquired in
+    // OnIPCChannelOpened.
+    RefPtr<ActorLifecycleProxy> proxy = dont_AddRef(GetLifecycleProxy());
+    ActorDisconnected(AbnormalShutdown);
+    DeallocShmems();
+  }
 
   base::ProcessId OtherPidMaybeInvalid() const { return mOtherPid; }
 
@@ -528,7 +550,7 @@ class IToplevelProtocol : public IRefCountedProtocol {
   // NOTE NOTE NOTE
   // Used to be on mState
   int32_t mLastLocalId;
-  IDMap<IProtocol*> mActorMap;
+  IDMap<RefPtr<ActorLifecycleProxy>> mActorMap;
   IDMap<RefPtr<Shmem::SharedMemory>> mShmemMap;
 
   MessageChannel mChannel;
@@ -754,6 +776,8 @@ class ManagedContainer {
     }
   }
 
+  Protocol* Peek() { return mArray.IsEmpty() ? nullptr : mArray.LastElement(); }
+
   void Clear() { mArray.Clear(); }
 
  private:

ipc/ipdl/ipdl/lower.py

@@ -474,10 +474,6 @@ def errfnSentinel(rvalue=ExprLiteral.FALSE):
     return inner
 
 
-def _destroyMethod():
-    return ExprVar("ActorDestroy")
-
-
 def errfnUnreachable(msg):
     return [_logicError(msg)]
 
@@ -3943,57 +3939,8 @@ class _GenerateProtocolActorCode(ipdl.ast.Visitor):
 
             self.cls.addstmts([meth, refmeth, Whitespace.NL])
 
-        # AllManagedActors(Array& inout) const
-        arrvar = ExprVar("arr__")
-        managedmeth = MethodDefn(
-            MethodDecl(
-                "AllManagedActors",
-                params=[
-                    Decl(
-                        _cxxArrayType(_refptr(_cxxLifecycleProxyType()), ref=True),
-                        arrvar.name,
-                    )
-                ],
-                methodspec=MethodSpec.OVERRIDE,
-                const=True,
-            )
-        )
-
-        # Count the number of managed actors, and allocate space in the output array.
-        managedmeth.addcode(
-            """
-            uint32_t total = 0;
-            """
-        )
-        for managed in ptype.manages:
-            managedmeth.addcode(
-                """
-                total += ${container}.Count();
-                """,
-                container=p.managedVar(managed, self.side),
-            )
-        managedmeth.addcode(
-            """
-            arr__.SetCapacity(total);
-
-            """
-        )
-
-        for managed in ptype.manages:
-            managedmeth.addcode(
-                """
-                for (auto* key : ${container}) {
-                    arr__.AppendElement(key->GetLifecycleProxy());
-                }
-
-                """,
-                container=p.managedVar(managed, self.side),
-            )
-
-        self.cls.addstmts([managedmeth, Whitespace.NL])
-
         # AllManagedActorsCount() const
-        managedmeth = MethodDefn(
+        managedcount = MethodDefn(
             MethodDecl(
                 "AllManagedActorsCount",
                 ret=Type.UINT32,
@@ -4003,26 +3950,26 @@ class _GenerateProtocolActorCode(ipdl.ast.Visitor):
         )
 
         # Count the number of managed actors.
-        managedmeth.addcode(
+        managedcount.addcode(
             """
             uint32_t total = 0;
             """
         )
         for managed in ptype.manages:
-            managedmeth.addcode(
+            managedcount.addcode(
                 """
                 total += ${container}.Count();
                 """,
                 container=p.managedVar(managed, self.side),
             )
-        managedmeth.addcode(
+        managedcount.addcode(
             """
             return total;
 
             """
         )
 
-        self.cls.addstmts([managedmeth, Whitespace.NL])
+        self.cls.addstmts([managedcount, Whitespace.NL])
 
         # OpenPEndpoint(...)/BindPEndpoint(...)
         for managed in ptype.manages:
@@ -4188,66 +4135,50 @@ class _GenerateProtocolActorCode(ipdl.ast.Visitor):
             ]
         )
 
-        clearsubtreevar = ExprVar("ClearSubtree")
+        # DoomSubtree()
+        doomsubtree = MethodDefn(
+            MethodDecl("DoomSubtree", methodspec=MethodSpec.OVERRIDE)
+        )
 
-        if ptype.isToplevel():
-            # OnChannelClose()
-            onclose = MethodDefn(
-                MethodDecl("OnChannelClose", methodspec=MethodSpec.OVERRIDE)
-            )
-            onclose.addcode(
-                """
-                DestroySubtree(NormalShutdown);
-                ClearSubtree();
-                DeallocShmems();
-                if (GetLifecycleProxy()) {
-                    GetLifecycleProxy()->Release();
-                }
-                """
-            )
-            self.cls.addstmts([onclose, Whitespace.NL])
-
-            # OnChannelError()
-            onerror = MethodDefn(
-                MethodDecl("OnChannelError", methodspec=MethodSpec.OVERRIDE)
-            )
-            onerror.addcode(
-                """
-                DestroySubtree(AbnormalShutdown);
-                ClearSubtree();
-                DeallocShmems();
-                if (GetLifecycleProxy()) {
-                    GetLifecycleProxy()->Release();
-                }
-                """
-            )
-            self.cls.addstmts([onerror, Whitespace.NL])
-
-        # private methods
-        self.cls.addstmt(Label.PRIVATE)
-
-        # ClearSubtree()
-        clearsubtree = MethodDefn(MethodDecl(clearsubtreevar.name))
         for managed in ptype.manages:
-            clearsubtree.addcode(
+            doomsubtree.addcode(
                 """
                 for (auto* key : ${container}) {
-                    key->ClearSubtree();
+                    key->DoomSubtree();
                 }
-                for (auto* key : ${container}) {
-                    // Recursively releasing ${container} kids.
-                    auto* proxy = key->GetLifecycleProxy();
-                    NS_IF_RELEASE(proxy);
-                }
-                ${container}.Clear();
-
                 """,
                 container=p.managedVar(managed, self.side),
             )
 
-        # don't release our own IPC reference: either the manager will do it,
-        # or we're toplevel
-        self.cls.addstmts([clearsubtree, Whitespace.NL])
+        doomsubtree.addcode("SetDoomed();\n")
+
+        self.cls.addstmts([doomsubtree, Whitespace.NL])
+
+        # IProtocol* PeekManagedActor() override
+        peekmanagedactor = MethodDefn(
+            MethodDecl(
+                "PeekManagedActor",
+                methodspec=MethodSpec.OVERRIDE,
+                ret=Type("IProtocol", ptr=True),
+            )
+        )
+
+        for managed in ptype.manages:
+            peekmanagedactor.addcode(
+                """
+                if (IProtocol* actor = ${container}.Peek()) {
+                    return actor;
+                }
+                """,
+                container=p.managedVar(managed, self.side),
+            )
+
+        peekmanagedactor.addcode("return nullptr;\n")
+
+        self.cls.addstmts([peekmanagedactor, Whitespace.NL])
+
+        # private methods
+        self.cls.addstmt(Label.PRIVATE)
 
         if not ptype.isToplevel():
             self.cls.addstmts(
@@ -4382,16 +4313,9 @@ class _GenerateProtocolActorCode(ipdl.ast.Visitor):
                 )
                 case = ExprCode(
                     """
-                    {
-                        ${manageecxxtype} actor = static_cast<${manageecxxtype}>(aListener);
+                    MOZ_ALWAYS_TRUE(${container}.EnsureRemoved(static_cast<${manageecxxtype}>(aListener)));
+                    return;
 
-                        const bool removed = ${container}.EnsureRemoved(actor);
-                        MOZ_RELEASE_ASSERT(removed, "actor not managed by this!");
-
-                        auto* proxy = actor->GetLifecycleProxy();
-                        NS_IF_RELEASE(proxy);
-                        return;
-                    }
                     """,
                     manageecxxtype=manageecxxtype,
                     container=p.managedVar(manageeipdltype, self.side),
@@ -4708,14 +4632,18 @@ class _GenerateProtocolActorCode(ipdl.ast.Visitor):
         return [
             StmtCode(
                 """
-            if (!${actor}) {
-                NS_WARNING("Cannot bind null ${actorname} actor");
-                return ${errfn};
-            }
+                if (!${actor}) {
+                    NS_WARNING("Cannot bind null ${actorname} actor");
+                    return ${errfn};
+                }
 
-            ${actor}->SetManagerAndRegister($,{setManagerArgs});
-            ${container}.Insert(${actor});
-            """,
+                if (${actor}->SetManagerAndRegister($,{setManagerArgs})) {
+                    ${container}.Insert(${actor});
+                } else {
+                    NS_WARNING("Failed to bind ${actorname} actor");
+                    return ${errfn};
+                }
+                """,
                 actor=actordecl.var(),
                 actorname=actorproto.name() + self.side.capitalize(),
                 errfn=errfn,
@@ -4766,22 +4694,13 @@ class _GenerateProtocolActorCode(ipdl.ast.Visitor):
         return method, (lbl, case)
 
     def destroyActor(self, md, actorexpr, why=_DestroyReason.Deletion):
-        if md and md.decl.type.isCtor():
-            destroyedType = md.decl.type.constructedType()
-        else:
-            destroyedType = self.protocol.decl.type
-
         return [
             StmtCode(
                 """
-                IProtocol* mgr = ${actor}->Manager();
-                ${actor}->DestroySubtree(${why});
-                ${actor}->ClearSubtree();
-                mgr->RemoveManagee(${protoId}, ${actor});
+                ${actor}->ActorDisconnected(${why});
                 """,
                 actor=actorexpr,
                 why=why,
-                protoId=_protocolId(destroyedType),
             )
         ]