[Extensions] Un-refcount PermissionSet

extensions/common/permissions/permissions_data.cc

Index: extensions/common/permissions/permissions_data.cc
diff --git a/extensions/common/permissions/permissions_data.cc b/extensions/common/permissions/permissions_data.cc
index 7fdbd2c36a79eea46b681b1f3ef464be2be27b15..3b944f4a17371660b190d183266d405bd6ce5cba 100644
--- a/extensions/common/permissions/permissions_data.cc
+++ b/extensions/common/permissions/permissions_data.cc
@@ -5,6 +5,7 @@
 #include "extensions/common/permissions/permissions_data.h"
 
 #include "base/command_line.h"
+#include "base/threading/thread_checker.h"
 #include "content/public/common/url_constants.h"
 #include "extensions/common/constants.h"
 #include "extensions/common/error_utils.h"
@@ -30,14 +31,14 @@ PermissionsData::PolicyDelegate* g_policy_delegate = NULL;
 PermissionsData::PermissionsData(const Extension* extension)
     : extension_id_(extension->id()), manifest_type_(extension->GetType()) {
   base::AutoLock auto_lock(runtime_lock_);

[not at google - send to devlin, 2015/09/22 21:51:26]

I know the code is like this already, but it seems impossible that a lock here
is going to do anything, since at this point the object hasn't even be
constructed, so isn't usable.

[Devlin, 2015/09/23 17:09:00]

heh, true.  Removed.

-  scoped_refptr<const PermissionSet> required_permissions =
+  const PermissionSet* required_permissions =
       PermissionsParser::GetRequiredPermissions(extension);
-  active_permissions_unsafe_ =
+  active_permissions_unsafe_.reset(
       new PermissionSet(required_permissions->apis(),
                         required_permissions->manifest_permissions(),
                         required_permissions->explicit_hosts(),
-                        required_permissions->scriptable_hosts());
-  withheld_permissions_unsafe_ = new PermissionSet();
+                        required_permissions->scriptable_hosts()));
+  withheld_permissions_unsafe_.reset(new PermissionSet());
 }
 
 PermissionsData::~PermissionsData() {
@@ -121,39 +122,57 @@ bool PermissionsData::IsRestrictedUrl(const GURL& document_url,
   return false;
 }
 
+void PermissionsData::LockToThread() const {
+  DCHECK(!thread_checker_);
+  thread_checker_.reset(new base::ThreadChecker());
+}
+
 void PermissionsData::SetPermissions(
-    const scoped_refptr<const PermissionSet>& active,
-    const scoped_refptr<const PermissionSet>& withheld) const {
+    scoped_ptr<const PermissionSet> active,
+    scoped_ptr<const PermissionSet> withheld) const {
+  CHECK(CalledOnValidThread());
+  base::AutoLock auto_lock(runtime_lock_);
+  active_permissions_unsafe_ = active.Pass();
+  withheld_permissions_unsafe_ = withheld.Pass();
+}
+
+void PermissionsData::SetActivePermissions(
+    scoped_ptr<const PermissionSet> active) const {
+  CHECK(CalledOnValidThread());
   base::AutoLock auto_lock(runtime_lock_);
-  active_permissions_unsafe_ = active;
-  withheld_permissions_unsafe_ = withheld;
+  active_permissions_unsafe_ = active.Pass();
 }
 
 void PermissionsData::UpdateTabSpecificPermissions(
     int tab_id,
-    scoped_refptr<const PermissionSet> permissions) const {
+    const PermissionSet& permissions) const {
+  CHECK(CalledOnValidThread());
   base::AutoLock auto_lock(runtime_lock_);
   CHECK_GE(tab_id, 0);
-  TabPermissionsMap::iterator iter = tab_specific_permissions_.find(tab_id);
-  if (iter == tab_specific_permissions_.end())
-    tab_specific_permissions_[tab_id] = permissions;
-  else
-    iter->second = PermissionSet::CreateUnion(*(iter->second), *permissions);
+  TabPermissionsMap::const_iterator iter =
+      tab_specific_permissions_.find(tab_id);
+  scoped_ptr<const PermissionSet> new_permissions = PermissionSet::CreateUnion(
+      iter == tab_specific_permissions_.end() ? PermissionSet() : *iter->second,
+      permissions);
+  tab_specific_permissions_.set(tab_id, new_permissions.Pass());
 }
 
 void PermissionsData::ClearTabSpecificPermissions(int tab_id) const {
+  CHECK(CalledOnValidThread());
   base::AutoLock auto_lock(runtime_lock_);

[not at google - send to devlin, 2015/09/22 21:51:26]

Thought: you could tighten this up a bit with a class that does both of these:
asserts that it's called on a valid thread, and then acquires the lock for that
thread. E.g.:

class AutoLockOnValidThread {
 public:
  AutoLockOnValidThread(base::Lock& lock, base::ThreadChecker* thread_checker)
      : auto_lock_(lock) {
    // Or maybe "thread_checker && ...".
    DCHECK(!thread_checker || thread_checker->CalledOnValidThread());
    // Note this has reversed the order of the lock acquiring and thread
    // checking, but I don't think that matters.
  }

 private:
  base::AutoLock auto_lock_;
};

This would remove the need for the PermissionsData::CalledOnValidThread method.

[Devlin, 2015/09/23 17:08:59]

Done.

   CHECK_GE(tab_id, 0);
   tab_specific_permissions_.erase(tab_id);
 }
 
 bool PermissionsData::HasAPIPermission(APIPermission::ID permission) const {
-  return active_permissions()->HasAPIPermission(permission);
+  base::AutoLock auto_lock(runtime_lock_);
+  return active_permissions_unsafe_->HasAPIPermission(permission);
 }
 
 bool PermissionsData::HasAPIPermission(
     const std::string& permission_name) const {
-  return active_permissions()->HasAPIPermission(permission_name);
+  base::AutoLock auto_lock(runtime_lock_);
+  return active_permissions_unsafe_->HasAPIPermission(permission_name);
 }
 
 bool PermissionsData::HasAPIPermissionForTab(
@@ -162,19 +181,19 @@ bool PermissionsData::HasAPIPermissionForTab(
   if (HasAPIPermission(permission))
     return true;
 
-  scoped_refptr<const PermissionSet> tab_permissions =
-      GetTabSpecificPermissions(tab_id);
-
-  // Place autolock below the HasAPIPermission() and
-  // GetTabSpecificPermissions(), since each already acquires the lock.
+  // Place autolock below the HasAPIPermission() since it already acquires the
+  // lock.

[not at google - send to devlin, 2015/09/22 21:51:26]

I'd be more comfortable if locks were held for the entirety of these methods, so
that they're atomic.

Pretty easy to fix here: put the auto_lock up top, then instead of using
HasAPIPermission(permission), unwrap into
active_permissions_unsafe_->HasAPIPermission(permission).

[Devlin, 2015/09/23 17:08:59]

Done.

   base::AutoLock auto_lock(runtime_lock_);
-  return tab_permissions.get() && tab_permissions->HasAPIPermission(permission);
+  const PermissionSet* tab_permissions = GetTabSpecificPermissions(tab_id);
+  return tab_permissions && tab_permissions->HasAPIPermission(permission);
 }
 
 bool PermissionsData::CheckAPIPermissionWithParam(
     APIPermission::ID permission,
     const APIPermission::CheckParam* param) const {
-  return active_permissions()->CheckAPIPermissionWithParam(permission, param);
+  base::AutoLock auto_lock(runtime_lock_);
+  return active_permissions_unsafe_->CheckAPIPermissionWithParam(permission,
+                                                                 param);
 }
 
 URLPatternSet PermissionsData::GetEffectiveHostPermissions() const {
@@ -186,24 +205,28 @@ URLPatternSet PermissionsData::GetEffectiveHostPermissions() const {
 }
 
 bool PermissionsData::HasHostPermission(const GURL& url) const {
-  return active_permissions()->HasExplicitAccessToOrigin(url);
+  base::AutoLock auto_lock(runtime_lock_);
+  return active_permissions_unsafe_->HasExplicitAccessToOrigin(url);
 }
 
 bool PermissionsData::HasEffectiveAccessToAllHosts() const {
-  return active_permissions()->HasEffectiveAccessToAllHosts();
+  base::AutoLock auto_lock(runtime_lock_);
+  return active_permissions_unsafe_->HasEffectiveAccessToAllHosts();
 }
 
 PermissionMessages PermissionsData::GetPermissionMessages() const {
+  base::AutoLock auto_lock(runtime_lock_);
   return PermissionMessageProvider::Get()->GetPermissionMessages(
       PermissionMessageProvider::Get()->GetAllPermissionIDs(
-          active_permissions().get(), manifest_type_));
+          active_permissions_unsafe_.get(), manifest_type_));
 }
 
 bool PermissionsData::HasWithheldImpliedAllHosts() const {
+  base::AutoLock auto_lock(runtime_lock_);
   // Since we currently only withhold all_hosts, it's sufficient to check
   // that either set is not empty.
-  return !withheld_permissions()->explicit_hosts().is_empty() ||
-         !withheld_permissions()->scriptable_hosts().is_empty();
+  return !withheld_permissions_unsafe_->explicit_hosts().is_empty() ||
+         !withheld_permissions_unsafe_->scriptable_hosts().is_empty();
 }
 
 bool PermissionsData::CanAccessPage(const Extension* extension,
@@ -211,13 +234,11 @@ bool PermissionsData::CanAccessPage(const Extension* extension,
                                     int tab_id,
                                     int process_id,
                                     std::string* error) const {
-  AccessType result = CanRunOnPage(extension,
-                                   document_url,
-                                   tab_id,
-                                   process_id,
-                                   active_permissions()->explicit_hosts(),
-                                   withheld_permissions()->explicit_hosts(),
-                                   error);
+  base::AutoLock auto_lock(runtime_lock_);
+  AccessType result =
+      CanRunOnPage(extension, document_url, tab_id, process_id,
+                   active_permissions_unsafe_->explicit_hosts(),
+                   withheld_permissions_unsafe_->explicit_hosts(), error);
   // TODO(rdevlin.cronin) Update callers so that they only need ACCESS_ALLOWED.
   return result == ACCESS_ALLOWED || result == ACCESS_WITHHELD;
 }
@@ -228,13 +249,10 @@ PermissionsData::AccessType PermissionsData::GetPageAccess(
     int tab_id,
     int process_id,
     std::string* error) const {
-  return CanRunOnPage(extension,
-                      document_url,
-                      tab_id,
-                      process_id,
-                      active_permissions()->explicit_hosts(),
-                      withheld_permissions()->explicit_hosts(),
-                      error);
+  base::AutoLock auto_lock(runtime_lock_);
+  return CanRunOnPage(extension, document_url, tab_id, process_id,
+                      active_permissions_unsafe_->explicit_hosts(),
+                      withheld_permissions_unsafe_->explicit_hosts(), error);
 }
 
 bool PermissionsData::CanRunContentScriptOnPage(const Extension* extension,
@@ -242,13 +260,11 @@ bool PermissionsData::CanRunContentScriptOnPage(const Extension* extension,
                                                 int tab_id,
                                                 int process_id,
                                                 std::string* error) const {
-  AccessType result = CanRunOnPage(extension,
-                                   document_url,
-                                   tab_id,
-                                   process_id,
-                                   active_permissions()->scriptable_hosts(),
-                                   withheld_permissions()->scriptable_hosts(),
-                                   error);
+  base::AutoLock auto_lock(runtime_lock_);
+  AccessType result =
+      CanRunOnPage(extension, document_url, tab_id, process_id,
+                   active_permissions_unsafe_->scriptable_hosts(),
+                   withheld_permissions_unsafe_->scriptable_hosts(), error);
   // TODO(rdevlin.cronin) Update callers so that they only need ACCESS_ALLOWED.
   return result == ACCESS_ALLOWED || result == ACCESS_WITHHELD;
 }
@@ -259,13 +275,10 @@ PermissionsData::AccessType PermissionsData::GetContentScriptAccess(
     int tab_id,
     int process_id,
     std::string* error) const {
-  return CanRunOnPage(extension,
-                      document_url,
-                      tab_id,
-                      process_id,
-                      active_permissions()->scriptable_hosts(),
-                      withheld_permissions()->scriptable_hosts(),
-                      error);
+  base::AutoLock auto_lock(runtime_lock_);
+  return CanRunOnPage(extension, document_url, tab_id, process_id,
+                      active_permissions_unsafe_->scriptable_hosts(),
+                      withheld_permissions_unsafe_->scriptable_hosts(), error);
 }
 
 bool PermissionsData::CanCaptureVisiblePage(int tab_id,
@@ -273,13 +286,13 @@ bool PermissionsData::CanCaptureVisiblePage(int tab_id,
   const URLPattern all_urls(URLPattern::SCHEME_ALL,
                             URLPattern::kAllUrlsPattern);
 
-  if (active_permissions()->explicit_hosts().ContainsPattern(all_urls))
+  base::AutoLock auto_lock(runtime_lock_);
+  if (active_permissions_unsafe_->explicit_hosts().ContainsPattern(all_urls))
     return true;
 
   if (tab_id >= 0) {
-    scoped_refptr<const PermissionSet> tab_permissions =
-        GetTabSpecificPermissions(tab_id);
-    if (tab_permissions.get() &&
+    const PermissionSet* tab_permissions = GetTabSpecificPermissions(tab_id);
+    if (tab_permissions &&
         tab_permissions->HasAPIPermission(APIPermission::kTab)) {
       return true;
     }
@@ -293,28 +306,26 @@ bool PermissionsData::CanCaptureVisiblePage(int tab_id,
   return false;
 }
 
-PermissionsData::TabPermissionsMap
-PermissionsData::CopyTabSpecificPermissionsMap() const {
-  base::AutoLock auto_lock(runtime_lock_);
-  return tab_specific_permissions_;
+bool PermissionsData::CalledOnValidThread() const {
+  return !thread_checker_ || thread_checker_->CalledOnValidThread();

[not at google - send to devlin, 2015/09/22 21:51:26]

I'd find "return thread_checker_ && thread_checker_->CalledOnValidThread" to be
equally valid. Why would the set *not* be bound to some thread at this point?

[Devlin, 2015/09/22 22:08:45]

It's not bound to a thread during construction, because constructing an
extension is a wonderful bounce-around-all-the-threads type of function.  I
fooled around with having it default to locking on the construction thread, and
then exposing the ability to reassign the thread, but with the amount of hopping
we do, it's pretty crazy.  That said, I still *think* this is safe because
during initialization (the only time it's not locked), only one thread will be
accessing the object at a time.

 }
 
-scoped_refptr<const PermissionSet> PermissionsData::GetTabSpecificPermissions(
+const PermissionSet* PermissionsData::GetTabSpecificPermissions(
     int tab_id) const {
-  base::AutoLock auto_lock(runtime_lock_);
   CHECK_GE(tab_id, 0);
+  runtime_lock_.AssertAcquired();
   TabPermissionsMap::const_iterator iter =
       tab_specific_permissions_.find(tab_id);
-  return (iter != tab_specific_permissions_.end()) ? iter->second : NULL;
+  return (iter != tab_specific_permissions_.end()) ? iter->second : nullptr;
 }
 
 bool PermissionsData::HasTabSpecificPermissionToExecuteScript(
     int tab_id,
     const GURL& url) const {
+  runtime_lock_.AssertAcquired();
   if (tab_id >= 0) {
-    scoped_refptr<const PermissionSet> tab_permissions =
-        GetTabSpecificPermissions(tab_id);
-    if (tab_permissions.get() &&
+    const PermissionSet* tab_permissions = GetTabSpecificPermissions(tab_id);
+    if (tab_permissions &&
         tab_permissions->explicit_hosts().MatchesSecurityOrigin(url)) {
       return true;
     }
@@ -330,6 +341,7 @@ PermissionsData::AccessType PermissionsData::CanRunOnPage(
     const URLPatternSet& permitted_url_patterns,
     const URLPatternSet& withheld_url_patterns,
     std::string* error) const {
+  runtime_lock_.AssertAcquired();
   if (g_policy_delegate &&
       !g_policy_delegate->CanExecuteScriptOnPage(
           extension, document_url, tab_id, process_id, error)) {