Bug 1527505 - Part 8: Lift the handling of expanded principals out of CommonTestPermissionInternal() into CommonPrepareToTestPermission(); r=nika

This patch in the series ensures that the order of operations in
recursive invocations of CommonTestPermission() remains consistent
compared to before the patch series, even though it is not strictly
needed for the performance improvements that the series focuses on.

The core idea behind CommonPrepareToTestPermission() now is to do the checks
that do not depend on the host name being tested, and for
CommonTestPermissionInternal() itself to focus on the rest of the checks,
that is looking up our hashtable entry based on the host name being tested,
and everything else that's needed from that point on.

Depends on D20235

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

--HG--
extra : moz-landing-system : lando

caps/moz.build

@@ -31,6 +31,7 @@ EXPORTS += [
 EXPORTS.mozilla = [
     'BasePrincipal.h',
     'ContentPrincipal.h',
+    'ExpandedPrincipal.h',
     'NullPrincipal.h',
     'NullPrincipalURI.h',
     'OriginAttributes.h',

extensions/cookie/nsPermissionManager.cpp

@@ -44,7 +44,6 @@
 #include "nsIObserverService.h"
 #include "nsPrintfCString.h"
 #include "mozilla/AbstractThread.h"
-#include "ExpandedPrincipal.h"
 #include "mozilla/StaticPtr.h"
 #include "mozilla/ClearOnShutdown.h"
 
@@ -2271,16 +2270,14 @@ nsPermissionManager::TestPermissionOriginNoSuffix(
   // CommonPrepareToTestPermission to compute it for us based on aType.
   auto preparationResult = CommonPrepareToTestPermission(
       nullptr, -1, aType, aPermission, nsIPermissionManager::UNKNOWN_ACTION,
-      false);
-  if (preparationResult.mShouldContinue == eDone) {
-    return NS_OK;
+      false, false, true);
+  if (preparationResult.is<nsresult>()) {
+    return preparationResult.as<nsresult>();
   }
 
-  MOZ_ASSERT(!preparationResult.mPrincipal);
-
-  return CommonTestPermissionInternal(
-      nullptr, nullptr, aOriginNoSuffix, preparationResult.mTypeIndex, aType,
-      aPermission, preparationResult.mDefaultPermission, false, true);
+  return CommonTestPermissionInternal(nullptr, nullptr, aOriginNoSuffix,
+                                      preparationResult.as<int32_t>(), aType,
+                                      aPermission, false, true);
 }
 
 NS_IMETHODIMP
@@ -2374,37 +2371,15 @@ nsPermissionManager::GetPermissionObject(nsIPrincipal* aPrincipal,
 }
 
 nsresult nsPermissionManager::CommonTestPermissionInternal(
-    BasePrincipal* aPrincipal, nsIURI* aURI, const nsACString& aOriginNoSuffix,
+    nsIPrincipal* aPrincipal, nsIURI* aURI, const nsACString& aOriginNoSuffix,
     int32_t aTypeIndex, const char* aType, uint32_t* aPermission,
-    uint32_t aDefaultPermission, bool aExactHostMatch, bool aIncludingSession) {
+    bool aExactHostMatch, bool aIncludingSession) {
   MOZ_ASSERT(aPrincipal || aURI || !aOriginNoSuffix.IsEmpty());
   MOZ_ASSERT_IF(aPrincipal, !aURI && aOriginNoSuffix.IsEmpty());
   MOZ_ASSERT_IF(aURI, !aPrincipal && aOriginNoSuffix.IsEmpty());
   NS_ENSURE_ARG_POINTER(aPrincipal || aURI || !aOriginNoSuffix.IsEmpty());
   NS_ENSURE_ARG_POINTER(aType);
 
-  // For expanded principals, we want to iterate over the allowlist and see
-  // if the permission is granted for any of them.
-  if (aPrincipal && aPrincipal->Is<ExpandedPrincipal>()) {
-    auto ep = aPrincipal->As<ExpandedPrincipal>();
-    for (auto& prin : ep->AllowList()) {
-      uint32_t perm;
-      nsresult rv = CommonTestPermission(prin, aTypeIndex, aType, &perm,
-                                         aDefaultPermission, true,
-                                         aExactHostMatch, aIncludingSession);
-      NS_ENSURE_SUCCESS(rv, rv);
-      if (perm == nsIPermissionManager::ALLOW_ACTION) {
-        *aPermission = perm;
-        return NS_OK;
-      } else if (perm == nsIPermissionManager::PROMPT_ACTION) {
-        // Store it, but keep going to see if we can do better.
-        *aPermission = perm;
-      }
-    }
-
-    return NS_OK;
-  }
-
 #ifdef DEBUG
   {
     nsCOMPtr<nsIPrincipal> prin = aPrincipal;

extensions/cookie/nsPermissionManager.h

@@ -23,8 +23,10 @@
 #include "nsIRunnable.h"
 #include "nsRefPtrHashtable.h"
 #include "mozilla/BasePrincipal.h"
+#include "mozilla/ExpandedPrincipal.h"
 #include "mozilla/MozPromise.h"
 #include "mozilla/Unused.h"
+#include "mozilla/Variant.h"
 #include "mozilla/Vector.h"
 
 namespace mozilla {
@@ -317,13 +319,9 @@ class nsPermissionManager final : public nsIPermissionManager,
                                           const nsACString& aOriginNoSuffix,
                                           uint32_t aType, bool aExactHostMatch);
 
-  enum TestPreparationEnum { eContinue, eDone };
-  struct TestPreparationResult {
-    mozilla::BasePrincipal* mPrincipal;
-    int32_t mTypeIndex;
-    uint32_t mDefaultPermission;
-    TestPreparationEnum mShouldContinue;
-  };
+  // The int32_t is the type index, the nsresult is an early bail-out return
+  // code.
+  typedef mozilla::Variant<int32_t, nsresult> TestPreparationResult;
   /**
    * Perform the early steps of a permission check and determine whether we need
    * to call CommonTestPermissionInternal() for the actual permission check.
@@ -344,15 +342,22 @@ class nsPermissionManager final : public nsIPermissionManager,
    *                           determine the result of the permission check.
    * @param aDefaultPermissionIsValid whether the previous argument contains a
    *                                  valid value.
+   * @param aExactHostMatch whether to look for the exact host name or also for
+   *                        subdomains that can have the same permission.
+   * @param aIncludingSession whether to include session permissions when
+   *                          testing for the permission.
    */
   TestPreparationResult CommonPrepareToTestPermission(
       nsIPrincipal* aPrincipal, int32_t aTypeIndex, const char* aType,
       uint32_t* aPermission, uint32_t aDefaultPermission,
-      bool aDefaultPermissionIsValid) {
+      bool aDefaultPermissionIsValid, bool aExactHostMatch,
+      bool aIncludingSession) {
+    using mozilla::AsVariant;
+
     auto* basePrin = mozilla::BasePrincipal::Cast(aPrincipal);
     if (basePrin && basePrin->IsSystemPrincipal()) {
       *aPermission = ALLOW_ACTION;
-      return {basePrin, -1, UNKNOWN_ACTION, eDone};
+      return AsVariant(NS_OK);
     }
 
     // For some permissions, query the default from a pref. We want to avoid
@@ -368,16 +373,41 @@ class nsPermissionManager final : public nsIPermissionManager,
     // Set the default.
     *aPermission = defaultPermission;
 
-    // For expanded principals, we want to iterate over the allowlist and see
-    // if the permission is granted for any of them.
     int32_t typeIndex =
         aTypeIndex == -1 ? GetTypeIndex(aType, false) : aTypeIndex;
-    // If type == -1, the type isn't known, just signal that we are done.
-    if (typeIndex == -1) {
-      return {basePrin, -1, UNKNOWN_ACTION, NS_OK, eDone};
+
+    // For expanded principals, we want to iterate over the allowlist and see
+    // if the permission is granted for any of them.
+    if (basePrin && basePrin->Is<ExpandedPrincipal>()) {
+      auto ep = basePrin->As<ExpandedPrincipal>();
+      for (auto& prin : ep->AllowList()) {
+        uint32_t perm;
+        nsresult rv = CommonTestPermission(prin, typeIndex, aType, &perm,
+                                           defaultPermission, true,
+                                           aExactHostMatch, aIncludingSession);
+        if (NS_WARN_IF(NS_FAILED(rv))) {
+          return AsVariant(rv);
         }
 
-    return {basePrin, typeIndex, uint32_t(defaultPermission), NS_OK, eContinue};
+        if (perm == nsIPermissionManager::ALLOW_ACTION) {
+          *aPermission = perm;
+          return AsVariant(NS_OK);
+        }
+        if (perm == nsIPermissionManager::PROMPT_ACTION) {
+          // Store it, but keep going to see if we can do better.
+          *aPermission = perm;
+        }
+      }
+
+      return AsVariant(NS_OK);
+    }
+
+    // If type == -1, the type isn't known, just signal that we are done.
+    if (typeIndex == -1) {
+      return AsVariant(NS_OK);
+    }
+
+    return AsVariant(typeIndex);
   }
 
   // If aTypeIndex is passed -1, we try to inder the type index from aType.
@@ -388,16 +418,14 @@ class nsPermissionManager final : public nsIPermissionManager,
                                 bool aExactHostMatch, bool aIncludingSession) {
     auto preparationResult = CommonPrepareToTestPermission(
         aPrincipal, aTypeIndex, aType, aPermission, aDefaultPermission,
-        aDefaultPermissionIsValid);
-    if (preparationResult.mShouldContinue == eDone) {
-      return NS_OK;
+        aDefaultPermissionIsValid, aExactHostMatch, aIncludingSession);
+    if (preparationResult.is<nsresult>()) {
+      return preparationResult.as<nsresult>();
     }
 
     return CommonTestPermissionInternal(
-        preparationResult.mPrincipal, nullptr, EmptyCString(),
-        preparationResult.mTypeIndex, aType, aPermission,
-        preparationResult.mDefaultPermission, aExactHostMatch,
-        aIncludingSession);
+        aPrincipal, nullptr, EmptyCString(), preparationResult.as<int32_t>(),
+        aType, aPermission, aExactHostMatch, aIncludingSession);
   }
   // If aTypeIndex is passed -1, we try to inder the type index from aType.
   nsresult CommonTestPermission(nsIURI* aURI, int32_t aTypeIndex,
@@ -407,23 +435,21 @@ class nsPermissionManager final : public nsIPermissionManager,
                                 bool aExactHostMatch, bool aIncludingSession) {
     auto preparationResult = CommonPrepareToTestPermission(
         nullptr, aTypeIndex, aType, aPermission, aDefaultPermission,
-        aDefaultPermissionIsValid);
-    if (preparationResult.mShouldContinue == eDone) {
-      return NS_OK;
+        aDefaultPermissionIsValid, aExactHostMatch, aIncludingSession);
+    if (preparationResult.is<nsresult>()) {
+      return preparationResult.as<nsresult>();
     }
 
-    MOZ_ASSERT(!preparationResult.mPrincipal);
-
     return CommonTestPermissionInternal(
-        nullptr, aURI, EmptyCString(), preparationResult.mTypeIndex, aType,
-        aPermission, preparationResult.mDefaultPermission, aExactHostMatch,
-        aIncludingSession);
+        nullptr, aURI, EmptyCString(), preparationResult.as<int32_t>(), aType,
+        aPermission, aExactHostMatch, aIncludingSession);
   }
   // Only one of aPrincipal or aURI is allowed to be passed in.
-  nsresult CommonTestPermissionInternal(
-      mozilla::BasePrincipal* aPrincipal, nsIURI* aURI,
-      const nsACString& aOriginNoSuffix, int32_t aTypeIndex, const char* aType,
-      uint32_t* aPermission, uint32_t aDefaultPermission, bool aExactHostMatch,
+  nsresult CommonTestPermissionInternal(nsIPrincipal* aPrincipal, nsIURI* aURI,
+                                        const nsACString& aOriginNoSuffix,
+                                        int32_t aTypeIndex, const char* aType,
+                                        uint32_t* aPermission,
+                                        bool aExactHostMatch,
                                         bool aIncludingSession);
 
   nsresult OpenDatabase(nsIFile* permissionsFile);