ma1 pushed to branch base-browser-128.3.0esr-14.0-1 at The Tor Project / Applications / Tor Browser

Commits: eda9f7de by Nika Layzell at 2024-10-01T22:29:35+02:00 Bug 1911745 - Unify BrowsingContext flag coherency checks, r=mccr8

Previously these checks were largely diagnostic tools for finding bugs in other code as it evolves. This unifies the checks a bit more and makes them stronger for BrowsingContexts created over IPC, providing a place for more coherency checks to be added in the future.

Differential Revision: https://phabricator.services.mozilla.com/D218860 - - - - -

2 changed files:

- docshell/base/BrowsingContext.cpp - docshell/base/BrowsingContext.h

Changes:

===================================== docshell/base/BrowsingContext.cpp ===================================== @@ -578,9 +578,20 @@ mozilla::ipc::IPCResult BrowsingContext::CreateFromIPC( context->mRequestContextId = aInit.mRequestContextId; // NOTE: Private browsing ID is set by `SetOriginAttributes`.

+ if (const char* failure = + context->BrowsingContextCoherencyChecks(aOriginProcess)) { + mozilla::ipc::IProtocol* actor = aOriginProcess; + if (!actor) { + actor = ContentChild::GetSingleton(); + } + return IPC_FAIL_UNSAFE_PRINTF(actor, "Incoherent BrowsingContext: %s", + failure); + } + Register(context);

- return context->Attach(/* aFromIPC */ true, aOriginProcess); + context->Attach(/* aFromIPC */ true, aOriginProcess); + return IPC_OK(); }

BrowsingContext::BrowsingContext(WindowContext* aParentWindow, @@ -795,8 +806,64 @@ void BrowsingContext::Embed() { } }

-mozilla::ipc::IPCResult BrowsingContext::Attach(bool aFromIPC, - ContentParent* aOriginProcess) { +const char* BrowsingContext::BrowsingContextCoherencyChecks( + ContentParent* aOriginProcess) { +#define COHERENCY_ASSERT(condition) \ + if (!(condition)) return "Assertion " #condition " failed"; + + if (mGroup->IsPotentiallyCrossOriginIsolated() != + (Top()->GetOpenerPolicy() == + nsILoadInfo::OPENER_POLICY_SAME_ORIGIN_EMBEDDER_POLICY_REQUIRE_CORP)) { + return "Invalid CrossOriginIsolated state"; + } + + if (aOriginProcess && !IsContent()) { + return "Content cannot create chrome BCs"; + } + + // LoadContext should generally match our opener or parent. + if (IsContent()) { + if (RefPtr<BrowsingContext> opener = GetOpener()) { + COHERENCY_ASSERT(opener->mType == mType); + COHERENCY_ASSERT(opener->mGroup == mGroup); + COHERENCY_ASSERT(opener->mUseRemoteTabs == mUseRemoteTabs); + COHERENCY_ASSERT(opener->mUseRemoteSubframes == mUseRemoteSubframes); + COHERENCY_ASSERT(opener->mPrivateBrowsingId == mPrivateBrowsingId); + COHERENCY_ASSERT( + opener->mOriginAttributes.EqualsIgnoringFPD(mOriginAttributes)); + } + } + if (RefPtr<BrowsingContext> parent = GetParent()) { + COHERENCY_ASSERT(parent->mType == mType); + COHERENCY_ASSERT(parent->mGroup == mGroup); + COHERENCY_ASSERT(parent->mUseRemoteTabs == mUseRemoteTabs); + COHERENCY_ASSERT(parent->mUseRemoteSubframes == mUseRemoteSubframes); + COHERENCY_ASSERT(parent->mPrivateBrowsingId == mPrivateBrowsingId); + COHERENCY_ASSERT( + parent->mOriginAttributes.EqualsIgnoringFPD(mOriginAttributes)); + } + + // UseRemoteSubframes and UseRemoteTabs must match. + if (mUseRemoteSubframes && !mUseRemoteTabs) { + return "Cannot set useRemoteSubframes without also setting useRemoteTabs"; + } + + // Double-check OriginAttributes/Private Browsing + // Chrome browsing contexts must not have a private browsing OriginAttribute + // Content browsing contexts must maintain the equality: + // mOriginAttributes.mPrivateBrowsingId == mPrivateBrowsingId + if (IsChrome()) { + COHERENCY_ASSERT(mOriginAttributes.mPrivateBrowsingId == 0); + } else { + COHERENCY_ASSERT(mOriginAttributes.mPrivateBrowsingId == + mPrivateBrowsingId); + } +#undef COHERENCY_ASSERT + + return nullptr; +} + +void BrowsingContext::Attach(bool aFromIPC, ContentParent* aOriginProcess) { MOZ_DIAGNOSTIC_ASSERT(!mEverAttached); MOZ_DIAGNOSTIC_ASSERT_IF(aFromIPC, aOriginProcess || XRE_IsContentProcess()); mEverAttached = true; @@ -815,25 +882,15 @@ mozilla::ipc::IPCResult BrowsingContext::Attach(bool aFromIPC, MOZ_DIAGNOSTIC_ASSERT(mGroup); MOZ_DIAGNOSTIC_ASSERT(!mIsDiscarded);

- if (mGroup->IsPotentiallyCrossOriginIsolated() != - (Top()->GetOpenerPolicy() == - nsILoadInfo::OPENER_POLICY_SAME_ORIGIN_EMBEDDER_POLICY_REQUIRE_CORP)) { - MOZ_DIAGNOSTIC_ASSERT(aFromIPC); - if (aFromIPC) { - auto* actor = aOriginProcess - ? static_castmozilla::ipc::IProtocol*(aOriginProcess) - : static_castmozilla::ipc::IProtocol*( - ContentChild::GetSingleton()); - return IPC_FAIL( - actor, - "Invalid CrossOriginIsolated state in BrowsingContext::Attach call"); - } else { - MOZ_CRASH( - "Invalid CrossOriginIsolated state in BrowsingContext::Attach call"); +#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED + // We'll already have checked this if `aFromIPC` is set before calling this + // function. + if (!aFromIPC) { + if (const char* failure = BrowsingContextCoherencyChecks(aOriginProcess)) { + MOZ_CRASH_UNSAFE_PRINTF("Incoherent BrowsingContext: %s", failure); } } - - AssertCoherentLoadContext(); +#endif

// Add ourselves either to our parent or BrowsingContextGroup's child list. // Important: We shouldn't return IPC_FAIL after this point, since the @@ -915,7 +972,6 @@ mozilla::ipc::IPCResult BrowsingContext::Attach(bool aFromIPC, if (XRE_IsParentProcess()) { Canonical()->CanonicalAttach(); } - return IPC_OK(); }

void BrowsingContext::Detach(bool aFromIPC) { @@ -1768,40 +1824,6 @@ nsresult BrowsingContext::SetOriginAttributes(const OriginAttributes& aAttrs) { return NS_OK; }

-void BrowsingContext::AssertCoherentLoadContext() { -#ifdef MOZ_DIAGNOSTIC_ASSERT_ENABLED - // LoadContext should generally match our opener or parent. - if (IsContent()) { - if (RefPtr<BrowsingContext> opener = GetOpener()) { - MOZ_DIAGNOSTIC_ASSERT(opener->mType == mType); - MOZ_DIAGNOSTIC_ASSERT(opener->mGroup == mGroup); - MOZ_DIAGNOSTIC_ASSERT(opener->mUseRemoteTabs == mUseRemoteTabs); - MOZ_DIAGNOSTIC_ASSERT(opener->mUseRemoteSubframes == mUseRemoteSubframes); - MOZ_DIAGNOSTIC_ASSERT(opener->mPrivateBrowsingId == mPrivateBrowsingId); - MOZ_DIAGNOSTIC_ASSERT( - opener->mOriginAttributes.EqualsIgnoringFPD(mOriginAttributes)); - } - } - if (RefPtr<BrowsingContext> parent = GetParent()) { - MOZ_DIAGNOSTIC_ASSERT(parent->mType == mType); - MOZ_DIAGNOSTIC_ASSERT(parent->mGroup == mGroup); - MOZ_DIAGNOSTIC_ASSERT(parent->mUseRemoteTabs == mUseRemoteTabs); - MOZ_DIAGNOSTIC_ASSERT(parent->mUseRemoteSubframes == mUseRemoteSubframes); - MOZ_DIAGNOSTIC_ASSERT(parent->mPrivateBrowsingId == mPrivateBrowsingId); - MOZ_DIAGNOSTIC_ASSERT( - parent->mOriginAttributes.EqualsIgnoringFPD(mOriginAttributes)); - } - - // UseRemoteSubframes and UseRemoteTabs must match. - MOZ_DIAGNOSTIC_ASSERT( - !mUseRemoteSubframes || mUseRemoteTabs, - "Cannot set useRemoteSubframes without also setting useRemoteTabs"); - - // Double-check OriginAttributes/Private Browsing - AssertOriginAttributesMatchPrivateBrowsing(); -#endif -} - void BrowsingContext::AssertOriginAttributesMatchPrivateBrowsing() { // Chrome browsing contexts must not have a private browsing OriginAttribute // Content browsing contexts must maintain the equality:

===================================== docshell/base/BrowsingContext.h ===================================== @@ -984,7 +984,18 @@ class BrowsingContext : public nsILoadContext, public nsWrapperCache { bool aHasPostData);

private: - mozilla::ipc::IPCResult Attach(bool aFromIPC, ContentParent* aOriginProcess); + // Assert that this BrowsingContext is coherent relative to related + // BrowsingContexts. This will be run before the BrowsingContext is attached. + // + // A non-null string return value indicates that there was a coherency check + // failure, which will be handled with either a crash or IPC failure. + // + // If provided, `aOriginProcess` is the process which is responsible for the + // creation of this BrowsingContext. + [[nodiscard]] const char* BrowsingContextCoherencyChecks( + ContentParent* aOriginProcess); + + void Attach(bool aFromIPC, ContentParent* aOriginProcess);

// Recomputes whether we can execute scripts in this BrowsingContext based on // the value of AllowJavascript() and whether scripts are allowed in the @@ -998,10 +1009,6 @@ class BrowsingContext : public nsILoadContext, public nsWrapperCache {

void AssertOriginAttributesMatchPrivateBrowsing();

- // Assert that the BrowsingContext's LoadContext flags appear coherent - // relative to related BrowsingContexts. - void AssertCoherentLoadContext(); - friend class ::nsOuterWindowProxy; friend class ::nsGlobalWindowOuter; friend class WindowContext;

View it on GitLab: https://gitlab.torproject.org/tpo/applications/tor-browser/-/commit/eda9f7de...