[Extensions] Un-refcount PermissionSet

extensions/common/permissions/permissions_data.cc

 // Copyright (c) 2013 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #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"
 #include "extensions/common/extension.h"
 #include "extensions/common/extensions_client.h"
 #include "extensions/common/manifest.h"
 #include "extensions/common/manifest_constants.h"
 #include "extensions/common/manifest_handlers/permissions_parser.h"
 #include "extensions/common/permissions/permission_message_provider.h"
 #include "extensions/common/switches.h"
 #include "extensions/common/url_pattern_set.h"
 #include "url/gurl.h"
 #include "url/url_constants.h"
 
 namespace extensions {
 
 namespace {
 
 PermissionsData::PolicyDelegate* g_policy_delegate = NULL;
 
 }  // namespace
 
 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() {
 }
 
 // static
 void PermissionsData::SetPolicyDelegate(PolicyDelegate* delegate) {
   g_policy_delegate = delegate;
 }
 
@@ skipping 16 matching lines @@
   // An extension may require user action to execute scripts iff the extension
   // shows up in chrome:extensions (so the user can grant withheld permissions),
   // is not part of chrome or corporate policy, not on the scripting whitelist,
   // and requires enough permissions that we should withhold them.
   return extension->ShouldDisplayInExtensionSettings() &&
       !Manifest::IsPolicyLocation(extension->location()) &&
       !Manifest::IsComponentLocation(extension->location()) &&
       !CanExecuteScriptEverywhere(extension) &&
       permissions->ShouldWarnAllHosts();
 }
 

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

Cleanup: add // static here.

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

Done.

 bool PermissionsData::ShouldSkipPermissionWarnings(
     const std::string& extension_id) {
   // See http://b/4946060 for more details.
   return extension_id == extension_misc::kProdHangoutsExtensionId;
 }
 
 // static
 bool PermissionsData::IsRestrictedUrl(const GURL& document_url,
                                       const Extension* extension,
                                       std::string* error) {
@@ skipping 26 matching lines @@
   if (extension && document_url.SchemeIs(kExtensionScheme) &&
       document_url.host() != extension->id() && !allow_on_chrome_urls) {
     if (error)
       *error = manifest_errors::kCannotAccessExtensionUrl;
     return true;
   }
 
   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;
-  withheld_permissions_unsafe_ = withheld;
+  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.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(
     int tab_id,
     APIPermission::ID permission) const {
   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 {
   base::AutoLock auto_lock(runtime_lock_);
   URLPatternSet effective_hosts = active_permissions_unsafe_->effective_hosts();
   for (const auto& val : tab_specific_permissions_)
     effective_hosts.AddPatterns(val.second->effective_hosts());
   return effective_hosts;
 }
 
 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,
                                     const GURL& document_url,
                                     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;
 }
 
 PermissionsData::AccessType PermissionsData::GetPageAccess(
     const Extension* extension,
     const GURL& document_url,
     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,
                                                 const GURL& document_url,
                                                 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;
 }
 
 PermissionsData::AccessType PermissionsData::GetContentScriptAccess(
     const Extension* extension,
     const GURL& document_url,
     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,
                                             std::string* error) const {
   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;
     }
     if (error)
       *error = manifest_errors::kActiveTabPermissionNotGranted;
     return false;
   }
 
   if (error)
     *error = manifest_errors::kAllURLOrActiveTabNeeded;
   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;
     }
   }
   return false;
 }
 
 PermissionsData::AccessType PermissionsData::CanRunOnPage(
     const Extension* extension,
     const GURL& document_url,
     int tab_id,
     int process_id,
     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)) {
     return ACCESS_DENIED;
   }
 
   if (IsRestrictedUrl(document_url, extension, error))
     return ACCESS_DENIED;
 
   if (HasTabSpecificPermissionToExecuteScript(tab_id, document_url))
     return ACCESS_ALLOWED;
 
   if (permitted_url_patterns.MatchesURL(document_url))
     return ACCESS_ALLOWED;
 
   if (withheld_url_patterns.MatchesURL(document_url))
     return ACCESS_WITHHELD;
 
   if (error) {
     *error = ErrorUtils::FormatErrorMessage(manifest_errors::kCannotAccessPage,
                                             document_url.spec());
   }
   return ACCESS_DENIED;
 }
 
 }  // namespace extensions