Backed out changeset c807d0b9d872 (bug 1555287) for valgrind bustages CLOSED TREE

--HG--
extra : rebase_source : e53f888399cf5eb46d3996a107d572aabfad4b97

docshell/base/BrowsingContext.cpp

@@ -210,7 +210,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(!mGroup->IsContextCached(this),
+      MOZ_DIAGNOSTIC_ASSERT(!Group()->IsContextCached(this),
                             "cannot be in bfcache");
 
       RefPtr<BrowsingContext> kungFuDeathGrip(this);
@@ -248,9 +248,7 @@ void BrowsingContext::Attach(bool aFromIPC) {
            XRE_IsParentProcess() ? "Parent" : "Child", Id(),
            mParent ? mParent->Id() : 0));
 
-  MOZ_DIAGNOSTIC_ASSERT(mGroup);
-  MOZ_DIAGNOSTIC_ASSERT(!mGroup->IsContextCached(this));
-  MOZ_DIAGNOSTIC_ASSERT(!mIsDiscarded);
+  MOZ_DIAGNOSTIC_ASSERT(!Group()->IsContextCached(this));
 
   auto* children = mParent ? &mParent->mChildren : &mGroup->Toplevels();
   MOZ_DIAGNOSTIC_ASSERT(!children->Contains(this));
@@ -264,7 +262,7 @@ void BrowsingContext::Attach(bool aFromIPC) {
           GetIPCInitializer());
     } else if (IsContent()) {
       MOZ_DIAGNOSTIC_ASSERT(XRE_IsParentProcess());
-      mGroup->EachParent([&](ContentParent* aParent) {
+      Group()->EachParent([&](ContentParent* aParent) {
         Unused << aParent->SendAttachBrowsingContext(GetIPCInitializer());
       });
     }
@@ -277,25 +275,36 @@ 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 (!mGroup->EvictCachedContext(this)) {
+  if (Group() && !Group()->EvictCachedContext(this)) {
     Children* children = nullptr;
     if (mParent) {
       children = &mParent->mChildren;
-    } else {
+    } else if (mGroup) {
       children = &mGroup->Toplevels();
     }
 
-    children->RemoveElement(this);
+    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);
+    }
   }
 
-  Unregister();
+  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;
 
   if (!aFromIPC && XRE_IsContentProcess()) {
     auto cc = ContentChild::GetSingleton();
@@ -304,32 +313,6 @@ 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",
@@ -355,7 +338,7 @@ void BrowsingContext::CacheChildren(bool aFromIPC) {
           ("%s: Caching children of 0x%08" PRIx64 "",
            XRE_IsParentProcess() ? "Parent" : "Child", Id()));
 
-  mGroup->CacheContexts(mChildren);
+  Group()->CacheContexts(mChildren);
   mChildren.Clear();
 
   if (!aFromIPC && XRE_IsContentProcess()) {
@@ -372,7 +355,7 @@ void BrowsingContext::RestoreChildren(Children&& aChildren, bool aFromIPC) {
 
   for (BrowsingContext* child : aChildren) {
     MOZ_DIAGNOSTIC_ASSERT(child->GetParent() == this);
-    Unused << mGroup->EvictCachedContext(child);
+    Unused << Group()->EvictCachedContext(child);
   }
 
   mChildren.AppendElements(aChildren);
@@ -384,7 +367,7 @@ void BrowsingContext::RestoreChildren(Children&& aChildren, bool aFromIPC) {
   }
 }
 
-bool BrowsingContext::IsCached() { return mGroup->IsContextCached(this); }
+bool BrowsingContext::IsCached() { return Group()->IsContextCached(this); }
 
 bool BrowsingContext::HasOpener() const {
   return sBrowsingContexts->Contains(mOpenerId);
@@ -548,12 +531,6 @@ 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));
@@ -693,7 +670,7 @@ void BrowsingContext::Blur(ErrorResult& aError) {
 }
 
 Nullable<WindowProxyHolder> BrowsingContext::GetTop(ErrorResult& aError) {
-  if (mIsDiscarded) {
+  if (mClosed) {
     return nullptr;
   }
 
@@ -717,7 +694,7 @@ void BrowsingContext::GetOpener(JSContext* aCx,
 }
 
 Nullable<WindowProxyHolder> BrowsingContext::GetParent(ErrorResult& aError) {
-  if (mIsDiscarded) {
+  if (mClosed) {
     return nullptr;
   }
 

docshell/base/BrowsingContext.h

@@ -148,10 +148,6 @@ 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);
@@ -384,9 +380,6 @@ 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.
@@ -468,10 +461,6 @@ 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,8 +541,6 @@ void nsDocShell::DestroyChildren() {
   }
 
   nsDocLoader::DestroyChildren();
-
-  mBrowsingContext->DetachChildren();
 }
 
 NS_IMPL_CYCLE_COLLECTION_INHERITED(nsDocShell, nsDocLoader,

dom/ipc/ContentChild.cpp

@@ -3855,15 +3855,6 @@ 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,9 +711,6 @@ 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,33 +5834,6 @@ 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,9 +626,6 @@ 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,11 +1424,6 @@ 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.