[Extensions] Un-refcount PermissionSet

[Extensions] Un-refcount PermissionSet

PermissionSet represents a set of permissions, and for some reason,
it's been refcounted. There's really no reason to have it, and it
makes everything more costly and difficult to reason about. Remove the
refcounting.

Note: This is part 1 of a 2-part series. This removes the ref-counting.
In a followup, I'll go through and update many of the places that
use const PermissionSet* and convert to const &.

BUG=455414
TBR=thestig@chromium.org (misc chrome files with ptr conversion)
TBR=nasko@chromium.org (extension messages - not actually changing any IPC messages)

Committed: https://crrev.com/e2d0fd0ad14bfd0cd82e0093b1488103af532c8e
Cr-Commit-Position: refs/heads/master@{#350684}

[Devlin, 2015-09-21 20:45:37]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-09-21 20:45:44]

Patchset #1 (id:2) has been deleted

[Devlin, 2015-09-21 20:45:51]

Patchset #1 (id:20050) has been deleted

[Devlin, 2015-09-21 20:45:57]

Patchset #1 (id:40001) has been deleted

[Devlin, 2015-09-21 20:46:03]

Patchset #1 (id:60001) has been deleted

[Devlin, 2015-09-21 20:48:16]

rdevlin.cronin@chromium.org changed reviewers:
+ kalman@chromium.org

[Devlin, 2015-09-21 20:48:17]

IT PASSES TRYBOT.

A few notes:
- Please hold off on any comments about replacing pointers with const& or
on-the-stack objects.  Those will come in a follow-up.
- Please pay extra-close attention to threading.  I *think* this is okay, but
extra eyes are great.

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

Gnarly. A couple of comments.

https://codereview.chromium.org/1349613003/diff/80001/chrome/browser/extensio...
File chrome/browser/extensions/api/permissions/permissions_api_helpers.h
(right):

https://codereview.chromium.org/1349613003/diff/80001/chrome/browser/extensio...
chrome/browser/extensions/api/permissions/permissions_api_helpers.h:30: const
PermissionSet* set);
(noting place which should be a reference)

https://codereview.chromium.org/1349613003/diff/80001/extensions/browser/exte...
File extensions/browser/extension_prefs.h (right):

https://codereview.chromium.org/1349613003/diff/80001/extensions/browser/exte...
extensions/browser/extension_prefs.h:363: const PermissionSet* permissions);
(noting places which should be a reference)

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/manif...
File extensions/common/manifest_handlers/permissions_parser.h (right):

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/manif...
extensions/common/manifest_handlers/permissions_parser.h:48: static const
PermissionSet* GetRequiredPermissions(
(noting place which should be a reference)

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
File extensions/common/permissions/permission_set.cc (right):

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permission_set.cc:125: explicit_hosts_,
scriptable_hosts_));
It would be good to copy the should_warn_all_hosts_ flag as well. With respect
to the other Clone() comment, how about adding a private copy constructor, and
then making this method simply "return make_scoped_ptr(new
PermissionSet(this))"?

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
File extensions/common/permissions/permission_set.h (right):

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permission_set.h:62: scoped_ptr<const
PermissionSet> Clone() const;
It's a bit odd to have a Clone() method but no DISALLOW_COPY_AND_ASSIGN. With
respect to other comment in Clone(), I'd make this DISALLOW_ASSIGN and then add
a private copy constructor.

(or just make it copy and assignable and do away with the Clone(), but I do like
the explicit copy)

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permission_set.h:162: // Whether or not this
permission set includes access to so many origins, we
Unrelated: this comment is wrong. It should just be "Cache whether this set
implies access to all hosts, because it's non-trivial to compute."

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:33: base::AutoLock
auto_lock(runtime_lock_);
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.

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:78: 
Cleanup: add // static here.

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:162: base::AutoLock
auto_lock(runtime_lock_);
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.

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:185: // lock.
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).

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:310: return !thread_checker_
|| thread_checker_->CalledOnValidThread();
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?

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
File extensions/common/permissions/permissions_data.h (right):

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.h:36: // only be set (or updated)
on the thread on which this object was created.
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.

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.h:38: // Accessing on an improper
thread will CHECK().
Brave!

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

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.h:97: void LockToThread() const;
"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".

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.h:208: const PermissionSet*
active_permissions() const {
(noting place which should be a reference)

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.h:230: // aren't any.
Mention "must be called with |runtime_lock_| acquired.

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

No code changes, just replying to two comments.

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:310: return !thread_checker_
|| thread_checker_->CalledOnValidThread();
On 2015/09/22 21:51:26, kalman wrote:
> 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?

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.

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
File extensions/common/permissions/permissions_data.h (right):

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.h:36: // only be set (or updated)
on the thread on which this object was created.
On 2015/09/22 21:51:26, kalman wrote:
> 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.

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]

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
File extensions/common/permissions/permissions_data.h (right):

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.h:36: // only be set (or updated)
on the thread on which this object was created.
On 2015/09/22 22:08:45, Devlin wrote:
> On 2015/09/22 21:51:26, kalman wrote:
> > 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.
> 
> 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).

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.

[Devlin, 2015-09-23 16:52:22]

On 2015/09/22 22:32:58, kalman wrote:
>
https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
> File extensions/common/permissions/permissions_data.h (right):
> 
>
https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
> extensions/common/permissions/permissions_data.h:36: // only be set (or
updated)
> on the thread on which this object was created.
> On 2015/09/22 22:08:45, Devlin wrote:
> > On 2015/09/22 21:51:26, kalman wrote:
> > > 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.
> > 
> > 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).
> 
> 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.
Right.

> 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?
Correct, that's what we have here.

> 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.
It's the culmination of many lies and questionable designs. :)  PermissionsData
is "const", but it's not (if it were, this wouldn't be a problem). 
PermissionSets are immutable, meaning we must destroy any permission set when we
change it (if you could just update a permission set, again, not a problem).  A
few other things factor in.

That said, if you take a step back, it's not totally unheard of.  It's basically
just a variable that is instantiated on one thread, but accessed on many. 
That's actually quite common.  I agree, though, everything together makes it
quite weird.

> 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 thought about this, and it may be worthwhile, but I didn't want to
unnecessarily perform (potentially expensive-ish) copies.  I think most of the
time that we actually need the PermissionSet it's when we're changing
permissions, so it wouldn't be too frequent, but...

> 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&)>).
Yuck.  That makes doing something like getting a PermissionSet intersection of
|foo| and |active_permissions| a right pain, especially without lambda binding.

> 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 agree, those are unreasonable. ;)
 
> 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.

I think that is, overall, reasonable.  It would be a good followup, though it
would be a little churny, and, unfortunately, a net-loss from a performance
stand point.  But, if it only happens on updating permissions, might not be too
bad...

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

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
File extensions/common/permissions/permission_set.cc (right):

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permission_set.cc:125: explicit_hosts_,
scriptable_hosts_));
On 2015/09/22 21:51:26, kalman wrote:
> It would be good to copy the should_warn_all_hosts_ flag as well. With respect
> to the other Clone() comment, how about adding a private copy constructor, and
> then making this method simply "return make_scoped_ptr(new
> PermissionSet(this))"?

I like it.  Done.

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
File extensions/common/permissions/permission_set.h (right):

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permission_set.h:62: scoped_ptr<const
PermissionSet> Clone() const;
On 2015/09/22 21:51:26, kalman wrote:
> It's a bit odd to have a Clone() method but no DISALLOW_COPY_AND_ASSIGN. With
> respect to other comment in Clone(), I'd make this DISALLOW_ASSIGN and then
add
> a private copy constructor.
> 
> (or just make it copy and assignable and do away with the Clone(), but I do
like
> the explicit copy)

Done.

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permission_set.h:162: // Whether or not this
permission set includes access to so many origins, we
On 2015/09/22 21:51:26, kalman wrote:
> Unrelated: this comment is wrong. It should just be "Cache whether this set
> implies access to all hosts, because it's non-trivial to compute."

Done.

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
File extensions/common/permissions/permissions_data.cc (right):

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:33: base::AutoLock
auto_lock(runtime_lock_);
On 2015/09/22 21:51:26, kalman wrote:
> 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.

heh, true.  Removed.

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:78: 
On 2015/09/22 21:51:26, kalman wrote:
> Cleanup: add // static here.

Done.

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:162: base::AutoLock
auto_lock(runtime_lock_);
On 2015/09/22 21:51:26, kalman wrote:
> 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.

Done.

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.cc:185: // lock.
On 2015/09/22 21:51:26, kalman wrote:
> 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).

Done.

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
File extensions/common/permissions/permissions_data.h (right):

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.h:38: // Accessing on an improper
thread will CHECK().
On 2015/09/22 21:51:26, kalman wrote:
> Brave!
> 
> Honestly I'd make this (all instances) a DCHECK, since CalledOnValidThread
> returns true in release mode anyway.

Done.

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.h:97: void LockToThread() const;
On 2015/09/22 21:51:27, kalman wrote:
> "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".

Done.

https://codereview.chromium.org/1349613003/diff/80001/extensions/common/permi...
extensions/common/permissions/permissions_data.h:230: // aren't any.
On 2015/09/22 21:51:27, kalman wrote:
> Mention "must be called with |runtime_lock_| acquired.

Done.

[not at google - send to devlin, 2015-09-23 18:30:20]

> That said, if you take a step back, it's not totally unheard of.  It's
basically
> just a variable that is instantiated on one thread, but accessed on many.

No it's not. It's a variable created on one thread which can only be returned on
that same thread. However, it can be accessed internally on any thread.

[not at google - send to devlin, 2015-09-23 20:24:19]

lgtm

[Devlin, 2015-09-24 22:14:29]

rdevlin.cronin@chromium.org changed reviewers:
+ nasko@chromium.org, thestig@chromium.org

[Devlin, 2015-09-24 22:15:09]

The CQ bit was checked by rdevlin.cronin@chromium.org

[Devlin, 2015-09-24 22:15:10]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org
Link to the patchset: https://codereview.chromium.org/1349613003/#ps140001
(title: "Rebase")

[commit-bot: I haz the power, 2015-09-24 22:17:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1349613003/140001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1349613003/140001

[commit-bot: I haz the power, 2015-09-24 22:35:59]

Committed patchset #4 (id:140001)

[commit-bot: I haz the power, 2015-09-24 22:36:45]

Patchset 4 (id:??) landed as
https://crrev.com/e2d0fd0ad14bfd0cd82e0093b1488103af532c8e
Cr-Commit-Position: refs/heads/master@{#350684}