Bug 1555287 - Make sure to detach browsing context children early. r=nika

Waiting for docshells and frameloaders to destroy will leave attached
browsing contexts attached too long. In case the children of a
browsing contexts cannot be cached we want to detach all of them as
soon as possible.

Also normalizes the use of BrowsingContext::mGroup.

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

--HG--
extra : moz-landing-system : lando

docshell/base/BrowsingContext.cpp

@@ -179,7 +179,8 @@ BrowsingContext::BrowsingContext(BrowsingContext* aParent,
       mBrowsingContextId(aBrowsingContextId),
       mGroup(aGroup),
       mParent(aParent),
-      mIsInProcess(false) {
+      mIsInProcess(false),
+      mIsDiscarded(false) {
   MOZ_RELEASE_ASSERT(!mParent || mParent->Group() == mGroup);
   MOZ_RELEASE_ASSERT(mBrowsingContextId != 0);
   MOZ_RELEASE_ASSERT(mGroup);
@@ -210,7 +211,7 @@ void BrowsingContext::SetEmbedderElement(Element* aEmbedder) {
 
       MOZ_DIAGNOSTIC_ASSERT(mType == Type::Chrome, "must be chrome");
       MOZ_DIAGNOSTIC_ASSERT(XRE_IsParentProcess(), "must be in parent");
-      MOZ_DIAGNOSTIC_ASSERT(!Group()->IsContextCached(this),
+      MOZ_DIAGNOSTIC_ASSERT(!mGroup->IsContextCached(this),
                             "cannot be in bfcache");
 
       RefPtr<BrowsingContext> kungFuDeathGrip(this);
@@ -248,7 +249,9 @@ void BrowsingContext::Attach(bool aFromIPC) {
            XRE_IsParentProcess() ? "Parent" : "Child", Id(),
            mParent ? mParent->Id() : 0));
 
-  MOZ_DIAGNOSTIC_ASSERT(!Group()->IsContextCached(this));
+  MOZ_DIAGNOSTIC_ASSERT(mGroup);
+  MOZ_DIAGNOSTIC_ASSERT(!mGroup->IsContextCached(this));
+  MOZ_DIAGNOSTIC_ASSERT(!mIsDiscarded);
 
   auto* children = mParent ? &mParent->mChildren : &mGroup->Toplevels();
   MOZ_DIAGNOSTIC_ASSERT(!children->Contains(this));
@@ -262,7 +265,7 @@ void BrowsingContext::Attach(bool aFromIPC) {
           GetIPCInitializer());
     } else if (IsContent()) {
       MOZ_DIAGNOSTIC_ASSERT(XRE_IsParentProcess());
-      Group()->EachParent([&](ContentParent* aParent) {
+      mGroup->EachParent([&](ContentParent* aParent) {
         Unused << aParent->SendAttachBrowsingContext(GetIPCInitializer());
       });
     }
@@ -275,36 +278,25 @@ void BrowsingContext::Detach(bool aFromIPC) {
            XRE_IsParentProcess() ? "Parent" : "Child", Id(),
            mParent ? mParent->Id() : 0));
 
+  // Unlinking might remove our group before Detach gets called.
+  if (NS_WARN_IF(!mGroup)) {
+    return;
+  }
+
   RefPtr<BrowsingContext> kungFuDeathGrip(this);
 
-  if (Group() && !Group()->EvictCachedContext(this)) {
+  if (!mGroup->EvictCachedContext(this)) {
     Children* children = nullptr;
     if (mParent) {
       children = &mParent->mChildren;
-    } else if (mGroup) {
+    } else {
       children = &mGroup->Toplevels();
     }
 
-    if (children) {
-      // TODO(farre): This assert looks extremely fishy, I know, but
-      // what we're actually saying is this: if we're detaching, but our
-      // parent doesn't have any children, it is because we're being
-      // detached by the cycle collector destroying docshells out of
-      // order.
-      MOZ_DIAGNOSTIC_ASSERT(children->IsEmpty() || children->Contains(this));
-
-      children->RemoveElement(this);
-    }
+    children->RemoveElement(this);
   }
 
-  if (mGroup) {
-    mGroup->Unregister(this);
-  }
-
-  // As our nsDocShell is going away, this should implicitly mark us as closed.
-  // We directly set our member, rather than using a transaction as we're going
-  // to send a `Detach` message to other processes either way.
-  mClosed = true;
+  Unregister();
 
   if (!aFromIPC && XRE_IsContentProcess()) {
     auto cc = ContentChild::GetSingleton();
@@ -313,6 +305,32 @@ void BrowsingContext::Detach(bool aFromIPC) {
   }
 }
 
+void BrowsingContext::DetachChildren(bool aFromIPC) {
+  if (mChildren.IsEmpty()) {
+    return;
+  }
+
+  MOZ_LOG(GetLog(), LogLevel::Debug,
+          ("%s: Detaching all children of 0x%08" PRIx64 "",
+           XRE_IsParentProcess() ? "Parent" : "Child", Id()));
+
+  // TODO(farre). Watch out! Notice the missing call to
+  // BrowsingContextGroup::Unregister here, this is intentional. If we
+  // unregister and set the detached flag here, tests will fail
+  // because not being able to look up Window.top. (Un-)Fortunately
+  // nsDocShell::Destroy will still call BrowsingContext::Detach,
+  // which will set the detached flag at that point. Bug 1558176 would
+  // clean this up.
+
+  mChildren.Clear();
+
+  if (!aFromIPC && XRE_IsContentProcess()) {
+    auto cc = ContentChild::GetSingleton();
+    MOZ_DIAGNOSTIC_ASSERT(cc);
+    cc->SendDetachBrowsingContextChildren(this);
+  }
+}
+
 void BrowsingContext::PrepareForProcessChange() {
   MOZ_LOG(GetLog(), LogLevel::Debug,
           ("%s: Preparing 0x%08" PRIx64 " for a process change",
@@ -338,7 +356,7 @@ void BrowsingContext::CacheChildren(bool aFromIPC) {
           ("%s: Caching children of 0x%08" PRIx64 "",
            XRE_IsParentProcess() ? "Parent" : "Child", Id()));
 
-  Group()->CacheContexts(mChildren);
+  mGroup->CacheContexts(mChildren);
   mChildren.Clear();
 
   if (!aFromIPC && XRE_IsContentProcess()) {
@@ -355,7 +373,7 @@ void BrowsingContext::RestoreChildren(Children&& aChildren, bool aFromIPC) {
 
   for (BrowsingContext* child : aChildren) {
     MOZ_DIAGNOSTIC_ASSERT(child->GetParent() == this);
-    Unused << Group()->EvictCachedContext(child);
+    Unused << mGroup->EvictCachedContext(child);
   }
 
   mChildren.AppendElements(aChildren);
@@ -367,7 +385,7 @@ void BrowsingContext::RestoreChildren(Children&& aChildren, bool aFromIPC) {
   }
 }
 
-bool BrowsingContext::IsCached() { return Group()->IsContextCached(this); }
+bool BrowsingContext::IsCached() { return mGroup->IsContextCached(this); }
 
 bool BrowsingContext::HasOpener() const {
   return sBrowsingContexts->Contains(mOpenerId);
@@ -531,6 +549,12 @@ bool BrowsingContext::IsActive() const {
   return false;
 }
 
+void BrowsingContext::Unregister() {
+  MOZ_DIAGNOSTIC_ASSERT(mGroup);
+  mGroup->Unregister(this);
+  mIsDiscarded = true;
+}
+
 BrowsingContext::~BrowsingContext() {
   MOZ_DIAGNOSTIC_ASSERT(!mParent || !mParent->mChildren.Contains(this));
   MOZ_DIAGNOSTIC_ASSERT(!mGroup || !mGroup->Toplevels().Contains(this));
@@ -670,7 +694,7 @@ void BrowsingContext::Blur(ErrorResult& aError) {
 }
 
 Nullable<WindowProxyHolder> BrowsingContext::GetTop(ErrorResult& aError) {
-  if (mClosed) {
+  if (mIsDiscarded) {
     return nullptr;
   }
 
@@ -694,7 +718,7 @@ void BrowsingContext::GetOpener(JSContext* aCx,
 }
 
 Nullable<WindowProxyHolder> BrowsingContext::GetParent(ErrorResult& aError) {
-  if (mClosed) {
+  if (mIsDiscarded) {
     return nullptr;
   }
 

docshell/base/BrowsingContext.h

@@ -148,6 +148,10 @@ class BrowsingContext : public nsWrapperCache, public BrowsingContextBase {
   // Prepare this BrowsingContext to leave the current process.
   void PrepareForProcessChange();
 
+  // Detach the current BrowsingContext's children, in both the child
+  // and the parent process.
+  void DetachChildren(bool aFromIPC = false);
+
   // Remove all children from the current BrowsingContext and cache
   // them to allow them to be attached again.
   void CacheChildren(bool aFromIPC = false);
@@ -380,6 +384,9 @@ class BrowsingContext : public nsWrapperCache, public BrowsingContextBase {
 
   bool IsActive() const;
 
+  // Removes the context from its group and sets mIsDetached to true.
+  void Unregister();
+
   friend class ::nsOuterWindowProxy;
   friend class ::nsGlobalWindowOuter;
   // Update the window proxy object that corresponds to this browsing context.
@@ -461,6 +468,10 @@ class BrowsingContext : public nsWrapperCache, public BrowsingContextBase {
   // process? This may be true with a null mDocShell after the Window has been
   // closed.
   bool mIsInProcess : 1;
+
+  // Has this browsing context been detached? BrowsingContexts should
+  // only be detached once.
+  bool mIsDiscarded : 1;
 };
 
 /**

docshell/base/nsDocShell.cpp

@@ -541,6 +541,8 @@ void nsDocShell::DestroyChildren() {
   }
 
   nsDocLoader::DestroyChildren();
+
+  mBrowsingContext->DetachChildren();
 }
 
 NS_IMPL_CYCLE_COLLECTION_INHERITED(nsDocShell, nsDocLoader,

dom/ipc/ContentChild.cpp

@@ -3855,6 +3855,15 @@ mozilla::ipc::IPCResult ContentChild::RecvDetachBrowsingContext(
   return IPC_OK();
 }
 
+mozilla::ipc::IPCResult ContentChild::RecvDetachBrowsingContextChildren(
+    BrowsingContext* aContext) {
+  MOZ_RELEASE_ASSERT(aContext);
+
+  aContext->DetachChildren(/* aFromIPC */ true);
+
+  return IPC_OK();
+}
+
 mozilla::ipc::IPCResult ContentChild::RecvCacheBrowsingContextChildren(
     BrowsingContext* aContext) {
   MOZ_RELEASE_ASSERT(aContext);

dom/ipc/ContentChild.h

@@ -711,6 +711,9 @@ class ContentChild final : public PContentChild,
 
   mozilla::ipc::IPCResult RecvDetachBrowsingContext(BrowsingContext* aContext);
 
+  mozilla::ipc::IPCResult RecvDetachBrowsingContextChildren(
+      BrowsingContext* aContext);
+
   mozilla::ipc::IPCResult RecvCacheBrowsingContextChildren(
       BrowsingContext* aContext);
 

dom/ipc/ContentParent.cpp

@@ -5834,6 +5834,33 @@ mozilla::ipc::IPCResult ContentParent::RecvCacheBrowsingContextChildren(
   return IPC_OK();
 }
 
+mozilla::ipc::IPCResult ContentParent::RecvDetachBrowsingContextChildren(
+    BrowsingContext* aContext) {
+  if (!aContext) {
+    MOZ_LOG(BrowsingContext::GetLog(), LogLevel::Debug,
+            ("ParentIPC: Trying to detach from already detached context"));
+    return IPC_OK();
+  }
+
+  if (!aContext->Canonical()->IsOwnedByProcess(ChildID())) {
+    // We're trying to detach the children of a BrowsingContext in
+    // another child process. This is illegal since the owner of the
+    // BrowsingContext is the proccess with the in-process docshell,
+    // which is tracked by OwnerProcessId.
+    MOZ_DIAGNOSTIC_ASSERT(false,
+                          "Trying to detach from out of process context");
+    return IPC_OK();
+  }
+
+  aContext->DetachChildren(/* aFromIPC */ true);
+
+  aContext->Group()->EachOtherParent(this, [&](ContentParent* aParent) {
+    Unused << aParent->SendDetachBrowsingContextChildren(aContext);
+  });
+
+  return IPC_OK();
+}
+
 mozilla::ipc::IPCResult ContentParent::RecvRestoreBrowsingContextChildren(
     BrowsingContext* aContext, BrowsingContext::Children&& aChildren) {
   if (!aContext) {

dom/ipc/ContentParent.h

@@ -626,6 +626,9 @@ class ContentParent final : public PContentParent,
 
   mozilla::ipc::IPCResult RecvDetachBrowsingContext(BrowsingContext* aContext);
 
+  mozilla::ipc::IPCResult RecvDetachBrowsingContextChildren(
+      BrowsingContext* aContext);
+
   mozilla::ipc::IPCResult RecvCacheBrowsingContextChildren(
       BrowsingContext* aContext);
 

dom/ipc/PContent.ipdl

@@ -1424,6 +1424,11 @@ both:
      */
     async DetachBrowsingContext(BrowsingContext aContext);
 
+    /**
+     * Removes all children from 'aContext'.
+     */
+    async DetachBrowsingContextChildren(BrowsingContext aContext);
+
     /**
      * Removes all of 'aContext'\'s children, and caches them in the
      * BrowsingContextGroup.