From c771ae8334e2f691decbcc95526b64e521c11339 Mon Sep 17 00:00:00 2001 From: Ehsan Akhgari Date: Mon, 19 Aug 2019 14:38:47 +0000 Subject: [PATCH] Bug 1572240 - Part 6: Remove nsIPermissionManager.testPermissionOriginNoSuffix; r=baku This is now dead code which will be kept alive by the vtable, and introduces needless overhead inside the permission manager. Differential Revision: https://phabricator.services.mozilla.com/D42207 --HG-- extra : moz-landing-system : lando --- .../permissions/nsPermissionManager.cpp | 66 ++-------- extensions/permissions/nsPermissionManager.h | 25 ++-- .../test/unit/test_permmanager_subdomains.js | 119 ------------------ netwerk/base/nsIPermissionManager.idl | 15 --- 4 files changed, 23 insertions(+), 202 deletions(-) diff --git a/extensions/permissions/nsPermissionManager.cpp b/extensions/permissions/nsPermissionManager.cpp index 16863d03909e..f57d37698c38 100644 --- a/extensions/permissions/nsPermissionManager.cpp +++ b/extensions/permissions/nsPermissionManager.cpp @@ -770,12 +770,6 @@ nsPermissionManager::PermissionKey::CreateFromURI(nsIURI* aURI, return new PermissionKey(origin); } -nsPermissionManager::PermissionKey* -nsPermissionManager::PermissionKey::CreateFromOriginNoSuffix( - const nsACString& aOriginNoSuffix) { - return new PermissionKey(aOriginNoSuffix); -} - /** * Simple callback used by |AsyncClose| to trigger a treatment once * the database is closed. @@ -2255,24 +2249,6 @@ nsPermissionManager::TestPermission(nsIURI* aURI, const nsACString& aType, false, true); } -NS_IMETHODIMP -nsPermissionManager::TestPermissionOriginNoSuffix( - const nsACString& aOriginNoSuffix, const nsACString& aType, - uint32_t* aPermission) { - // Our caller isn't providing a type index hint, so we just pass -1 to force - // CommonPrepareToTestPermission to compute it for us based on aType. - auto preparationResult = CommonPrepareToTestPermission( - nullptr, -1, aType, aPermission, nsIPermissionManager::UNKNOWN_ACTION, - false, false, true); - if (preparationResult.is()) { - return preparationResult.as(); - } - - return CommonTestPermissionInternal(nullptr, nullptr, aOriginNoSuffix, - preparationResult.as(), aType, - aPermission, false, true); -} - NS_IMETHODIMP nsPermissionManager::TestPermissionFromWindow(mozIDOMWindow* aWindow, const nsACString& aType, @@ -2364,13 +2340,12 @@ nsPermissionManager::GetPermissionObject(nsIPrincipal* aPrincipal, } nsresult nsPermissionManager::CommonTestPermissionInternal( - nsIPrincipal* aPrincipal, nsIURI* aURI, const nsACString& aOriginNoSuffix, - int32_t aTypeIndex, const nsACString& aType, uint32_t* aPermission, - 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()); + nsIPrincipal* aPrincipal, nsIURI* aURI, int32_t aTypeIndex, + const nsACString& aType, uint32_t* aPermission, bool aExactHostMatch, + bool aIncludingSession) { + MOZ_ASSERT(aPrincipal || aURI); + MOZ_ASSERT_IF(aPrincipal, !aURI); + NS_ENSURE_ARG_POINTER(aPrincipal || aURI); #ifdef DEBUG { @@ -2379,8 +2354,6 @@ nsresult nsPermissionManager::CommonTestPermissionInternal( if (aURI) { prin = mozilla::BasePrincipal::CreateContentPrincipal( aURI, OriginAttributes()); - } else if (!aOriginNoSuffix.IsEmpty()) { - prin = mozilla::BasePrincipal::CreateContentPrincipal(aOriginNoSuffix); } } MOZ_ASSERT(prin); @@ -2390,8 +2363,7 @@ nsresult nsPermissionManager::CommonTestPermissionInternal( PermissionHashKey* entry = aPrincipal ? GetPermissionHashKey(aPrincipal, aTypeIndex, aExactHostMatch) - : GetPermissionHashKey(aURI, aOriginNoSuffix, aTypeIndex, - aExactHostMatch); + : GetPermissionHashKey(aURI, aTypeIndex, aExactHostMatch); if (!entry || (!aIncludingSession && entry->GetPermission(aTypeIndex).mNonSessionExpireType == nsIPermissionManager::EXPIRE_SESSION)) { @@ -2464,12 +2436,9 @@ nsPermissionManager::GetPermissionHashKey(nsIPrincipal* aPrincipal, // accepts host on the format "". This will perform an exact match lookup // as the string doesn't contain any dots. nsPermissionManager::PermissionHashKey* -nsPermissionManager::GetPermissionHashKey(nsIURI* aURI, - const nsACString& aOriginNoSuffix, - uint32_t aType, +nsPermissionManager::GetPermissionHashKey(nsIURI* aURI, uint32_t aType, bool aExactHostMatch) { - MOZ_ASSERT(aURI || !aOriginNoSuffix.IsEmpty()); - MOZ_ASSERT_IF(aURI, aOriginNoSuffix.IsEmpty()); + MOZ_ASSERT(aURI); #ifdef DEBUG { @@ -2477,9 +2446,6 @@ nsPermissionManager::GetPermissionHashKey(nsIURI* aURI, nsresult rv = NS_OK; if (aURI) { rv = GetPrincipal(aURI, getter_AddRefs(principal)); - } else { - principal = - mozilla::BasePrincipal::CreateContentPrincipal(aOriginNoSuffix); } MOZ_ASSERT_IF(NS_SUCCEEDED(rv), PermissionAvailable(principal, mTypeArray[aType])); @@ -2488,8 +2454,7 @@ nsPermissionManager::GetPermissionHashKey(nsIURI* aURI, nsresult rv; RefPtr key = - aURI ? PermissionKey::CreateFromURI(aURI, rv) - : PermissionKey::CreateFromOriginNoSuffix(aOriginNoSuffix); + aURI ? PermissionKey::CreateFromURI(aURI, rv) : nullptr; if (!key) { return nullptr; } @@ -2514,9 +2479,6 @@ nsPermissionManager::GetPermissionHashKey(nsIURI* aURI, if (NS_WARN_IF(NS_FAILED(rv))) { return nullptr; } - } else { - principal = - mozilla::BasePrincipal::CreateContentPrincipal(aOriginNoSuffix); } RemoveFromPrincipal(principal, mTypeArray[aType]); } else if (permEntry.mPermission == nsIPermissionManager::UNKNOWN_ACTION) { @@ -2534,15 +2496,9 @@ nsPermissionManager::GetPermissionHashKey(nsIURI* aURI, nsCOMPtr uri; if (aURI) { uri = GetNextSubDomainURI(aURI); - } else { - nsresult rv = NS_NewURI(getter_AddRefs(uri), aOriginNoSuffix); - if (NS_WARN_IF(NS_FAILED(rv))) { - return nullptr; - } - uri = GetNextSubDomainURI(uri); } if (uri) { - return GetPermissionHashKey(uri, EmptyCString(), aType, aExactHostMatch); + return GetPermissionHashKey(uri, aType, aExactHostMatch); } } diff --git a/extensions/permissions/nsPermissionManager.h b/extensions/permissions/nsPermissionManager.h index 8f766c164d06..4ec716c805de 100644 --- a/extensions/permissions/nsPermissionManager.h +++ b/extensions/permissions/nsPermissionManager.h @@ -77,8 +77,6 @@ class nsPermissionManager final : public nsIPermissionManager, static PermissionKey* CreateFromPrincipal(nsIPrincipal* aPrincipal, nsresult& aResult); static PermissionKey* CreateFromURI(nsIURI* aURI, nsresult& aResult); - static PermissionKey* CreateFromOriginNoSuffix( - const nsACString& aOriginNoSuffix); explicit PermissionKey(const nsACString& aOrigin) : mOrigin(aOrigin), mHashCode(mozilla::HashString(aOrigin)) {} @@ -327,9 +325,8 @@ class nsPermissionManager final : public nsIPermissionManager, PermissionHashKey* GetPermissionHashKey(nsIPrincipal* aPrincipal, uint32_t aType, bool aExactHostMatch); - PermissionHashKey* GetPermissionHashKey(nsIURI* aURI, - const nsACString& aOriginNoSuffix, - uint32_t aType, bool aExactHostMatch); + PermissionHashKey* GetPermissionHashKey(nsIURI* aURI, uint32_t aType, + bool aExactHostMatch); // The int32_t is the type index, the nsresult is an early bail-out return // code. @@ -436,8 +433,8 @@ class nsPermissionManager final : public nsIPermissionManager, } return CommonTestPermissionInternal( - aPrincipal, nullptr, EmptyCString(), preparationResult.as(), - aType, aPermission, aExactHostMatch, aIncludingSession); + aPrincipal, nullptr, preparationResult.as(), 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, @@ -453,14 +450,16 @@ class nsPermissionManager final : public nsIPermissionManager, } return CommonTestPermissionInternal( - nullptr, aURI, EmptyCString(), preparationResult.as(), aType, - aPermission, aExactHostMatch, aIncludingSession); + nullptr, aURI, preparationResult.as(), aType, aPermission, + aExactHostMatch, aIncludingSession); } // Only one of aPrincipal or aURI is allowed to be passed in. - nsresult CommonTestPermissionInternal( - nsIPrincipal* aPrincipal, nsIURI* aURI, const nsACString& aOriginNoSuffix, - int32_t aTypeIndex, const nsACString& aType, uint32_t* aPermission, - bool aExactHostMatch, bool aIncludingSession); + nsresult CommonTestPermissionInternal(nsIPrincipal* aPrincipal, nsIURI* aURI, + int32_t aTypeIndex, + const nsACString& aType, + uint32_t* aPermission, + bool aExactHostMatch, + bool aIncludingSession); nsresult OpenDatabase(nsIFile* permissionsFile); nsresult InitDB(bool aRemoveFile); diff --git a/extensions/permissions/test/unit/test_permmanager_subdomains.js b/extensions/permissions/test/unit/test_permmanager_subdomains.js index d533d73fda86..7caecb4fdd39 100644 --- a/extensions/permissions/test/unit/test_permmanager_subdomains.js +++ b/extensions/permissions/test/unit/test_permmanager_subdomains.js @@ -21,13 +21,6 @@ function run_test() { pm.testPermissionFromPrincipal(sub1Principal, "test/subdomains"), pm.ALLOW_ACTION ); - Assert.equal( - pm.testPermissionOriginNoSuffix( - sub1Principal.originNoSuffix, - "test/subdomains" - ), - pm.ALLOW_ACTION - ); // A sub-sub-domain should get the permission. let subsubPrincipal = getPrincipalFromURI("http://sub.sub1.example.com"); @@ -35,13 +28,6 @@ function run_test() { pm.testPermissionFromPrincipal(subsubPrincipal, "test/subdomains"), pm.ALLOW_ACTION ); - Assert.equal( - pm.testPermissionOriginNoSuffix( - subsubPrincipal.originNoSuffix, - "test/subdomains" - ), - pm.ALLOW_ACTION - ); // Another sub-domain shouldn't get the permission. let sub2Principal = getPrincipalFromURI("http://sub2.example.com"); @@ -49,13 +35,6 @@ function run_test() { pm.testPermissionFromPrincipal(sub2Principal, "test/subdomains"), pm.UNKNOWN_ACTION ); - Assert.equal( - pm.testPermissionOriginNoSuffix( - sub2Principal.originNoSuffix, - "test/subdomains" - ), - pm.UNKNOWN_ACTION - ); // Remove current permissions. pm.removeFromPrincipal(sub1Principal, "test/subdomains"); @@ -63,13 +42,6 @@ function run_test() { pm.testPermissionFromPrincipal(sub1Principal, "test/subdomains"), pm.UNKNOWN_ACTION ); - Assert.equal( - pm.testPermissionOriginNoSuffix( - sub1Principal.originNoSuffix, - "test/subdomains" - ), - pm.UNKNOWN_ACTION - ); // Adding the permission to the main domain. Checks if it is working. let mainPrincipal = getPrincipalFromURI("http://example.com"); @@ -78,48 +50,20 @@ function run_test() { pm.testPermissionFromPrincipal(mainPrincipal, "test/subdomains"), pm.ALLOW_ACTION ); - Assert.equal( - pm.testPermissionOriginNoSuffix( - mainPrincipal.originNoSuffix, - "test/subdomains" - ), - pm.ALLOW_ACTION - ); // All sub-domains should have the permission now. Assert.equal( pm.testPermissionFromPrincipal(sub1Principal, "test/subdomains"), pm.ALLOW_ACTION ); - Assert.equal( - pm.testPermissionOriginNoSuffix( - sub1Principal.originNoSuffix, - "test/subdomains" - ), - pm.ALLOW_ACTION - ); Assert.equal( pm.testPermissionFromPrincipal(sub2Principal, "test/subdomains"), pm.ALLOW_ACTION ); - Assert.equal( - pm.testPermissionOriginNoSuffix( - sub2Principal.originNoSuffix, - "test/subdomains" - ), - pm.ALLOW_ACTION - ); Assert.equal( pm.testPermissionFromPrincipal(subsubPrincipal, "test/subdomains"), pm.ALLOW_ACTION ); - Assert.equal( - pm.testPermissionOriginNoSuffix( - subsubPrincipal.originNoSuffix, - "test/subdomains" - ), - pm.ALLOW_ACTION - ); // Remove current permissions. pm.removeFromPrincipal(mainPrincipal, "test/subdomains"); @@ -127,46 +71,18 @@ function run_test() { pm.testPermissionFromPrincipal(mainPrincipal, "test/subdomains"), pm.UNKNOWN_ACTION ); - Assert.equal( - pm.testPermissionOriginNoSuffix( - mainPrincipal.originNoSuffix, - "test/subdomains" - ), - pm.UNKNOWN_ACTION - ); Assert.equal( pm.testPermissionFromPrincipal(sub1Principal, "test/subdomains"), pm.UNKNOWN_ACTION ); - Assert.equal( - pm.testPermissionOriginNoSuffix( - sub1Principal.originNoSuffix, - "test/subdomains" - ), - pm.UNKNOWN_ACTION - ); Assert.equal( pm.testPermissionFromPrincipal(sub2Principal, "test/subdomains"), pm.UNKNOWN_ACTION ); - Assert.equal( - pm.testPermissionOriginNoSuffix( - sub2Principal.originNoSuffix, - "test/subdomains" - ), - pm.UNKNOWN_ACTION - ); Assert.equal( pm.testPermissionFromPrincipal(subsubPrincipal, "test/subdomains"), pm.UNKNOWN_ACTION ); - Assert.equal( - pm.testPermissionOriginNoSuffix( - subsubPrincipal.originNoSuffix, - "test/subdomains" - ), - pm.UNKNOWN_ACTION - ); // A sanity check that the previous implementation wasn't passing... let crazyPrincipal = getPrincipalFromURI("http://com"); @@ -175,55 +91,20 @@ function run_test() { pm.testPermissionFromPrincipal(crazyPrincipal, "test/subdomains"), pm.ALLOW_ACTION ); - Assert.equal( - pm.testPermissionOriginNoSuffix( - crazyPrincipal.originNoSuffix, - "test/subdomains" - ), - pm.ALLOW_ACTION - ); Assert.equal( pm.testPermissionFromPrincipal(mainPrincipal, "test/subdomains"), pm.UNKNOWN_ACTION ); - Assert.equal( - pm.testPermissionOriginNoSuffix( - mainPrincipal.originNoSuffix, - "test/subdomains" - ), - pm.UNKNOWN_ACTION - ); Assert.equal( pm.testPermissionFromPrincipal(sub1Principal, "test/subdomains"), pm.UNKNOWN_ACTION ); - Assert.equal( - pm.testPermissionOriginNoSuffix( - sub1Principal.originNoSuffix, - "test/subdomains" - ), - pm.UNKNOWN_ACTION - ); Assert.equal( pm.testPermissionFromPrincipal(sub2Principal, "test/subdomains"), pm.UNKNOWN_ACTION ); - Assert.equal( - pm.testPermissionOriginNoSuffix( - sub2Principal.originNoSuffix, - "test/subdomains" - ), - pm.UNKNOWN_ACTION - ); Assert.equal( pm.testPermissionFromPrincipal(subsubPrincipal, "test/subdomains"), pm.UNKNOWN_ACTION ); - Assert.equal( - pm.testPermissionOriginNoSuffix( - subsubPrincipal.originNoSuffix, - "test/subdomains" - ), - pm.UNKNOWN_ACTION - ); } diff --git a/netwerk/base/nsIPermissionManager.idl b/netwerk/base/nsIPermissionManager.idl index de6ee59bbba5..bb5a6da5f2e9 100644 --- a/netwerk/base/nsIPermissionManager.idl +++ b/netwerk/base/nsIPermissionManager.idl @@ -219,21 +219,6 @@ interface nsIPermissionManager : nsISupports uint32_t testPermissionFromPrincipal(in nsIPrincipal principal, in ACString type); - /** - * Test whether a website specified by a given origin string has permission - * to perform the given action. This function is similar to testPermission() - * and is intended to be used where the cost of parsing a URI in the common - * case is to be avoided. - * @param originNoSuffix the origin string to be tested. - * @param type a case-sensitive ASCII string, identifying the - * permission. - * @param return see add(), param permission. returns UNKNOWN_ACTION - * when there is no stored permission for this uri and/ - * or type. - */ - uint32_t testPermissionOriginNoSuffix(in ACString originNoSuffix, - in ACString type); - /** * Test whether the principal associated with the window's document has the * permission to perform a given action. System principals will always