[Extensions] Un-refcount PermissionSet

extensions/common/permissions/permissions_data.h

 // 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.
 
 #ifndef EXTENSIONS_COMMON_PERMISSIONS_PERMISSIONS_DATA_H_
 #define EXTENSIONS_COMMON_PERMISSIONS_PERMISSIONS_DATA_H_
 
 #include <map>
 #include <string>
 #include <vector>
 
+#include "base/containers/scoped_ptr_map.h"
 #include "base/memory/ref_counted.h"
 #include "base/strings/string16.h"
 #include "base/synchronization/lock.h"
 #include "extensions/common/manifest.h"
 #include "extensions/common/permissions/api_permission.h"
 #include "extensions/common/permissions/permission_message.h"
 #include "extensions/common/permissions/permission_set.h"
 
 class GURL;
 
+namespace base {
+class ThreadChecker;
+}
+
 namespace extensions {
 class Extension;
 class URLPatternSet;
 
 // A container for the permissions state of an extension, including active,
 // withheld, and tab-specific permissions.
+// Thread-Safety: Since this is an object on the Extension object, *some* thread
+// safety is provided. All utility functions for checking if a permission is
+// present or an operation is allowed are thread-safe. However, permissions can
+// only be set (or updated) on the thread on which this object was created.

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

Why is this? Just for sanity checking? Because you're locking everything anyway.
If you want to make this fancy, you need a read/write lock but I don't know if
chromium supplies that, so if not, probably not worth implementing it.

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

So this is (one of the) fun parts.  We lock, but we also allow access (and
potentially caching) of the PermissionSets via, say, active_permissions().  So
consider the possibility of:
- Thread 1 accesses permissions, stores a raw pointer on the stack.
- Thread 2 modifies the permissions, which destroys the current PermissionSet
due to the loveliness of immutability.
- Thread 1 tries to access the permission set, and the world blows up.

This is prevented by the two restrictions:
- You can only access permissions on the "main" thread.
- You can only update permissions on the "main" thread.
(I should update the part about "thread on which this object was created" to be
the thread it's locked to).

This won't protect against someone storing a raw pointer to the permissions on
their object or anything, but that's silly, and we shouldn't do that (any more
than storing a raw pointer to an extension, or anything else with questionable
lifetime).

[not at google - send to devlin, 2015/09/22 22:32:58]

I see.

So to say it in a more general way, to make sure I understand: it's possible for
Thread 1 to acquire a weak reference to the internal state of this object (a
PermissionSet), and for Thread 2 to destroy that state, which creates problems.
A solution would be to make sure that anybody that wants a weak reference to the
internal state must execute on the same thread as anybody that wants to mutate
it?

It's a bit of an odd contract there. It's interesting functionality we're trying
to provide, I've never seen anything quite like it across thread boundaries.
I've certainly never seen a lock used in this way.

I can think of 2 reasonable ways to solve this:
- Lift all operations out of the PermissionSets into PermissionsData. E.g. every
time somebody tries to call active_permissions()->HasPermission(...) replace
that with HasActivePermission(...) and so on.
- Only return copies of the PermissionSet.

I can think of 1 less reasonable way to solve this:
- Require access to be via a callback, like
WithActivePermissions(base::Callback<void(const PermissionSet&)>).

I can think of a number of unreasonable ways to solve this, including:
- Instead of returning a PermissionSet, return a PermissionSetRef where
PermissionSetRef is stores a weak reference to the PermissionSet... but if that
set is destroyed, it takes a copy. Sort of like some combination of
copy-on-write and a WeakPtr.
- Allow access to the lock.

I think my final answer would be: alright, what you have works, but it's very
subtle and the comment doesn't explain what its purpose is. The eventual
solution I'd like to see is a combination of those 2 reasonable ways: lift the
common operations on active_permissions into PermissionsData, then provide a
CopyActivePermissions crutch for any boutique logic.

+// Permissions may be accessed synchronously on that same thread.
+// Accessing on an improper thread will CHECK().

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

Brave!

Honestly I'd make this (all instances) a DCHECK, since CalledOnValidThread
returns true in release mode anyway.

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

Done.

 class PermissionsData {
  public:
   // The possible types of access for a given frame.
   enum AccessType {
     ACCESS_DENIED,   // The extension is not allowed to access the given page.
     ACCESS_ALLOWED,  // The extension is allowed to access the given page.
     ACCESS_WITHHELD  // The browser must determine if the extension can access
                      // the given page.
   };
 
-  using TabPermissionsMap = std::map<int, scoped_refptr<const PermissionSet>>;
+  using TabPermissionsMap =
+      base::ScopedPtrMap<int, scoped_ptr<const PermissionSet>>;
 
   // Delegate class to allow different contexts (e.g. browser vs renderer) to
   // have control over policy decisions.
   class PolicyDelegate {
    public:
     virtual ~PolicyDelegate() {}
 
     // Returns false if script access should be blocked on this page.
     // Otherwise, default policy should decide.
     virtual bool CanExecuteScriptOnPage(const Extension* extension,
@@ skipping 24 matching lines @@
   // with the given |extension_id|.
   static bool ShouldSkipPermissionWarnings(const std::string& extension_id);
 
   // Returns true if the given |url| is restricted for the given |extension|,
   // as is commonly the case for chrome:// urls.
   // NOTE: You probably want to use CanAccessPage().
   static bool IsRestrictedUrl(const GURL& document_url,
                               const Extension* extension,
                               std::string* error);
 
+  // Locks the permissions data to the current thread. We don't do this on
+  // construction, since extensions are initialized across multiple threads.
+  void LockToThread() const;

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

"Lock" is an unfortunate term to be using, since it implies a lock in the
synchronisation sense. The terminology I usually see is either "attach" or
"bind". MessageLoop uses "BindToCurrentThread".

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

Done.

+
   // Sets the runtime permissions of the given |extension| to |active| and
   // |withheld|.
-  void SetPermissions(const scoped_refptr<const PermissionSet>& active,
-                      const scoped_refptr<const PermissionSet>& withheld) const;
+  void SetPermissions(scoped_ptr<const PermissionSet> active,
+                      scoped_ptr<const PermissionSet> withheld) const;
+
+  // Sets the active permissions, leaving withheld the same.
+  void SetActivePermissions(scoped_ptr<const PermissionSet> active) const;
 
   // Updates the tab-specific permissions of |tab_id| to include those from
   // |permissions|.
-  void UpdateTabSpecificPermissions(
-      int tab_id,
-      scoped_refptr<const PermissionSet> permissions) const;
+  void UpdateTabSpecificPermissions(int tab_id,
+                                    const PermissionSet& permissions) const;
 
   // Clears the tab-specific permissions of |tab_id|.
   void ClearTabSpecificPermissions(int tab_id) const;
 
   // Returns true if the |extension| has the given |permission|. Prefer
   // IsExtensionWithPermissionOrSuggestInConsole when developers may be using an
   // api that requires a permission they didn't know about, e.g. open web apis.
   // Note this does not include APIs with no corresponding permission, like
   // "runtime" or "browserAction".
   // TODO(mpcomplete): drop the "API" from these names, it's confusing.
@@ skipping 72 matching lines @@
                                     int tab_id,
                                     int process_id,
                                     std::string* error) const;
 
   // Returns true if extension is allowed to obtain the contents of a page as
   // an image. Since a page may contain sensitive information, this is
   // restricted to the extension's host permissions as well as the extension
   // page itself.
   bool CanCaptureVisiblePage(int tab_id, std::string* error) const;
 
-  // Returns the tab permissions map.
-  TabPermissionsMap CopyTabSpecificPermissionsMap() const;
-
-  scoped_refptr<const PermissionSet> active_permissions() const {
-    // We lock so that we can't also be setting the permissions while returning.
-    base::AutoLock auto_lock(runtime_lock_);
-    return active_permissions_unsafe_;
+  const TabPermissionsMap& tab_specific_permissions() const {
+    CHECK(CalledOnValidThread());
+    return tab_specific_permissions_;
   }
 
-  scoped_refptr<const PermissionSet> withheld_permissions() const {
-    // We lock so that we can't also be setting the permissions while returning.
-    base::AutoLock auto_lock(runtime_lock_);
-    return withheld_permissions_unsafe_;
+  const PermissionSet* active_permissions() const {

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

(noting place which should be a reference)

+    CHECK(CalledOnValidThread());
+    return active_permissions_unsafe_.get();
+  }
+
+  const PermissionSet* withheld_permissions() const {
+    CHECK(CalledOnValidThread());
+    return withheld_permissions_unsafe_.get();
   }
 
 #if defined(UNIT_TEST)
-  scoped_refptr<const PermissionSet> GetTabSpecificPermissionsForTesting(
-      int tab_id) const {
+  const PermissionSet* GetTabSpecificPermissionsForTesting(int tab_id) const {
+    base::AutoLock auto_lock(runtime_lock_);
     return GetTabSpecificPermissions(tab_id);
   }
 #endif
 
  private:
+  // Returns true if this is called on a valid thread.
+  bool CalledOnValidThread() const;
+
   // Gets the tab-specific host permissions of |tab_id|, or NULL if there
   // aren't any.

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

Mention "must be called with |runtime_lock_| acquired.

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

Done.

-  scoped_refptr<const PermissionSet> GetTabSpecificPermissions(
-      int tab_id) const;
+  const PermissionSet* GetTabSpecificPermissions(int tab_id) const;
 
   // Returns true if the |extension| has tab-specific permission to operate on
   // the tab specified by |tab_id| with the given |url|.
   // Note that if this returns false, it doesn't mean the extension can't run on
   // the given tab, only that it does not have tab-specific permission to do so.
   bool HasTabSpecificPermissionToExecuteScript(int tab_id,
                                                const GURL& url) const;
 
   // Returns whether or not the extension is permitted to run on the given page,
   // checking against |permitted_url_patterns| in addition to blocking special
@@ skipping 12 matching lines @@
   // The associated extension's manifest type.
   Manifest::Type manifest_type_;
 
   mutable base::Lock runtime_lock_;
 
   // The permission's which are currently active on the extension during
   // runtime.
   // Unsafe indicates that we must lock anytime this is directly accessed.
   // Unless you need to change |active_permissions_unsafe_|, use the (safe)
   // active_permissions() accessor.
-  mutable scoped_refptr<const PermissionSet> active_permissions_unsafe_;
+  mutable scoped_ptr<const PermissionSet> active_permissions_unsafe_;
 
   // The permissions the extension requested, but was not granted due because
   // they are too powerful. This includes things like all_hosts.
   // Unsafe indicates that we must lock anytime this is directly accessed.
   // Unless you need to change |withheld_permissions_unsafe_|, use the (safe)
   // withheld_permissions() accessor.
-  mutable scoped_refptr<const PermissionSet> withheld_permissions_unsafe_;
+  mutable scoped_ptr<const PermissionSet> withheld_permissions_unsafe_;
 
   mutable TabPermissionsMap tab_specific_permissions_;
 
+  mutable scoped_ptr<base::ThreadChecker> thread_checker_;
+
   DISALLOW_COPY_AND_ASSIGN(PermissionsData);
 };
 
 }  // namespace extensions
 
 #endif  // EXTENSIONS_COMMON_PERMISSIONS_PERMISSIONS_DATA_H_