Bug 1374665 - Stop parsing principals during GetPermissionsForKey, r=ehsan
☠☠ backed out by 62899ab59159 ☠ ☠
authorMichael Layzell <michael@thelayzells.com>
Tue, 20 Jun 2017 13:28:47 -0400
changeset 414077 44532a19e524df52ccf35fd0e342996027d86342
parent 414076 36d662fbab77e831c6c7bf0c3bd734020bb76a87
child 414078 4bc36447fcf42277b0e13b42f9ec710536c3f051
push id7566
push usermtabara@mozilla.com
push dateWed, 02 Aug 2017 08:25:16 +0000
treeherdermozilla-beta@86913f512c3c [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersehsan
bugs1374665
milestone56.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1374665 - Stop parsing principals during GetPermissionsForKey, r=ehsan MozReview-Commit-ID: 28BCIqA2Kf2
extensions/cookie/nsPermissionManager.cpp
extensions/cookie/nsPermissionManager.h
--- a/extensions/cookie/nsPermissionManager.cpp
+++ b/extensions/cookie/nsPermissionManager.cpp
@@ -3120,43 +3120,36 @@ nsPermissionManager::GetPermissionsWithK
   aPerms.Clear();
   if (NS_WARN_IF(XRE_IsContentProcess())) {
     return NS_ERROR_NOT_AVAILABLE;
   }
 
   for (auto iter = mPermissionTable.Iter(); !iter.Done(); iter.Next()) {
     PermissionHashKey* entry = iter.Get();
 
-    // XXX: Is it worthwhile to have a shortcut Origin->Key implementation? as
-    // we could implement this without creating a codebase principal.
-
-    // Fetch the principal for the given origin.
-    nsCOMPtr<nsIPrincipal> principal;
-    nsresult rv = GetPrincipalFromOrigin(entry->GetKey()->mOrigin,
-                                         getter_AddRefs(principal));
-    if (NS_WARN_IF(NS_FAILED(rv))) {
+    nsAutoCString permissionKey;
+    GetKeyForOrigin(entry->GetKey()->mOrigin, permissionKey);
+
+    // If the keys don't match, and we aren't getting the default "" key, then
+    // we can exit early. We have to keep looking if we're getting the default
+    // key, as we may see a preload permission which should be transmitted.
+    if (aPermissionKey != permissionKey && !aPermissionKey.IsEmpty()) {
       continue;
     }
 
     for (const auto& permEntry : entry->GetPermissions()) {
       // Given how "default" permissions work and the possibility of them being
       // overridden with UNKNOWN_ACTION, we might see this value here - but we
       // do not want to send it to the content process.
       if (permEntry.mPermission == nsIPermissionManager::UNKNOWN_ACTION) {
         continue;
       }
 
-      // XXX: This performs extra work, such as in many cases re-computing the
-      // Origin (which we just computed the nsIPrincipal from). We may want to
-      // implement a custom version of this logic which avoids that extra work.
-      // See bug 1354700.
-      nsAutoCString permissionKey;
-      GetKeyForPermission(principal, mTypeArray[permEntry.mType].get(), permissionKey);
-
-      if (permissionKey == aPermissionKey) {
+      bool isPreload = IsPreloadPermission(mTypeArray[permEntry.mType].get());
+      if ((isPreload && aPermissionKey.IsEmpty()) || (!isPreload && aPermissionKey == permissionKey)) {
         aPerms.AppendElement(IPC::Permission(entry->GetKey()->mOrigin,
                                              mTypeArray.ElementAt(permEntry.mType),
                                              permEntry.mPermission,
                                              permEntry.mExpireType,
                                              permEntry.mExpireTime));
       }
     }
   }
@@ -3209,53 +3202,73 @@ nsPermissionManager::SetPermissionsWithK
     AddInternal(principal, perm.type, perm.capability, 0, perm.expireType,
                 perm.expireTime, modificationTime, eNotify, eNoDBOperation,
                 true /* ignoreSessionPermissions */);
   }
   return NS_OK;
 }
 
 /* static */ void
-nsPermissionManager::GetKeyForPrincipal(nsIPrincipal* aPrincipal, nsACString& aKey)
+nsPermissionManager::GetKeyForOrigin(const nsACString& aOrigin, nsACString& aKey)
 {
-  MOZ_ASSERT(aPrincipal);
   aKey.Truncate();
 
-  nsCOMPtr<nsIURI> uri;
-  nsresult rv = aPrincipal->GetURI(getter_AddRefs(uri));
-  if (NS_WARN_IF(NS_FAILED(rv) || !uri)) {
-    // NOTE: We don't propagate the error here, instead we produce the default
-    // "" permission key. This means that we can assign every principal a key,
-    // even if the GetURI operation on that principal is not meaningful.
+  // We only key origins for http, https, and ftp URIs. All origins begin with
+  // the URL which they apply to, which means that they should begin with their
+  // scheme in the case where they are one of these interesting URIs. We don't
+  // want to actually parse the URL here however, because this can be called on
+  // hot paths.
+  if (!StringBeginsWith(aOrigin, NS_LITERAL_CSTRING("http:")) &&
+      !StringBeginsWith(aOrigin, NS_LITERAL_CSTRING("https:")) &&
+      !StringBeginsWith(aOrigin, NS_LITERAL_CSTRING("ftp:"))) {
+    return;
+  }
+
+  // We need to look at the originAttributes if they are present, to make sure
+  // to remove any which we don't want. We put the rest of the origin, not
+  // including the attributes, into the key.
+  OriginAttributes attrs;
+  if (!attrs.PopulateFromOrigin(aOrigin, aKey)) {
     aKey.Truncate();
     return;
   }
-
-  nsAutoCString scheme;
-  rv = uri->GetScheme(scheme);
+  attrs.StripAttributes(OriginAttributes::STRIP_USER_CONTEXT_ID |
+                        OriginAttributes::STRIP_FIRST_PARTY_DOMAIN);
+
+#ifdef DEBUG
+  // Parse the origin string into a principal, and extract some useful
+  // information from it for assertions.
+  nsCOMPtr<nsIPrincipal> dbgPrincipal;
+  MOZ_ALWAYS_SUCCEEDS(GetPrincipalFromOrigin(aOrigin, getter_AddRefs(dbgPrincipal)));
+  nsCOMPtr<nsIURI> dbgUri;
+  MOZ_ALWAYS_SUCCEEDS(dbgPrincipal->GetURI(getter_AddRefs(dbgUri)));
+  nsAutoCString dbgScheme;
+  MOZ_ALWAYS_SUCCEEDS(dbgUri->GetScheme(dbgScheme));
+  MOZ_ASSERT(dbgScheme.EqualsLiteral("http") ||
+             dbgScheme.EqualsLiteral("https") ||
+             dbgScheme.EqualsLiteral("ftp"));
+  MOZ_ASSERT(dbgPrincipal->OriginAttributesRef() == attrs);
+#endif
+
+  // Append the stripped suffix to the output origin key.
+  nsAutoCString suffix;
+  attrs.CreateSuffix(suffix);
+  aKey.Append(suffix);
+}
+
+/* static */ void
+nsPermissionManager::GetKeyForPrincipal(nsIPrincipal* aPrincipal, nsACString& aKey)
+{
+  nsAutoCString origin;
+  nsresult rv = aPrincipal->GetOrigin(origin);
   if (NS_WARN_IF(NS_FAILED(rv))) {
-    // NOTE: Produce the default "" key as a fallback.
     aKey.Truncate();
     return;
   }
-
-  // URIs which have schemes other than http, https and ftp share the ""
-  // permission key.
-  if (scheme.EqualsLiteral("http") ||
-      scheme.EqualsLiteral("https") ||
-      scheme.EqualsLiteral("ftp")) {
-    rv = GetOriginFromPrincipal(aPrincipal, aKey);
-    if (NS_SUCCEEDED(rv)) {
-      return;
-    }
-  }
-
-  // NOTE: Produce the default "" key as a fallback.
-  aKey.Truncate();
-  return;
+  GetKeyForOrigin(origin, aKey);
 }
 
 /* static */ void
 nsPermissionManager::GetKeyForPermission(nsIPrincipal* aPrincipal, const char* aType, nsACString& aKey)
 {
   // Preload permissions have the "" key.
   if (IsPreloadPermission(aType)) {
     aKey.Truncate();
--- a/extensions/cookie/nsPermissionManager.h
+++ b/extensions/cookie/nsPermissionManager.h
@@ -228,16 +228,32 @@ public:
    * @param aPermissionKey  A string which will be filled with the permission key.
    */
   static void GetKeyForPrincipal(nsIPrincipal* aPrincipal, nsACString& aPermissionKey);
 
   /**
    * See `nsIPermissionManager::GetPermissionsWithKey` for more info on
    * permission keys.
    *
+   * Get the permission key corresponding to the given Origin. This method is
+   * like GetKeyForPrincipal, except that it avoids creating a nsIPrincipal
+   * object when you already have access to an origin string.
+   *
+   * If this method is passed a nonsensical origin string it may produce a
+   * nonsensical permission key result.
+   *
+   * @param aOrigin  The origin which the key is to be extracted from.
+   * @param aPermissionKey  A string which will be filled with the permission key.
+   */
+  static void GetKeyForOrigin(const nsACString& aOrigin, nsACString& aPermissionKey);
+
+  /**
+   * See `nsIPermissionManager::GetPermissionsWithKey` for more info on
+   * permission keys.
+   *
    * Get the permission key corresponding to the given Principal and type. This
    * method is intentionally infallible, as we want to provide an permission key
    * to every principal. Principals which don't have meaningful URIs with
    * http://, https://, or ftp:// schemes are given the default "" Permission
    * Key.
    *
    * This method is different from GetKeyForPrincipal in that it also takes
    * permissions which must be sent down before loading a document into account.