Bug 1182408 - Part 2: Use nsTHashTable::Iterator in nsPermissionManager. r=ehsan
authorBirunthan Mohanathas <birunthan@mohanathas.com>
Wed, 15 Jul 2015 12:58:08 -0700
changeset 253096 2c709a5c7aff60fdfd39dede4bbcbf14df9d2495
parent 253095 f7eb7bc6a99c8a2fea9f455a8221f9bf80d673b5
child 253097 5ebdd39f878955a2a8de6b9c4acf2bebeb233663
push id29061
push userryanvm@gmail.com
push dateThu, 16 Jul 2015 18:53:45 +0000
treeherdermozilla-central@a0f4a688433d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersehsan
bugs1182408
milestone42.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 1182408 - Part 2: Use nsTHashTable::Iterator in nsPermissionManager. r=ehsan
extensions/cookie/nsPermissionManager.cpp
extensions/cookie/nsPermissionManager.h
--- a/extensions/cookie/nsPermissionManager.cpp
+++ b/extensions/cookie/nsPermissionManager.cpp
@@ -1450,67 +1450,41 @@ nsPermissionManager::GetPermissionHashKe
       return GetPermissionHashKey(domain, aAppId, aIsInBrowserElement, aType, aExactHostMatch);
     }
   }
 
   // No entry, really...
   return nullptr;
 }
 
-// helper struct for passing arguments into hash enumeration callback.
-struct nsGetEnumeratorData
-{
-  nsGetEnumeratorData(nsCOMArray<nsIPermission> *aArray,
-                      const nsTArray<nsCString> *aTypes,
-                      int64_t aSince = 0)
-   : array(aArray)
-   , types(aTypes)
-   , since(aSince) {}
-
-  nsCOMArray<nsIPermission> *array;
-  const nsTArray<nsCString> *types;
-  int64_t since;
-};
-
-static PLDHashOperator
-AddPermissionsToList(nsPermissionManager::PermissionHashKey* entry, void *arg)
-{
-  nsGetEnumeratorData *data = static_cast<nsGetEnumeratorData *>(arg);
-
-  for (uint32_t i = 0; i < entry->GetPermissions().Length(); ++i) {
-    nsPermissionManager::PermissionEntry& permEntry = entry->GetPermissions()[i];
-
-    // 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 return them via the enumerator.
-    if (permEntry.mPermission == nsIPermissionManager::UNKNOWN_ACTION) {
-      continue;
-    }
-
-    nsPermission *perm = new nsPermission(entry->GetKey()->mHost,
-                                          entry->GetKey()->mAppId,
-                                          entry->GetKey()->mIsInBrowserElement,
-                                          data->types->ElementAt(permEntry.mType),
-                                          permEntry.mPermission,
-                                          permEntry.mExpireType,
-                                          permEntry.mExpireTime);
-
-    data->array->AppendObject(perm);
-  }
-
-  return PL_DHASH_NEXT;
-}
-
 NS_IMETHODIMP nsPermissionManager::GetEnumerator(nsISimpleEnumerator **aEnum)
 {
   // roll an nsCOMArray of all our permissions, then hand out an enumerator
   nsCOMArray<nsIPermission> array;
-  nsGetEnumeratorData data(&array, &mTypeArray);
+
+  for (auto iter = mPermissionTable.Iter(); !iter.Done(); iter.Next()) {
+    PermissionHashKey* entry = iter.Get();
+    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 return them via the enumerator.
+      if (permEntry.mPermission == nsIPermissionManager::UNKNOWN_ACTION) {
+        continue;
+      }
 
-  mPermissionTable.EnumerateEntries(AddPermissionsToList, &data);
+      array.AppendObject(
+        new nsPermission(entry->GetKey()->mHost,
+                         entry->GetKey()->mAppId,
+                         entry->GetKey()->mIsInBrowserElement,
+                         mTypeArray.ElementAt(permEntry.mType),
+                         permEntry.mPermission,
+                         permEntry.mExpireType,
+                         permEntry.mExpireTime));
+    }
+  }
 
   return NS_NewArrayEnumerator(aEnum, array);
 }
 
 NS_IMETHODIMP nsPermissionManager::Observe(nsISupports *aSubject, const char *aTopic, const char16_t *someData)
 {
   ENSURE_NOT_CHILD_PROCESS;
 
@@ -1524,52 +1498,39 @@ NS_IMETHODIMP nsPermissionManager::Obser
   else if (!nsCRT::strcmp(aTopic, "profile-do-change")) {
     // the profile has already changed; init the db from the new location
     InitDB(false);
   }
 
   return NS_OK;
 }
 
-static PLDHashOperator
-AddPermissionsModifiedSinceToList(
-  nsPermissionManager::PermissionHashKey* entry, void* arg)
-{
-  nsGetEnumeratorData* data = static_cast<nsGetEnumeratorData *>(arg);
-
-  for (size_t i = 0; i < entry->GetPermissions().Length(); ++i) {
-    const nsPermissionManager::PermissionEntry& permEntry = entry->GetPermissions()[i];
-
-    if (data->since > permEntry.mModificationTime) {
-      continue;
-    }
-
-    nsPermission* perm = new nsPermission(entry->GetKey()->mHost,
-                                          entry->GetKey()->mAppId,
-                                          entry->GetKey()->mIsInBrowserElement,
-                                          data->types->ElementAt(permEntry.mType),
-                                          permEntry.mPermission,
-                                          permEntry.mExpireType,
-                                          permEntry.mExpireTime);
-
-    data->array->AppendObject(perm);
-  }
-  return PL_DHASH_NEXT;
-}
-
 nsresult
 nsPermissionManager::RemoveAllModifiedSince(int64_t aModificationTime)
 {
   ENSURE_NOT_CHILD_PROCESS;
 
-  // roll an nsCOMArray of all our permissions, then hand out an enumerator
   nsCOMArray<nsIPermission> array;
-  nsGetEnumeratorData data(&array, &mTypeArray, aModificationTime);
+  for (auto iter = mPermissionTable.Iter(); !iter.Done(); iter.Next()) {
+    PermissionHashKey* entry = iter.Get();
+    for (const auto& permEntry : entry->GetPermissions()) {
+      if (aModificationTime > permEntry.mModificationTime) {
+        continue;
+      }
 
-  mPermissionTable.EnumerateEntries(AddPermissionsModifiedSinceToList, &data);
+      array.AppendObject(
+        new nsPermission(entry->GetKey()->mHost,
+                         entry->GetKey()->mAppId,
+                         entry->GetKey()->mIsInBrowserElement,
+                         mTypeArray.ElementAt(permEntry.mType),
+                         permEntry.mPermission,
+                         permEntry.mExpireType,
+                         permEntry.mExpireTime));
+    }
+  }
 
   for (int32_t i = 0; i<array.Count(); ++i) {
     nsAutoCString host;
     bool isInBrowserElement = false;
     nsAutoCString type;
     uint32_t appId = 0;
 
     array[i]->GetHost(host);
@@ -1594,52 +1555,29 @@ nsPermissionManager::RemoveAllModifiedSi
       nsPermissionManager::eWriteToDB);
   }
   // now re-import any defaults as they may now be required if we just deleted
   // an override.
   ImportDefaults();
   return NS_OK;
 }
 
-PLDHashOperator
-nsPermissionManager::GetPermissionsForApp(nsPermissionManager::PermissionHashKey* entry, void* arg)
-{
-  GetPermissionsForAppStruct* data = static_cast<GetPermissionsForAppStruct*>(arg);
-  if (entry->GetKey()->mAppId != data->appId ||
-      (data->browserOnly && !entry->GetKey()->mIsInBrowserElement)) {
-    return PL_DHASH_NEXT;
-  }
-
-  for (uint32_t i = 0; i < entry->GetPermissions().Length(); ++i) {
-    nsPermissionManager::PermissionEntry& permEntry = entry->GetPermissions()[i];
-    data->permissions.AppendObject(new nsPermission(entry->GetKey()->mHost,
-                                                    entry->GetKey()->mAppId,
-                                                    entry->GetKey()->mIsInBrowserElement,
-                                                    gPermissionManager->mTypeArray.ElementAt(permEntry.mType),
-                                                    permEntry.mPermission,
-                                                    permEntry.mExpireType,
-                                                    permEntry.mExpireTime));
-  }
-
-  return PL_DHASH_NEXT;
-}
-
 NS_IMETHODIMP
 nsPermissionManager::RemovePermissionsForApp(uint32_t aAppId, bool aBrowserOnly)
 {
   ENSURE_NOT_CHILD_PROCESS;
   NS_ENSURE_ARG(aAppId != nsIScriptSecurityManager::NO_APP_ID);
 
   // We begin by removing all the permissions from the DB.
   // After clearing the DB, we call AddInternal() to make sure that all
   // processes are aware of this change and the representation of the DB in
   // memory is updated.
-  // We have to get all permissions associated with an application and then
-  // remove those because doing so in EnumerateEntries() would fail because
-  // we might happen to actually delete entries from the list.
+  // We have to get all permissions associated with an application first
+  // because removing entries from the permissions table while iterating over
+  // it is dangerous.
 
   nsAutoCString sql;
   sql.AppendLiteral("DELETE FROM moz_hosts WHERE appId=");
   sql.AppendInt(aAppId);
 
   if (aBrowserOnly) {
     sql.AppendLiteral(" AND isInBrowserElement=1");
   }
@@ -1647,27 +1585,44 @@ nsPermissionManager::RemovePermissionsFo
   nsCOMPtr<mozIStorageAsyncStatement> removeStmt;
   nsresult rv = mDBConn->CreateAsyncStatement(sql, getter_AddRefs(removeStmt));
   NS_ENSURE_SUCCESS(rv, rv);
 
   nsCOMPtr<mozIStoragePendingStatement> pending;
   rv = removeStmt->ExecuteAsync(nullptr, getter_AddRefs(pending));
   NS_ENSURE_SUCCESS(rv, rv);
 
-  GetPermissionsForAppStruct data(aAppId, aBrowserOnly);
-  mPermissionTable.EnumerateEntries(GetPermissionsForApp, &data);
+  nsCOMArray<nsIPermission> permissions;
+  for (auto iter = mPermissionTable.Iter(); !iter.Done(); iter.Next()) {
+    PermissionHashKey* entry = iter.Get();
+    if (entry->GetKey()->mAppId != aAppId ||
+        (aBrowserOnly && !entry->GetKey()->mIsInBrowserElement)) {
+      continue;
+    }
 
-  for (int32_t i=0; i<data.permissions.Count(); ++i) {
+    for (const auto& permEntry : entry->GetPermissions()) {
+      permissions.AppendObject(
+        new nsPermission(entry->GetKey()->mHost,
+                         entry->GetKey()->mAppId,
+                         entry->GetKey()->mIsInBrowserElement,
+                         mTypeArray.ElementAt(permEntry.mType),
+                         permEntry.mPermission,
+                         permEntry.mExpireType,
+                         permEntry.mExpireTime));
+    }
+  }
+
+  for (int32_t i = 0; i < permissions.Count(); ++i) {
     nsAutoCString host;
     bool isInBrowserElement;
     nsAutoCString type;
 
-    data.permissions[i]->GetHost(host);
-    data.permissions[i]->GetIsInBrowserElement(&isInBrowserElement);
-    data.permissions[i]->GetType(type);
+    permissions[i]->GetHost(host);
+    permissions[i]->GetIsInBrowserElement(&isInBrowserElement);
+    permissions[i]->GetType(type);
 
     nsCOMPtr<nsIPrincipal> principal;
     if (NS_FAILED(GetPrincipal(host, aAppId, isInBrowserElement,
                                getter_AddRefs(principal)))) {
       NS_ERROR("GetPrincipal() failed!");
       continue;
     }
 
@@ -1680,72 +1635,69 @@ nsPermissionManager::RemovePermissionsFo
                 0,
                 nsPermissionManager::eNotify,
                 nsPermissionManager::eNoDBOperation);
   }
 
   return NS_OK;
 }
 
-PLDHashOperator
-nsPermissionManager::RemoveExpiredPermissionsForAppEnumerator(
-  nsPermissionManager::PermissionHashKey* entry, void* arg)
-{
-  uint32_t* appId = static_cast<uint32_t*>(arg);
-  if (entry->GetKey()->mAppId != *appId) {
-    return PL_DHASH_NEXT;
-  }
-
-  for (uint32_t i = 0; i < entry->GetPermissions().Length(); ++i) {
-    nsPermissionManager::PermissionEntry& permEntry = entry->GetPermissions()[i];
-    if (permEntry.mExpireType != nsIPermissionManager::EXPIRE_SESSION) {
-      continue;
-    }
-
-    if (permEntry.mNonSessionExpireType == nsIPermissionManager::EXPIRE_SESSION) {
-      PermissionEntry oldPermissionEntry = entry->GetPermissions()[i];
-
-      entry->GetPermissions().RemoveElementAt(i);
-
-      gPermissionManager->NotifyObserversWithPermission(entry->GetKey()->mHost,
-                                                        entry->GetKey()->mAppId,
-                                                        entry->GetKey()->mIsInBrowserElement,
-                                                        gPermissionManager->mTypeArray.ElementAt(oldPermissionEntry.mType),
-                                                        oldPermissionEntry.mPermission,
-                                                        oldPermissionEntry.mExpireType,
-                                                        oldPermissionEntry.mExpireTime,
-                                                        MOZ_UTF16("deleted"));
-      --i;
-      continue;
-    }
-
-    permEntry.mPermission = permEntry.mNonSessionPermission;
-    permEntry.mExpireType = permEntry.mNonSessionExpireType;
-    permEntry.mExpireTime = permEntry.mNonSessionExpireTime;
-
-    gPermissionManager->NotifyObserversWithPermission(entry->GetKey()->mHost,
-                                                      entry->GetKey()->mAppId,
-                                                      entry->GetKey()->mIsInBrowserElement,
-                                                      gPermissionManager->mTypeArray.ElementAt(permEntry.mType),
-                                                      permEntry.mPermission,
-                                                      permEntry.mExpireType,
-                                                      permEntry.mExpireTime,
-                                                      MOZ_UTF16("changed"));
-  }
-
-  return PL_DHASH_NEXT;
-}
-
 nsresult
 nsPermissionManager::RemoveExpiredPermissionsForApp(uint32_t aAppId)
 {
   ENSURE_NOT_CHILD_PROCESS;
 
-  if (aAppId != nsIScriptSecurityManager::NO_APP_ID) {
-    mPermissionTable.EnumerateEntries(RemoveExpiredPermissionsForAppEnumerator, &aAppId);
+  if (aAppId == nsIScriptSecurityManager::NO_APP_ID) {
+    return NS_OK;
+  }
+
+  for (auto iter = mPermissionTable.Iter(); !iter.Done(); iter.Next()) {
+    PermissionHashKey* entry = iter.Get();
+    if (entry->GetKey()->mAppId != aAppId) {
+      continue;
+    }
+
+    for (uint32_t i = 0; i < entry->GetPermissions().Length(); ++i) {
+      PermissionEntry& permEntry = entry->GetPermissions()[i];
+      if (permEntry.mExpireType != nsIPermissionManager::EXPIRE_SESSION) {
+        continue;
+      }
+
+      if (permEntry.mNonSessionExpireType ==
+            nsIPermissionManager::EXPIRE_SESSION) {
+        PermissionEntry oldPermEntry = entry->GetPermissions()[i];
+
+        entry->GetPermissions().RemoveElementAt(i);
+
+        NotifyObserversWithPermission(entry->GetKey()->mHost,
+                                      entry->GetKey()->mAppId,
+                                      entry->GetKey()->mIsInBrowserElement,
+                                      mTypeArray.ElementAt(oldPermEntry.mType),
+                                      oldPermEntry.mPermission,
+                                      oldPermEntry.mExpireType,
+                                      oldPermEntry.mExpireTime,
+                                      MOZ_UTF16("deleted"));
+
+        --i;
+        continue;
+      }
+
+      permEntry.mPermission = permEntry.mNonSessionPermission;
+      permEntry.mExpireType = permEntry.mNonSessionExpireType;
+      permEntry.mExpireTime = permEntry.mNonSessionExpireTime;
+
+      NotifyObserversWithPermission(entry->GetKey()->mHost,
+                                    entry->GetKey()->mAppId,
+                                    entry->GetKey()->mIsInBrowserElement,
+                                    mTypeArray.ElementAt(permEntry.mType),
+                                    permEntry.mPermission,
+                                    permEntry.mExpireType,
+                                    permEntry.mExpireTime,
+                                    MOZ_UTF16("changed"));
+    }
   }
 
   return NS_OK;
 }
 
 //*****************************************************************************
 //*** nsPermissionManager private methods
 //*****************************************************************************
--- a/extensions/cookie/nsPermissionManager.h
+++ b/extensions/cookie/nsPermissionManager.h
@@ -268,51 +268,16 @@ private:
                        int64_t aExpireTime,
                        int64_t aModificationTime,
                        uint32_t aAppId,
                        bool aIsInBrowserElement);
 
   nsresult RemoveExpiredPermissionsForApp(uint32_t aAppId);
 
   /**
-   * This struct has to be passed as an argument to GetPermissionsForApp.
-   * |appId| and |browserOnly| have to be defined.
-   * |permissions| will be filed with permissions that are related to the app.
-   * If |browserOnly| is true, only permissions related to a browserElement will
-   * be in |permissions|.
-   */
-  struct GetPermissionsForAppStruct {
-    uint32_t                  appId;
-    bool                      browserOnly;
-    nsCOMArray<nsIPermission> permissions;
-
-    GetPermissionsForAppStruct() = delete;
-    GetPermissionsForAppStruct(uint32_t aAppId, bool aBrowserOnly)
-      : appId(aAppId)
-      , browserOnly(aBrowserOnly)
-    {}
-  };
-
-  /**
-   * This method will return the list of all permissions that are related to a
-   * specific app.
-   * @param arg has to be an instance of GetPermissionsForAppStruct.
-   */
-  static PLDHashOperator
-  GetPermissionsForApp(PermissionHashKey* entry, void* arg);
-
-  /**
-   * This method restores an app's permissions when its session ends.
-   */
-  static PLDHashOperator
-  RemoveExpiredPermissionsForAppEnumerator(PermissionHashKey* entry,
-                                           void* nonused);
-
-
-  /**
    * This method removes all permissions modified after the specified time.
    */
   nsresult
   RemoveAllModifiedSince(int64_t aModificationTime);
 
   /**
    * Retrieve permissions from chrome process.
    */