Bug 1563542 - Correctly align usage of mIsDiscarded and mClosed for BrowsingContext, r=peterv a=RyanVM DEVEDITION_69_0b3_BUILD1 DEVEDITION_69_0b3_RELEASE FIREFOX_69_0b3_BUILD1 FIREFOX_69_0b3_RELEASE
authorNika Layzell <nika@thelayzells.com>
Mon, 08 Jul 2019 17:27:27 +0000
changeset 544438 9cdcc170dc1a51fc14a10d38f8aa2237d9928ba4
parent 544437 f4452e031aedcf554ecb556d372bf6a5c773048b
child 544439 446a160a8c4e68d29b1f3a2b6a285c3f6b7f81f7
push id2131
push userffxbld-merge
push dateMon, 26 Aug 2019 18:30:20 +0000
treeherdermozilla-release@b19ffb3ca153 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspeterv, RyanVM
bugs1563542
milestone69.0
Bug 1563542 - Correctly align usage of mIsDiscarded and mClosed for BrowsingContext, r=peterv a=RyanVM 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
docshell/base/BrowsingContext.cpp
docshell/base/BrowsingContext.h
dom/base/nsFrameLoader.cpp
dom/ipc/WindowGlobalChild.cpp
--- a/docshell/base/BrowsingContext.cpp
+++ b/docshell/base/BrowsingContext.cpp
@@ -60,16 +60,20 @@ static void Register(BrowsingContext* aB
 
   aBrowsingContext->Group()->Register(aBrowsingContext);
 }
 
 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() {
   BrowsingContext* bc = this;
   while (bc->mParent) {
     bc = bc->mParent;
   }
   return bc;
@@ -322,17 +326,17 @@ void BrowsingContext::Detach(bool aFromI
 }
 
 void BrowsingContext::PrepareForProcessChange() {
   MOZ_LOG(GetLog(), LogLevel::Debug,
           ("%s: Preparing 0x%08" PRIx64 " for a process change",
            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;
 
   // XXX: We should transplant our WindowProxy into a Cross-Process WindowProxy
   // if mWindowProxy is non-nullptr. (bug 1510760)
   mWindowProxy = nullptr;
 
   // NOTE: For now, clear our nsDocShell reference, as we're primarily in a
--- a/docshell/base/BrowsingContext.h
+++ b/docshell/base/BrowsingContext.h
@@ -121,16 +121,21 @@ class BrowsingContext : public nsWrapper
   // Cast this object to a canonical browsing context, and return it.
   CanonicalBrowsingContext* Canonical();
 
   // Is the most recent Document in this BrowsingContext loaded within this
   // process? This may be true with a null mDocShell after the Window has been
   // 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; }
   void SetDocShell(nsIDocShell* aDocShell);
   void ClearDocShell() { mDocShell = nullptr; }
 
   // Get the embedder element for this BrowsingContext if the embedder is
   // in-process, or null if it's not.
--- a/dom/base/nsFrameLoader.cpp
+++ b/dom/base/nsFrameLoader.cpp
@@ -312,18 +312,18 @@ static already_AddRefed<BrowsingContext>
   RefPtr<nsDocShell> parentDocShell = nsDocShell::Cast(doc->GetDocShell());
 
   if (NS_WARN_IF(!parentDocShell)) {
     return nullptr;
   }
 
   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;
   }
 
   // Determine the frame name for the new browsing context.
   nsAutoString frameName;
   GetFrameName(aOwner, frameName);
 
   if (IsTopContent(parentContext, aOwner)) {
--- a/dom/ipc/WindowGlobalChild.cpp
+++ b/dom/ipc/WindowGlobalChild.cpp
@@ -65,19 +65,19 @@ already_AddRefed<WindowGlobalChild> Wind
       do_QueryInterface(aWindow->GetDocument()->GetChannel());
   nsILoadInfo::CrossOriginOpenerPolicy policy;
   if (chan && NS_SUCCEEDED(chan->GetCrossOriginOpenerPolicy(&policy))) {
     bc->SetOpenerPolicy(policy);
   }
 
   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();
   }
 
   WindowGlobalInit init(principal, aWindow->GetDocumentURI(), bc,
                         wgc->mInnerWindowId, wgc->mOuterWindowId);
 
   // Send the link constructor over PInProcessChild or PBrowser.