[Extensions] Fix thread-safety bug in PermissionsData

[Extensions] Fix thread-safety bug in PermissionsData

Since PermissionsData was returning a const & to a scoped_refptr in the
permission accessors, the auto lock wasn't sufficient. Adjust so that a regular
scoped_refptr is returned.

BUG=465185
Committed: https://crrev.com/db43894584558054b52e29f7b769f2fd2044f6d2
Cr-Commit-Position: refs/heads/master@{#326712}

[Devlin, 2015-04-23 23:37:41]

Patchset #1 (id:1) has been deleted

[Devlin, 2015-04-23 23:37:50]

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

[Jeffrey Yasskin, 2015-04-23 23:45:41]

https://codereview.chromium.org/1057753003/diff/20001/extensions/common/exten...
File extensions/common/extension_messages.cc (right):

https://codereview.chromium.org/1057753003/diff/20001/extensions/common/exten...
extensions/common/extension_messages.cc:63:
extensions::PermissionsData::TabPermissionsMap tab_permissions =
This is a whole std::map, so copying it could be pretty expensive. Can you have
the getter do the iteration?

Otherwise, call it CopyTabSpecific...() to be clear there's a pretty high cost.

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

https://codereview.chromium.org/1057753003/diff/20001/extensions/common/permi...
extensions/common/permissions/permissions_data.h:205: TabPermissionsMap
GetTabSpecificPermissionsMap() const;
I'm nervous about returning a std::map by value.

[Devlin, 2015-04-23 23:54:44]

No changes - clarifying question.

https://codereview.chromium.org/1057753003/diff/20001/extensions/common/exten...
File extensions/common/extension_messages.cc (right):

https://codereview.chromium.org/1057753003/diff/20001/extensions/common/exten...
extensions/common/extension_messages.cc:63:
extensions::PermissionsData::TabPermissionsMap tab_permissions =
On 2015/04/23 23:45:41, Jeffrey Yasskin wrote:
> This is a whole std::map, so copying it could be pretty expensive. Can you
have
> the getter do the iteration?
> 
> Otherwise, call it CopyTabSpecific...() to be clear there's a pretty high
cost.

That was why I changed the name. ;)  By "can you have the getter do the
iteration", are you suggesting having something like a
ForEachTabPermission(const base::Closure& closure);
method on PermissionsData?  If so, I suppose that could work, since this is the
only place we use it.

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

https://codereview.chromium.org/1057753003/diff/20001/extensions/common/permi...
extensions/common/permissions/permissions_data.h:205: TabPermissionsMap
GetTabSpecificPermissionsMap() const;
On 2015/04/23 23:45:41, Jeffrey Yasskin wrote:
> I'm nervous about returning a std::map by value.

Returning by reference is also Bad, because it's a map with refptrs - refptrs
which can be invalidated by setting tab permissions.  Is there a better way,
that's still safe, other than copying?

[Jeffrey Yasskin, 2015-04-24 00:03:56]

https://codereview.chromium.org/1057753003/diff/20001/extensions/common/exten...
File extensions/common/extension_messages.cc (right):

https://codereview.chromium.org/1057753003/diff/20001/extensions/common/exten...
extensions/common/extension_messages.cc:63:
extensions::PermissionsData::TabPermissionsMap tab_permissions =
On 2015/04/23 23:54:43, Devlin wrote:
> On 2015/04/23 23:45:41, Jeffrey Yasskin wrote:
> > This is a whole std::map, so copying it could be pretty expensive. Can you
> have
> > the getter do the iteration?
> > 
> > Otherwise, call it CopyTabSpecific...() to be clear there's a pretty high
> cost.
> 
> That was why I changed the name. ;)  By "can you have the getter do the
> iteration", are you suggesting having something like a
> ForEachTabPermission(const base::Closure& closure);
> method on PermissionsData?  If so, I suppose that could work, since this is
the
> only place we use it.

Calling a closure under a lock isn't great either, since you can't tell whether
the closure acquires another lock. You could have the PermissionsData do the
whole conversion (since this is the only use of GetTabSpecificPermissionsMap).
I'd be all right calling it "Copy..." too. Get... doesn't look expensive enough.

[Devlin, 2015-04-24 00:17:10]

https://codereview.chromium.org/1057753003/diff/20001/extensions/common/exten...
File extensions/common/extension_messages.cc (right):

https://codereview.chromium.org/1057753003/diff/20001/extensions/common/exten...
extensions/common/extension_messages.cc:63:
extensions::PermissionsData::TabPermissionsMap tab_permissions =
On 2015/04/24 00:03:55, Jeffrey Yasskin wrote:
> On 2015/04/23 23:54:43, Devlin wrote:
> > On 2015/04/23 23:45:41, Jeffrey Yasskin wrote:
> > > This is a whole std::map, so copying it could be pretty expensive. Can you
> > have
> > > the getter do the iteration?
> > > 
> > > Otherwise, call it CopyTabSpecific...() to be clear there's a pretty high
> > cost.
> > 
> > That was why I changed the name. ;)  By "can you have the getter do the
> > iteration", are you suggesting having something like a
> > ForEachTabPermission(const base::Closure& closure);
> > method on PermissionsData?  If so, I suppose that could work, since this is
> the
> > only place we use it.
> 
> Calling a closure under a lock isn't great either, since you can't tell
whether
> the closure acquires another lock. You could have the PermissionsData do the
> whole conversion (since this is the only use of GetTabSpecificPermissionsMap).
> I'd be all right calling it "Copy..." too. Get... doesn't look expensive
enough.

Let's go with Copy, then.  I don't think PermissionsData should be doing
message-specific stuff.

Done.

[Jeffrey Yasskin, 2015-04-24 00:24:16]

lgtm

[Devlin, 2015-04-24 00:26:09]

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

[commit-bot: I haz the power, 2015-04-24 00:26:42]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1057753003/40001

[commit-bot: I haz the power, 2015-04-24 01:17:19]

Committed patchset #2 (id:40001)

[commit-bot: I haz the power, 2015-04-24 01:18:15]

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