Bug 1563542 - Correctly align usage of mIsDiscarded and mClosed for BrowsingContext, r=peterv

In the bug which introduced mIsDiscarded, the code was changed to not set
mClosed during Detach, and only set mIsDiscarded. This was a mistake because a
bunch of places are only reading mClosed. Specifically when creating a
BrowsingContext for an iframe, we check GetClosed() to see whether to skip
creating it. Not doing this check can lead to assertions like the one in this
bug.

This patch changes the behaviour to continue setting `mClosed`, and also updates
the relevant `GetClosed()` checks to correctly check `IsDiscarded()`

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

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

docshell/base/BrowsingContext.cpp

@@ -65,6 +65,10 @@ void BrowsingContext::Unregister() {
   MOZ_DIAGNOSTIC_ASSERT(mGroup);
   mGroup->Unregister(this);
   mIsDiscarded = true;
+
+  // NOTE: Doesn't use SetClosed, as it will be set in all processes
+  // automatically by calls to Detach()
+  mClosed = true;
 }
 
 BrowsingContext* BrowsingContext::Top() {
@@ -327,7 +331,7 @@ void BrowsingContext::PrepareForProcessChange() {
            XRE_IsParentProcess() ? "Parent" : "Child", Id()));
 
   MOZ_ASSERT(mIsInProcess, "Must currently be an in-process frame");
-  MOZ_ASSERT(!mClosed, "We're already closed?");
+  MOZ_ASSERT(!mIsDiscarded, "We're already closed?");
 
   mIsInProcess = false;
 

docshell/base/BrowsingContext.h

@@ -126,6 +126,11 @@ class BrowsingContext : public nsWrapperCache, public BrowsingContextBase {
   // closed.
   bool IsInProcess() const { return mIsInProcess; }
 
+  // Has this BrowsingContext been discarded. A discarded browsing context has
+  // been destroyed, and may not be available on the other side of an IPC
+  // message.
+  bool IsDiscarded() const { return mIsDiscarded; }
+
   // Get the DocShell for this BrowsingContext if it is in-process, or
   // null if it's not.
   nsIDocShell* GetDocShell() { return mDocShell; }

dom/base/nsFrameLoader.cpp

@@ -317,8 +317,8 @@ static already_AddRefed<BrowsingContext> CreateBrowsingContext(
 
   RefPtr<BrowsingContext> parentContext = parentDocShell->GetBrowsingContext();
 
-  // Don't create a child docshell for a closed browsing context.
-  if (NS_WARN_IF(!parentContext) || parentContext->GetClosed()) {
+  // Don't create a child docshell for a discarded browsing context.
+  if (NS_WARN_IF(!parentContext) || parentContext->IsDiscarded()) {
     return nullptr;
   }
 

dom/ipc/WindowGlobalChild.cpp

@@ -70,9 +70,9 @@ already_AddRefed<WindowGlobalChild> WindowGlobalChild::Create(
 
   RefPtr<WindowGlobalChild> wgc = new WindowGlobalChild(aWindow, bc);
 
-  // If we have already closed our browsing context, return a pre-closed
+  // If we have already closed our browsing context, return a pre-destroyed
   // WindowGlobalChild actor.
-  if (bc->GetClosed()) {
+  if (bc->IsDiscarded()) {
     wgc->ActorDestroy(FailedConstructor);
     return wgc.forget();
   }