Bug 1691410 - Add support for reverting racy changes in CanSet, r=kmag

In some cases, a content process may think they should be able to make a change
to a synced field, but in the meantime something in the parent process has
changed and the change can no longer be applied. This was the cause of a number
of issues around the in-flight process ID, and can cause issues such as crashes
if the CanSet method was made too strict.

This patch introduces a new possible return type from `CanSet` which allows
requesting a `Revert`. A reverted field change will either be cancelled at the
source (if the CanSet fails in the setting process), or will be cancelled by
sending a new transaction back to the source process reverting the change to
ensure consistency.

In addition, some additional logging is added which made it easier to locate the
underlying bug and verify the correctness of the change.

The current primary use-case for this new feature is the CurrentInnerWindowId
field which can be updated by the previous process' docshell after the parent
process has already performed a switch to a new process. This can lead to the
current WindowContext being inaccurate for a BrowsingContext in some edge cases
as we allow the flawed set due the in-flight process ID matching.

This patch changes the logic to no longer check the in-flight process ID, and
instead revert any changes to the CurrentInnerWindowId field coming from a
process which is not currently active in the BrowsingContext.

No tests were added as it is very timing-sensitive, and difficult to create the
specific scenario, however without these changes my patch for bug 1663757
consistently causes geckoview-junit crashes due to currentWindowGlobal being
incorrect.

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

docshell/base/BrowsingContext.cpp

@@ -132,6 +132,7 @@ extern mozilla::LazyLogModule gUserInteractionPRLog;
   MOZ_LOG(gUserInteractionPRLog, LogLevel::Debug, (msg, ##__VA_ARGS__))
 
 static LazyLogModule gBrowsingContextLog("BrowsingContext");
+static LazyLogModule gBrowsingContextSyncLog("BrowsingContextSync");
 
 typedef nsDataHashtable<nsUint64HashKey, BrowsingContext*> BrowsingContextMap;
 
@@ -224,6 +225,9 @@ void BrowsingContext::Init() {
 /* static */
 LogModule* BrowsingContext::GetLog() { return gBrowsingContextLog; }
 
+/* static */
+LogModule* BrowsingContext::GetSyncLog() { return gBrowsingContextSyncLog; }
+
 /* static */
 already_AddRefed<BrowsingContext> BrowsingContext::Get(uint64_t aId) {
   return do_AddRef(sBrowsingContexts->Get(aId));
@@ -2831,37 +2835,32 @@ bool BrowsingContext::CanSet(FieldIndex<IDX_EmbedderElementType>,
   return CheckOnlyEmbedderCanSet(aSource);
 }
 
-bool BrowsingContext::CanSet(FieldIndex<IDX_CurrentInnerWindowId>,
-                             const uint64_t& aValue, ContentParent* aSource) {
+auto BrowsingContext::CanSet(FieldIndex<IDX_CurrentInnerWindowId>,
+                             const uint64_t& aValue, ContentParent* aSource)
+    -> CanSetResult {
   // Generally allow clearing this. We may want to be more precise about this
   // check in the future.
   if (aValue == 0) {
-    return true;
-  }
-
-  if (aSource) {
-    MOZ_ASSERT(XRE_IsParentProcess());
-
-    // If in the parent process, double-check ownership and WindowGlobalParent
-    // as well.
-    RefPtr<WindowGlobalParent> wgp =
-        WindowGlobalParent::GetByInnerWindowId(aValue);
-    if (NS_WARN_IF(!wgp) || NS_WARN_IF(wgp->BrowsingContext() != this)) {
-      return false;
-    }
-
-    // Double-check ownership if we aren't the setter.
-    if (!Canonical()->IsOwnedByProcess(aSource->ChildID()) &&
-        aSource->ChildID() != Canonical()->GetInFlightProcessId()) {
-      return false;
-    }
-  } else if (XRE_IsContentProcess() && !IsOwnedByProcess()) {
-    return false;
+    return CanSetResult::Allow;
   }
 
   // We must have access to the specified context.
   RefPtr<WindowContext> window = WindowContext::GetById(aValue);
-  return window && window->GetBrowsingContext() == this;
+  if (!window || window->GetBrowsingContext() != this) {
+    return CanSetResult::Deny;
+  }
+
+  if (aSource) {
+    // If the sending process is no longer the current owner, revert
+    MOZ_ASSERT(XRE_IsParentProcess());
+    if (!Canonical()->IsOwnedByProcess(aSource->ChildID())) {
+      return CanSetResult::Revert;
+    }
+  } else if (XRE_IsContentProcess() && !IsOwnedByProcess()) {
+    return CanSetResult::Deny;
+  }
+
+  return CanSetResult::Allow;
 }
 
 void BrowsingContext::DidSet(FieldIndex<IDX_CurrentInnerWindowId>) {

docshell/base/BrowsingContext.h

@@ -223,6 +223,7 @@ class BrowsingContext : public nsILoadContext, public nsWrapperCache {
 
   static void Init();
   static LogModule* GetLog();
+  static LogModule* GetSyncLog();
 
   // Look up a BrowsingContext in the current process by ID.
   static already_AddRefed<BrowsingContext> Get(uint64_t aId);
@@ -885,6 +886,8 @@ class BrowsingContext : public nsILoadContext, public nsWrapperCache {
   void SendCommitTransaction(ContentChild* aChild, const BaseTransaction& aTxn,
                              uint64_t aEpoch);
 
+  using CanSetResult = syncedcontext::CanSetResult;
+
   // Ensure that opener is in the same BrowsingContextGroup.
   bool CanSet(FieldIndex<IDX_OpenerId>, const uint64_t& aValue,
               ContentParent* aSource) {
@@ -947,8 +950,8 @@ class BrowsingContext : public nsILoadContext, public nsWrapperCache {
   bool CanSet(FieldIndex<IDX_EmbedderInnerWindowId>, const uint64_t& aValue,
               ContentParent* aSource);
 
-  bool CanSet(FieldIndex<IDX_CurrentInnerWindowId>, const uint64_t& aValue,
-              ContentParent* aSource);
+  CanSetResult CanSet(FieldIndex<IDX_CurrentInnerWindowId>,
+                      const uint64_t& aValue, ContentParent* aSource);
 
   void DidSet(FieldIndex<IDX_CurrentInnerWindowId>);
 

docshell/base/SyncedContext.h

@@ -85,10 +85,12 @@ class Transaction {
   // `Commit`, which will perform the necessary synchronization.
   //
   // `Validate` must be called before calling this method.
-  void Apply(Context* aOwner);
+  void Apply(Context* aOwner, bool aFromIPC);
 
   // Returns the set of fields which failed to validate, or an empty set if
   // there were no validation errors.
+  //
+  // NOTE: This method mutates `this` if any changes were reverted.
   IndexSet Validate(Context* aOwner, ContentParent* aSource);
 
   template <typename F>
@@ -193,6 +195,17 @@ class FieldStorage {
   Values mValues;
 };
 
+// Alternative return type enum for `CanSet` validators which allows specifying
+// more behaviour.
+enum class CanSetResult : uint8_t {
+  // The set attempt is denied. This is equivalent to returning `false`.
+  Deny,
+  // The set attempt is allowed. This is equivalent to returning `true`.
+  Allow,
+  // The set attempt is reverted non-fatally.
+  Revert,
+};
+
 // Helper type traits to use concrete types rather than generic forwarding
 // references for the `SetXXX` methods defined on the synced context type.
 //

docshell/base/SyncedContextInlines.h

@@ -17,6 +17,66 @@ namespace mozilla {
 namespace dom {
 namespace syncedcontext {
 
+template <typename T>
+struct IsMozillaMaybe : std::false_type {};
+template <typename T>
+struct IsMozillaMaybe<Maybe<T>> : std::true_type {};
+
+// A super-sketchy logging only function for generating a human-readable version
+// of the value of a synced context field.
+template <typename T>
+void FormatFieldValue(nsACString& aStr, const T& aValue) {
+  if constexpr (std::is_same_v<bool, T>) {
+    if (aValue) {
+      aStr.AppendLiteral("true");
+    } else {
+      aStr.AppendLiteral("false");
+    }
+  } else if constexpr (std::is_same_v<nsID, T>) {
+    aStr.Append(nsIDToCString(aValue).get());
+  } else if constexpr (std::is_integral_v<T>) {
+    aStr.AppendInt(aValue);
+  } else if constexpr (std::is_enum_v<T>) {
+    aStr.AppendInt(static_cast<std::underlying_type_t<T>>(aValue));
+  } else if constexpr (std::is_floating_point_v<T>) {
+    aStr.AppendFloat(aValue);
+  } else if constexpr (std::is_base_of_v<nsAString, T>) {
+    AppendUTF16toUTF8(aValue, aStr);
+  } else if constexpr (std::is_base_of_v<nsACString, T>) {
+    aStr.Append(aValue);
+  } else if constexpr (IsMozillaMaybe<T>::value) {
+    if (aValue) {
+      aStr.AppendLiteral("Some(");
+      FormatFieldValue(aStr, aValue.ref());
+      aStr.AppendLiteral(")");
+    } else {
+      aStr.AppendLiteral("Nothing");
+    }
+  } else {
+    aStr.AppendLiteral("???");
+  }
+}
+
+// Sketchy logging-only logic to generate a human-readable output of the actions
+// a synced context transaction is going to perform.
+template <typename Context>
+nsAutoCString FormatTransaction(
+    IndexSet aModified, const typename Context::FieldValues& aOldValues,
+    const typename Context::FieldValues& aNewValues) {
+  nsAutoCString result;
+  Context::FieldValues::EachIndex([&](auto idx) {
+    if (aModified.contains(idx)) {
+      result.Append(Context::FieldIndexToName(idx));
+      result.AppendLiteral("(");
+      FormatFieldValue(result, aOldValues.Get(idx));
+      result.AppendLiteral("->");
+      FormatFieldValue(result, aNewValues.Get(idx));
+      result.AppendLiteral(") ");
+    }
+  });
+  return result;
+}
+
 template <typename Context>
 nsCString FormatValidationError(IndexSet aFailedFields, const char* prefix) {
   MOZ_ASSERT(!aFailedFields.isEmpty());
@@ -40,6 +100,10 @@ nsresult Transaction<Context>::Commit(Context* aOwner) {
     MOZ_CRASH_UNSAFE_PRINTF("%s", error.get());
   }
 
+  if (mModified.isEmpty()) {
+    return NS_OK;
+  }
+
   if (XRE_IsContentProcess()) {
     ContentChild* cc = ContentChild::GetSingleton();
 
@@ -64,7 +128,7 @@ nsresult Transaction<Context>::Commit(Context* aOwner) {
     });
   }
 
-  Apply(aOwner);
+  Apply(aOwner, /* aFromIPC */ false);
   return NS_OK;
 }
 
@@ -73,7 +137,7 @@ mozilla::ipc::IPCResult Transaction<Context>::CommitFromIPC(
     const MaybeDiscarded<Context>& aOwner, ContentParent* aSource) {
   MOZ_DIAGNOSTIC_ASSERT(XRE_IsParentProcess());
   if (aOwner.IsNullOrDiscarded()) {
-    MOZ_LOG(Context::GetLog(), LogLevel::Debug,
+    MOZ_LOG(Context::GetSyncLog(), LogLevel::Debug,
             ("IPC: Trying to send a message to dead or detached context"));
     return IPC_OK();
   }
@@ -88,13 +152,19 @@ mozilla::ipc::IPCResult Transaction<Context>::CommitFromIPC(
     return IPC_FAIL(aSource, error.get());
   }
 
+  // Validate may have dropped some fields from the transaction, check it's not
+  // empty before continuing.
+  if (mModified.isEmpty()) {
+    return IPC_OK();
+  }
+
   BrowsingContextGroup* group = owner->Group();
   group->EachOtherParent(aSource, [&](ContentParent* aParent) {
     owner->SendCommitTransaction(aParent, *this,
                                  aParent->GetBrowsingContextFieldEpoch());
   });
 
-  Apply(owner);
+  Apply(owner, /* aFromIPC */ true);
   return IPC_OK();
 }
 
@@ -104,7 +174,7 @@ mozilla::ipc::IPCResult Transaction<Context>::CommitFromIPC(
     ContentChild* aSource) {
   MOZ_DIAGNOSTIC_ASSERT(XRE_IsContentProcess());
   if (aOwner.IsNullOrDiscarded()) {
-    MOZ_LOG(Context::GetLog(), LogLevel::Debug,
+    MOZ_LOG(Context::GetSyncLog(), LogLevel::Debug,
             ("ChildIPC: Trying to send a message to dead or detached context"));
     return IPC_OK();
   }
@@ -113,16 +183,34 @@ mozilla::ipc::IPCResult Transaction<Context>::CommitFromIPC(
   // Clear any fields which have been obsoleted by the epoch.
   EachIndex([&](auto idx) {
     if (mModified.contains(idx) && FieldEpoch(idx, owner) > aEpoch) {
+      MOZ_LOG(
+          Context::GetSyncLog(), LogLevel::Debug,
+          ("Transaction::Obsoleted(#%" PRIx64 ", %" PRIu64 ">%" PRIu64 "): %s",
+           owner->Id(), FieldEpoch(idx, owner), aEpoch,
+           Context::FieldIndexToName(idx)));
       mModified -= idx;
     }
   });
 
-  Apply(owner);
+  if (mModified.isEmpty()) {
+    return IPC_OK();
+  }
+
+  Apply(owner, /* aFromIPC */ true);
   return IPC_OK();
 }
 
 template <typename Context>
-void Transaction<Context>::Apply(Context* aOwner) {
+void Transaction<Context>::Apply(Context* aOwner, bool aFromIPC) {
+  MOZ_ASSERT(!mModified.isEmpty());
+
+  MOZ_LOG(
+      Context::GetSyncLog(), LogLevel::Debug,
+      ("Transaction::Apply(#%" PRIx64 ", %s): %s", aOwner->Id(),
+       aFromIPC ? "ipc" : "local",
+       FormatTransaction<Context>(mModified, aOwner->mFields.mValues, mValues)
+           .get()));
+
   EachIndex([&](auto idx) {
     if (mModified.contains(idx)) {
       auto& txnField = mValues.Get(idx);
@@ -135,17 +223,54 @@ void Transaction<Context>::Apply(Context* aOwner) {
   mModified.clear();
 }
 
+inline CanSetResult AsCanSetResult(CanSetResult aValue) { return aValue; }
+inline CanSetResult AsCanSetResult(bool aValue) {
+  return aValue ? CanSetResult::Allow : CanSetResult::Deny;
+}
+
 template <typename Context>
 IndexSet Transaction<Context>::Validate(Context* aOwner,
                                         ContentParent* aSource) {
   IndexSet failedFields;
-  // Validate that the set from content is allowed before continuing.
+  Transaction<Context> revertTxn;
+
   EachIndex([&](auto idx) {
-    if (mModified.contains(idx) &&
-        NS_WARN_IF(!aOwner->CanSet(idx, mValues.Get(idx), aSource))) {
-      failedFields += idx;
+    if (!mModified.contains(idx)) {
+      return;
+    }
+
+    switch (AsCanSetResult(aOwner->CanSet(idx, mValues.Get(idx), aSource))) {
+      case CanSetResult::Allow:
+        break;
+      case CanSetResult::Deny:
+        failedFields += idx;
+        break;
+      case CanSetResult::Revert:
+        revertTxn.mValues.Get(idx) = aOwner->mFields.mValues.Get(idx);
+        revertTxn.mModified += idx;
+        break;
     }
   });
+
+  // If any changes need to be reverted, log them, remove them from this
+  // transaction, and optionally send a message to the source process.
+  if (!revertTxn.mModified.isEmpty()) {
+    // NOTE: Logging with modified IndexSet from revert transaction, and values
+    // from this transaction, so we log the failed values we're going to revert.
+    MOZ_LOG(Context::GetSyncLog(), LogLevel::Debug,
+            ("Transaction::PartialRevert(#%" PRIx64 ", pid %d): %s",
+             aOwner->Id(), aSource ? aSource->OtherPid() : -1,
+             FormatTransaction<Context>(revertTxn.mModified, mValues,
+                                        revertTxn.mValues)
+                 .get()));
+
+    mModified -= revertTxn.mModified;
+
+    if (aSource) {
+      aOwner->SendCommitTransaction(aSource, revertTxn,
+                                    aSource->GetBrowsingContextFieldEpoch());
+    }
+  }
   return failedFields;
 }
 

docshell/base/WindowContext.cpp

@@ -29,6 +29,7 @@ namespace dom {
 template class syncedcontext::Transaction<WindowContext>;
 
 static LazyLogModule gWindowContextLog("WindowContext");
+static LazyLogModule gWindowContextSyncLog("WindowContextSync");
 
 extern mozilla::LazyLogModule gUserInteractionPRLog;
 
@@ -41,6 +42,9 @@ static StaticAutoPtr<WindowContextByIdMap> gWindowContexts;
 /* static */
 LogModule* WindowContext::GetLog() { return gWindowContextLog; }
 
+/* static */
+LogModule* WindowContext::GetSyncLog() { return gWindowContextSyncLog; }
+
 /* static */
 already_AddRefed<WindowContext> WindowContext::GetById(
     uint64_t aInnerWindowId) {

docshell/base/WindowContext.h

@@ -96,6 +96,7 @@ class WindowContext : public nsISupports, public nsWrapperCache {
  public:
   static already_AddRefed<WindowContext> GetById(uint64_t aInnerWindowId);
   static LogModule* GetLog();
+  static LogModule* GetSyncLog();
 
   BrowsingContext* GetBrowsingContext() const { return mBrowsingContext; }
   BrowsingContextGroup* Group() const;