Bug 1659696 - Check PendingInitialization before targeting in window.open, r=kmag

This requires adding the flag as a synced field on the BrowsingContext, and
checking it in a few more places. Attempts to open a new window in this racy
manner will now raise an exception.

This should avoid the issue from bug 1658854 by blocking the buggy attempts to
load before the nested event loop has been exited.

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

docshell/base/BrowsingContext.cpp

@@ -445,7 +445,6 @@ BrowsingContext::BrowsingContext(WindowContext* aParentWindow,
       mIsDiscarded(false),
       mWindowless(false),
       mDanglingRemoteOuterProxies(false),
-      mPendingInitialization(false),
       mEmbeddedByThisProcess(false),
       mUseRemoteTabs(false),
       mUseRemoteSubframes(false) {
@@ -2620,6 +2619,13 @@ bool BrowsingContext::CanSet(FieldIndex<IDX_BrowserId>, const uint32_t& aValue,
   return GetBrowserId() == 0 && IsTop() && Children().IsEmpty();
 }
 
+bool BrowsingContext::CanSet(FieldIndex<IDX_PendingInitialization>,
+                             bool aNewValue, ContentParent* aSource) {
+  // Can only be cleared from `true` to `false`, and should only ever be set on
+  // the toplevel BrowsingContext.
+  return IsTop() && GetPendingInitialization() && !aNewValue;
+}
+
 void BrowsingContext::SessionHistoryChanged(int32_t aIndexDelta,
                                             int32_t aLengthDelta) {
   if (XRE_IsParentProcess() || StaticPrefs::fission_sessionHistoryInParent()) {

docshell/base/BrowsingContext.h

@@ -89,6 +89,9 @@ class WindowProxyHolder;
   FIELD(Name, nsString)                                                      \
   FIELD(Closed, bool)                                                        \
   FIELD(IsActive, bool)                                                      \
+  /* If true, we're within the nested event loop in window.open, and this    \
+   * context may not be used as the target of a load */                      \
+  FIELD(PendingInitialization, bool)                                         \
   FIELD(OpenerPolicy, nsILoadInfo::CrossOriginOpenerPolicy)                  \
   /* Current opener for the BrowsingContext. Weak reference */               \
   FIELD(OpenerId, uint64_t)                                                  \
@@ -624,8 +627,13 @@ class BrowsingContext : public nsILoadContext, public nsWrapperCache {
 
   RefPtr<SessionStorageManager> GetSessionStorageManager();
 
-  bool PendingInitialization() const { return mPendingInitialization; };
-  void SetPendingInitialization(bool aVal) { mPendingInitialization = aVal; };
+  // Set PendingInitialization on this BrowsingContext before the context has
+  // been attached.
+  void InitPendingInitialization(bool aPendingInitialization) {
+    MOZ_ASSERT(!EverAttached());
+    mFields.SetWithoutSyncing<IDX_PendingInitialization>(
+        aPendingInitialization);
+  }
 
   const OriginAttributes& OriginAttributesRef() { return mOriginAttributes; }
   nsresult SetOriginAttributes(const OriginAttributes& aAttrs);
@@ -839,6 +847,9 @@ class BrowsingContext : public nsILoadContext, public nsWrapperCache {
   bool CanSet(FieldIndex<IDX_UseErrorPages>, const bool& aUseErrorPages,
               ContentParent* aSource);
 
+  bool CanSet(FieldIndex<IDX_PendingInitialization>, bool aNewValue,
+              ContentParent* aSource);
+
   template <size_t I, typename T>
   bool CanSet(FieldIndex<I>, const T&, ContentParent*) {
     return true;
@@ -927,10 +938,6 @@ class BrowsingContext : public nsILoadContext, public nsWrapperCache {
   // process, and might have remote window proxies that need to be cleaned up.
   bool mDanglingRemoteOuterProxies : 1;
 
-  // If true, the docShell has not been fully initialized, and may not be used
-  // as the target of a load.
-  bool mPendingInitialization : 1;
-
   // True if this BrowsingContext has been embedded in a element in this
   // process.
   bool mEmbeddedByThisProcess : 1;

docshell/base/nsDocShell.cpp

@@ -8584,6 +8584,12 @@ nsresult nsDocShell::PerformRetargeting(nsDocShellLoadState* aLoadState) {
   NS_ENSURE_SUCCESS(rv, rv);
   NS_ENSURE_TRUE(targetContext, rv);
 
+  // If our target BrowsingContext is still pending initialization, ignore the
+  // navigation request targeting it.
+  if (NS_WARN_IF(targetContext->GetPendingInitialization())) {
+    return NS_OK;
+  }
+
   aLoadState->SetTargetBrowsingContext(targetContext);
   //
   // Transfer the load to the target BrowsingContext... Clear the window target
@@ -9001,7 +9007,7 @@ nsresult nsDocShell::InternalLoad(nsDocShellLoadState* aLoadState,
     MOZ_ASSERT(false, "InternalLoad needs a valid triggeringPrincipal");
     return NS_ERROR_FAILURE;
   }
-  if (mBrowsingContext->PendingInitialization()) {
+  if (NS_WARN_IF(mBrowsingContext->GetPendingInitialization())) {
     return NS_ERROR_NOT_AVAILABLE;
   }
 

dom/ipc/ContentChild.cpp

@@ -950,13 +950,14 @@ nsresult ContentChild::ProvideWindowCommon(
   MOZ_ALWAYS_SUCCEEDS(browsingContext->SetRemoteSubframes(useRemoteSubframes));
   MOZ_ALWAYS_SUCCEEDS(browsingContext->SetOriginAttributes(
       aOpenWindowInfo->GetOriginAttributes()));
-  browsingContext->EnsureAttached();
 
-  browsingContext->SetPendingInitialization(true);
+  browsingContext->InitPendingInitialization(true);
   auto unsetPending = MakeScopeExit([browsingContext]() {
-    browsingContext->SetPendingInitialization(false);
+    Unused << browsingContext->SetPendingInitialization(false);
   });
 
+  browsingContext->EnsureAttached();
+
   // The initial about:blank document we generate within the nsDocShell will
   // almost certainly be replaced at some point. Unfortunately, getting the
   // principal right here causes bugs due to frame scripts not getting events
@@ -1073,6 +1074,7 @@ nsresult ContentChild::ProvideWindowCommon(
       MOZ_DIAGNOSTIC_ASSERT(browsingContext->GetOpenerId() == 0);
     } else {
       MOZ_DIAGNOSTIC_ASSERT(browsingContext->HadOriginalOpener());
+      MOZ_DIAGNOSTIC_ASSERT(browsingContext->GetOpenerId() == parent->Id());
     }
 
     // Unfortunately we don't get a window unless we've shown the frame.  That's

toolkit/components/windowwatcher/nsWindowWatcher.cpp

@@ -676,6 +676,12 @@ nsresult nsWindowWatcher::OpenWindowInternal(
     return NS_ERROR_DOM_INVALID_ACCESS_ERR;
   }
 
+  // If our target BrowsingContext is still pending initialization, ignore the
+  // navigation request targeting it.
+  if (newBC && NS_WARN_IF(newBC->GetPendingInitialization())) {
+    return NS_ERROR_ABORT;
+  }
+
   // no extant window? make a new one.
 
   // If no parent, consider it chrome when running in the parent process.