Bug 1337056 - Part 11: Change the permission key assertion to a fatal assert on debug builds, r=ehsan
authorMichael Layzell <michael@thelayzells.com>
Wed, 15 Mar 2017 16:15:47 -0400
changeset 348705 2686c6ee997dce91e573d7b0e1555859e33810e6
parent 348704 63e7bf43da6505b9789b714495bebca9fc1c05da
child 348706 f86ef08fdae0a012c28e1264e83238cfb31ccc2e
push id31533
push userkwierso@gmail.com
push dateTue, 21 Mar 2017 23:08:53 +0000
treeherdermozilla-central@8744e9f8eb99 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersehsan
bugs1337056
milestone55.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 1337056 - Part 11: Change the permission key assertion to a fatal assert on debug builds, r=ehsan MozReview-Commit-ID: HTxvlomRKWy
dom/ipc/ContentParent.cpp
extensions/cookie/nsPermissionManager.cpp
extensions/cookie/test/browser.ini
--- a/dom/ipc/ContentParent.cpp
+++ b/dom/ipc/ContentParent.cpp
@@ -5106,24 +5106,29 @@ ContentParent::TransmitPermissionsFor(ns
 
   return NS_OK;
 }
 
 void
 ContentParent::EnsurePermissionsByKey(const nsCString& aKey)
 {
 #ifdef MOZ_PERMISSIONS
+  // NOTE: Make sure to initialize the permission manager before updating the
+  // mActivePermissionKeys list. If the permission manager is being initialized
+  // by this call to GetPermissionManager, and we've added the key to
+  // mActivePermissionKeys, then the permission manager will send down a
+  // SendAddPermission before receiving the SendSetPermissionsWithKey message.
+  nsCOMPtr<nsIPermissionManager> permManager =
+    services::GetPermissionManager();
+
   if (mActivePermissionKeys.Contains(aKey)) {
     return;
   }
   mActivePermissionKeys.PutEntry(aKey);
 
-  nsCOMPtr<nsIPermissionManager> permManager =
-    services::GetPermissionManager();
-
   nsTArray<IPC::Permission> perms;
   nsresult rv = permManager->GetPermissionsWithKey(aKey, perms);
   if (NS_WARN_IF(NS_FAILED(rv))) {
     return;
   }
 
   Unused << SendSetPermissionsWithKey(aKey, perms);
 #endif
--- a/extensions/cookie/nsPermissionManager.cpp
+++ b/extensions/cookie/nsPermissionManager.cpp
@@ -642,30 +642,28 @@ nsPermissionManager::PermissionKey::Crea
 {
   nsAutoCString origin;
   aResult = GetOriginFromPrincipal(aPrincipal, origin);
   if (NS_WARN_IF(NS_FAILED(aResult))) {
     return nullptr;
   }
 
 #ifdef DEBUG
-  // Creating a PermissionsKey to look up a permission if we haven't had those keys
-  // synced down yet is problematic, so we do a check here and emit an assertion if
-  // we see it happening.
+  // Creating a PermissionsKey to look up a permission if we haven't had those
+  // keys synced down yet is problematic, so we do a check here and crash on
+  // debug builds if we see it happening.
   if (XRE_IsContentProcess()) {
     nsAutoCString permissionKey;
     GetKeyForPrincipal(aPrincipal, permissionKey);
 
-    // NOTE: Theoretically an addon could ask for permissions which the process
-    // wouldn't have access to, and we wouldn't want to crash the process in
-    // this case, but our chrome code should never do this. Using NS_ASSERTION
-    // here so that we can test fetching unavaliable permissions in tests.
-    NS_ASSERTION(gPermissionManager->mAvailablePermissionKeys.Contains(permissionKey),
-                 nsPrintfCString("This content process hasn't received the "
+    if (!gPermissionManager->mAvailablePermissionKeys.Contains(permissionKey)) {
+      NS_WARNING(nsPrintfCString("This content process hasn't received the "
                                  "permissions for %s yet", permissionKey.get()).get());
+      MOZ_CRASH("The content process hasn't recieved permissions for an origin yet.");
+    }
   }
 #endif
 
   return new PermissionKey(origin);
 }
 
 /**
  * Simple callback used by |AsyncClose| to trigger a treatment once
--- a/extensions/cookie/test/browser.ini
+++ b/extensions/cookie/test/browser.ini
@@ -1,5 +1,9 @@
 [DEFAULT]
 
 [browser_test_favicon.js]
 [browser_permmgr_sync.js]
-skip-if = !e10s # This tests e10s specific behavior
+# The browser_permmgr_sync test tests e10s specific behavior, and runs code
+# paths which would hit the debug only assertion in
+# nsPermissionManager::PermissionKey::CreateFromPrincipal. Because of this, it
+# is only run in e10s opt builds.
+skip-if = debug || !e10s