From cbb7e8111c33dee75bd406d4d44e9ae0c965e4ab Mon Sep 17 00:00:00 2001 From: Nika Layzell Date: Wed, 16 Sep 2020 20:47:55 +0000 Subject: [PATCH] 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 | 8 +++++++- docshell/base/BrowsingContext.h | 19 +++++++++++++------ docshell/base/nsDocShell.cpp | 8 +++++++- dom/ipc/ContentChild.cpp | 8 +++++--- .../windowwatcher/nsWindowWatcher.cpp | 6 ++++++ 5 files changed, 38 insertions(+), 11 deletions(-) diff --git a/docshell/base/BrowsingContext.cpp b/docshell/base/BrowsingContext.cpp index 5bdb5c84aa99..13efe69cfba4 100644 --- a/docshell/base/BrowsingContext.cpp +++ b/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, const uint32_t& aValue, return GetBrowserId() == 0 && IsTop() && Children().IsEmpty(); } +bool BrowsingContext::CanSet(FieldIndex, + 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()) { diff --git a/docshell/base/BrowsingContext.h b/docshell/base/BrowsingContext.h index abb1f44ce961..8702dc3b08f5 100644 --- a/docshell/base/BrowsingContext.h +++ b/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 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( + aPendingInitialization); + } const OriginAttributes& OriginAttributesRef() { return mOriginAttributes; } nsresult SetOriginAttributes(const OriginAttributes& aAttrs); @@ -839,6 +847,9 @@ class BrowsingContext : public nsILoadContext, public nsWrapperCache { bool CanSet(FieldIndex, const bool& aUseErrorPages, ContentParent* aSource); + bool CanSet(FieldIndex, bool aNewValue, + ContentParent* aSource); + template bool CanSet(FieldIndex, 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; diff --git a/docshell/base/nsDocShell.cpp b/docshell/base/nsDocShell.cpp index 2992db0b5975..e6cd4a181075 100644 --- a/docshell/base/nsDocShell.cpp +++ b/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; } diff --git a/dom/ipc/ContentChild.cpp b/dom/ipc/ContentChild.cpp index 3c2a4e058436..28efe89bf958 100644 --- a/dom/ipc/ContentChild.cpp +++ b/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 diff --git a/toolkit/components/windowwatcher/nsWindowWatcher.cpp b/toolkit/components/windowwatcher/nsWindowWatcher.cpp index 2fec933fd165..6d4958c5cabb 100644 --- a/toolkit/components/windowwatcher/nsWindowWatcher.cpp +++ b/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.