From 5d8224bf064292e1813819825cc545283887adbc Mon Sep 17 00:00:00 2001 From: Michael Layzell Date: Tue, 20 Jun 2017 13:28:47 -0400 Subject: [PATCH] Bug 1374665 - Stop parsing principals during GetPermissionsForKey, r=ehsan MozReview-Commit-ID: 28BCIqA2Kf2 --- extensions/cookie/nsPermissionManager.cpp | 105 ++++++++++++---------- extensions/cookie/nsPermissionManager.h | 16 ++++ 2 files changed, 75 insertions(+), 46 deletions(-) diff --git a/extensions/cookie/nsPermissionManager.cpp b/extensions/cookie/nsPermissionManager.cpp index dfa2c7f425cb..a3c8d611eba1 100644 --- a/extensions/cookie/nsPermissionManager.cpp +++ b/extensions/cookie/nsPermissionManager.cpp @@ -3125,14 +3125,13 @@ nsPermissionManager::GetPermissionsWithKey(const nsACString& aPermissionKey, 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. + nsAutoCString permissionKey; + GetKeyForOrigin(entry->GetKey()->mOrigin, permissionKey); - // Fetch the principal for the given origin. - nsCOMPtr principal; - nsresult rv = GetPrincipalFromOrigin(entry->GetKey()->mOrigin, - getter_AddRefs(principal)); - if (NS_WARN_IF(NS_FAILED(rv))) { + // 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; } @@ -3144,14 +3143,8 @@ nsPermissionManager::GetPermissionsWithKey(const nsACString& aPermissionKey, 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, @@ -3213,44 +3206,64 @@ nsPermissionManager::SetPermissionsWithKey(const nsACString& aPermissionKey, return NS_OK; } +/* static */ void +nsPermissionManager::GetKeyForOrigin(const nsACString& aOrigin, nsACString& aKey) +{ + aKey.Truncate(); + + // 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; + } + 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 dbgPrincipal; + MOZ_ALWAYS_SUCCEEDS(GetPrincipalFromOrigin(aOrigin, getter_AddRefs(dbgPrincipal))); + nsCOMPtr 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) { - MOZ_ASSERT(aPrincipal); - aKey.Truncate(); - - nsCOMPtr 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. - aKey.Truncate(); - return; - } - - nsAutoCString scheme; - rv = uri->GetScheme(scheme); + 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 diff --git a/extensions/cookie/nsPermissionManager.h b/extensions/cookie/nsPermissionManager.h index e1774a409d0b..b122801f90cc 100644 --- a/extensions/cookie/nsPermissionManager.h +++ b/extensions/cookie/nsPermissionManager.h @@ -229,6 +229,22 @@ public: */ 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.