[tor-commits] [Git][tpo/applications/tor-browser][base-browser-128.3.0esr-14.0-1] Bug 1911745 - Unify BrowsingContext flag coherency checks, r=mccr8

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

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_cast<mozilla::ipc::IProtocol*>(aOriginProcess)
 -                        : static_cast<mozilla::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;