[Extensions] Un-refcount PermissionSet

extensions/common/permissions/permissions_data.h

Index: extensions/common/permissions/permissions_data.h
diff --git a/extensions/common/permissions/permissions_data.h b/extensions/common/permissions/permissions_data.h
index 43be869029d784ff470ae8db08d9ae3ce46fcd1c..585145da276c1011626c8b70257e593769df38e4 100644
--- a/extensions/common/permissions/permissions_data.h
+++ b/extensions/common/permissions/permissions_data.h
@@ -9,6 +9,7 @@
 #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"
@@ -19,12 +20,22 @@
 
 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.
@@ -35,7 +46,8 @@ class PermissionsData {
                      // 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.
@@ -80,16 +92,22 @@ class PermissionsData {
                               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;
@@ -182,33 +200,35 @@ class PermissionsData {
   // page itself.
   bool CanCaptureVisiblePage(int tab_id, std::string* error) const;
 
-  // Returns the tab permissions map.
-  TabPermissionsMap CopyTabSpecificPermissionsMap() const;
+  const TabPermissionsMap& tab_specific_permissions() const {
+    CHECK(CalledOnValidThread());
+    return tab_specific_permissions_;
+  }
 
-  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 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();
   }
 
-  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* 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|.
@@ -241,17 +261,19 @@ class PermissionsData {
   // 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);
 };