Create thread-safe RendererExtensionRegistry from ExtensionSet

Create thread-safe RendererExtensionRegistry from ExtensionSet

The worker thread needs to be able to access the set of current extensions in order to associate a v8::context with an extension.

BUG=501569
Committed: https://crrev.com/6572d5c749800cc81808e7628e11202fd40fc4f2
Cr-Commit-Position: refs/heads/master@{#344244}

[annekao, 2015-08-13 22:25:51]

Patchset #1 (id:1) has been deleted

[annekao, 2015-08-13 22:28:43]

annekao@google.com changed reviewers:
+ rdevlin.cronin@chromium.org

[not at google - send to devlin, 2015-08-13 23:33:55]

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

[not at google - send to devlin, 2015-08-13 23:33:56]

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

https://codereview.chromium.org/1293673002/diff/20001/extensions/common/exten...
extensions/common/extension_set.cc:47: base::AutoLock auto_lock(runtime_lock_);
I think we'll need the lock over every method, not just the mutation ones. E.g.
I don't know how C++ will behave in Contains if the set changes between the
find() and the end() call.

https://codereview.chromium.org/1293673002/diff/20001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1293673002/diff/20001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:123: LAZY_INSTANCE_INITIALIZER;
If we're doing this, ExtensionSet should handle its own singleton-ness.

So have a static method Get() on ExtensionSet, which routes to its own
g_instance method, etc.

(then entirely remove the fact that Dispatcher owns ExtensionSet).

[not at google - send to devlin, 2015-08-13 23:34:31]

https://codereview.chromium.org/1293673002/diff/20001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1293673002/diff/20001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:123: LAZY_INSTANCE_INITIALIZER;
> g_instance method

I mean g_instance variable.

[annekao, 2015-08-13 23:40:26]

On 2015/08/13 23:33:56, kalman wrote:
>
https://codereview.chromium.org/1293673002/diff/20001/extensions/renderer/dis...
> extensions/renderer/dispatcher.cc:123: LAZY_INSTANCE_INITIALIZER;
> If we're doing this, ExtensionSet should handle its own singleton-ness.
> 
> So have a static method Get() on ExtensionSet, which routes to its own
> g_instance method, etc.
> 
> (then entirely remove the fact that Dispatcher owns ExtensionSet).

That's what I had originally, but I was under the impression that since other
classes have ExtensionSets, they may not necessarily be the same as the
ExtensionSet in Dispatcher.  For example, ActiveTabPermissionGranter has an
ExtensionSet that only has extensions with the activeTab permission.

Would it be OK for only the Dispatcher to use the static Get() method?

[not at google - send to devlin, 2015-08-14 16:22:25]

On 2015/08/13 23:40:26, annekao wrote:
> On 2015/08/13 23:33:56, kalman wrote:
> >
>
https://codereview.chromium.org/1293673002/diff/20001/extensions/renderer/dis...
> > extensions/renderer/dispatcher.cc:123: LAZY_INSTANCE_INITIALIZER;
> > If we're doing this, ExtensionSet should handle its own singleton-ness.
> > 
> > So have a static method Get() on ExtensionSet, which routes to its own
> > g_instance method, etc.
> > 
> > (then entirely remove the fact that Dispatcher owns ExtensionSet).
> 
> That's what I had originally, but I was under the impression that since other
> classes have ExtensionSets, they may not necessarily be the same as the
> ExtensionSet in Dispatcher.  For example, ActiveTabPermissionGranter has an
> ExtensionSet that only has extensions with the activeTab permission.
> 
> Would it be OK for only the Dispatcher to use the static Get() method?

Oh right, ExtensionSet is in common. Yeah I'd pull it into a different file. I
have a long-standing TODO to have a single point of extension management on the
renderer - perhaps add a RendererExtensionRegistry class which has an
extensions() method, you would use it like:

const ExtensionSet& extensions = RendererExtensionRegistry::Get()->extensions();

similar to what you have, but decoupled from Dispatcher.

[Devlin, 2015-08-14 16:32:01]

On 2015/08/14 16:22:25, kalman wrote:
> On 2015/08/13 23:40:26, annekao wrote:
> > On 2015/08/13 23:33:56, kalman wrote:
> > >
> >
>
https://codereview.chromium.org/1293673002/diff/20001/extensions/renderer/dis...
> > > extensions/renderer/dispatcher.cc:123: LAZY_INSTANCE_INITIALIZER;
> > > If we're doing this, ExtensionSet should handle its own singleton-ness.
> > > 
> > > So have a static method Get() on ExtensionSet, which routes to its own
> > > g_instance method, etc.
> > > 
> > > (then entirely remove the fact that Dispatcher owns ExtensionSet).
> > 
> > That's what I had originally, but I was under the impression that since
other
> > classes have ExtensionSets, they may not necessarily be the same as the
> > ExtensionSet in Dispatcher.  For example, ActiveTabPermissionGranter has an
> > ExtensionSet that only has extensions with the activeTab permission.
> > 
> > Would it be OK for only the Dispatcher to use the static Get() method?
> 
> Oh right, ExtensionSet is in common. Yeah I'd pull it into a different file. I
> have a long-standing TODO to have a single point of extension management on
the
> renderer - perhaps add a RendererExtensionRegistry class which has an
> extensions() method, you would use it like:
> 
> const ExtensionSet& extensions =
RendererExtensionRegistry::Get()->extensions();
> 
> similar to what you have, but decoupled from Dispatcher.

Jumping on this a little late; sorry.  Making the change to the entire
ExtensionSet class strikes me as a little bit of overkill, and might have some
unforeseen/undesired side-effects.  What if we instead just have a renderer
class that has it's lock?  E.g.,

class RendererExtensionRegistry {
  base::Lock lock_;
  ExtensionSet extensions_;
};

g_lazy_renderer_registry = LAZY_INSTANCE;

This way, we don't make all the other extension sets have the unnecessary locks.
(Acquiring the lock isn't *that* expensive when you don't have to wait for it,
but it's still more than a no-op, since it has to go through all that
platform-specific mutex stuff.  Also, this change alone wouldn't make any other
extension set safe, since everything that owns them [most commonly the
ExtensionRegistry] isn't thread safe - and locks don't prevent things like an
ExtensionSet being deleted on the browser thread and accessed on the file
thread, unless you want to have a much bigger change to make the full registry
thread safe.)

[not at google - send to devlin, 2015-08-14 16:37:25]

On 2015/08/14 16:32:01, Devlin wrote:
> On 2015/08/14 16:22:25, kalman wrote:
> > On 2015/08/13 23:40:26, annekao wrote:
> > > On 2015/08/13 23:33:56, kalman wrote:
> > > >
> > >
> >
>
https://codereview.chromium.org/1293673002/diff/20001/extensions/renderer/dis...
> > > > extensions/renderer/dispatcher.cc:123: LAZY_INSTANCE_INITIALIZER;
> > > > If we're doing this, ExtensionSet should handle its own singleton-ness.
> > > > 
> > > > So have a static method Get() on ExtensionSet, which routes to its own
> > > > g_instance method, etc.
> > > > 
> > > > (then entirely remove the fact that Dispatcher owns ExtensionSet).
> > > 
> > > That's what I had originally, but I was under the impression that since
> other
> > > classes have ExtensionSets, they may not necessarily be the same as the
> > > ExtensionSet in Dispatcher.  For example, ActiveTabPermissionGranter has
an
> > > ExtensionSet that only has extensions with the activeTab permission.
> > > 
> > > Would it be OK for only the Dispatcher to use the static Get() method?
> > 
> > Oh right, ExtensionSet is in common. Yeah I'd pull it into a different file.
I
> > have a long-standing TODO to have a single point of extension management on
> the
> > renderer - perhaps add a RendererExtensionRegistry class which has an
> > extensions() method, you would use it like:
> > 
> > const ExtensionSet& extensions =
> RendererExtensionRegistry::Get()->extensions();
> > 
> > similar to what you have, but decoupled from Dispatcher.
> 
> Jumping on this a little late; sorry.  Making the change to the entire
> ExtensionSet class strikes me as a little bit of overkill, and might have some
> unforeseen/undesired side-effects.  What if we instead just have a renderer
> class that has it's lock?  E.g.,
> 
> class RendererExtensionRegistry {
>   base::Lock lock_;
>   ExtensionSet extensions_;
> };
> 
> g_lazy_renderer_registry = LAZY_INSTANCE;
> 
> This way, we don't make all the other extension sets have the unnecessary
locks.
> (Acquiring the lock isn't *that* expensive when you don't have to wait for it,
> but it's still more than a no-op, since it has to go through all that
> platform-specific mutex stuff.  Also, this change alone wouldn't make any
other
> extension set safe, since everything that owns them [most commonly the
> ExtensionRegistry] isn't thread safe - and locks don't prevent things like an
> ExtensionSet being deleted on the browser thread and accessed on the file
> thread, unless you want to have a much bigger change to make the full registry
> thread safe.)

Yeah it's a shame to need to make the entire of the extension set thread safe.
That's why we have ExtensionInfoMap on the browser, which internally maintains a
copy of its own extensionset for the IO thread. Unfortunately this wouldn't work
for us because we need to deal with an arbitrary number of threads.

longer term I'd go with that RendererExtensionRegistry class which *is*
thread-safe and which has a conversion function which "copies" it into a new
ExtensionSet. That's by-default thread safe, and inefficient, but most code
should be able to use the RendererExtensionRegistry class anyway.

this is a pretty full-on change.

[Devlin, 2015-08-14 16:45:11]

On 2015/08/14 16:37:25, kalman wrote:
> Yeah it's a shame to need to make the entire of the extension set thread safe.
> That's why we have ExtensionInfoMap on the browser, which internally maintains
a
> copy of its own extensionset for the IO thread. Unfortunately this wouldn't
work
> for us because we need to deal with an arbitrary number of threads.
Right, wouldn't think of duplicating that weirdness on the renderer.

> longer term I'd go with that RendererExtensionRegistry class which *is*
> thread-safe and which has a conversion function which "copies" it into a new
> ExtensionSet. That's by-default thread safe, and inefficient, but most code
> should be able to use the RendererExtensionRegistry class anyway.
> this is a pretty full-on change.
I think this is actually pretty easy.  Short-term fix is just to make a lazy
instance (which is by default thread safe) that has a lock and an extension set,
and a few forwarding methods to the extension set that acquire the lock so that
it handles thread safety.  Isn't this all we need?

Long-term, I could see making that change on ExtensionSet itself, and then
having a single instance of (each enabled|disabled|etc) the ExtensionSet on the
browser side - but that's a bigger fix, and one I don't want this gated on.

[not at google - send to devlin, 2015-08-14 17:18:01]

> I think this is actually pretty easy.  Short-term fix is just to make a lazy
> instance (which is by default thread safe) that has a lock and an extension
set,
> and a few forwarding methods to the extension set that acquire the lock so
that
> it handles thread safety.  Isn't this all we need?

I haven't looked at the call sites, but I expect that requiring everything on
the renderer to take a lock out is a churn-y change. It's of course not
sustainable.

Another solution: make a ThreadSafeExtensionSet class which mimics the
ExtensionSet interface, delegates everything to an internal ExtensionSet
instance, with a method to copy its internal structure for the times when it
needs to interface with anything that can't be easily updated to use a
ThreadSafeExtensionSet.

I don't think you need to create any kind of LazyInstance with this?

[not at google - send to devlin, 2015-08-14 17:35:27]

I had a thought: I do think we should have some kind of "thread safe bundle"
concept on the renderer; a singleton object that is safe to access across
threads, and only has objects that are thread safe (like this
ThreadSafeExtensionSet). Dispatcher is not thread safe. The WakeEventPage native
handler I've been working on will need that sort of thing.

[Devlin, 2015-08-14 18:21:04]

On 2015/08/14 17:18:01, kalman wrote:
> > I think this is actually pretty easy.  Short-term fix is just to make a lazy
> > instance (which is by default thread safe) that has a lock and an extension
> set,
> > and a few forwarding methods to the extension set that acquire the lock so
> that
> > it handles thread safety.  Isn't this all we need?
> 
> I haven't looked at the call sites, but I expect that requiring everything on
> the renderer to take a lock out is a churn-y change. It's of course not
> sustainable.

That's not what I was suggesting - I was saying make a thread-safe class which
contains an extension set, and forwards the calls to it.  More verbosely:
class RendererExtensionRegistry {
 public:
  Contains();
  Insert();
  ...
 private:
  ExtensionSet extensions_;
  base::Lock lock_;
};

RendererExtensionSet::Contains() {
  AutoLock lock(lock_);
  return extensions_.Contains();
}

RendererExtensionSet::Insert() {
  AutoLock lock(lock_);
  return extensions_.Insert();
}

I tried to describe this above (even had the class structure), but maybe it
wasn't clear enough.

> 
> Another solution: make a ThreadSafeExtensionSet class which mimics the
> ExtensionSet interface, delegates everything to an internal ExtensionSet
> instance, with a method to copy its internal structure for the times when it
> needs to interface with anything that can't be easily updated to use a
> ThreadSafeExtensionSet.

That's a lot more like what I was saying.  I'm not sure of the times when we
really need to copy the set - do you know if there are any?

> I don't think you need to create any kind of LazyInstance with this?

Depends on what you want the lifetime to be like.  Even if the
ThreadSafeExtensionSet/RendererExtensionRegistry is threadsafe, whatever it's
owned by (the dispatcher?) won't be.  Of course, most things like the dispatcher
basically survive for the life of the process, so maybe not a big deal... I'd
probably still prefer a LazyInstance, though, so that it's not coupled to the
dispatcher or any other thread-specific object.

[Devlin, 2015-08-14 18:22:41]

On 2015/08/14 17:35:27, kalman wrote:
> I had a thought: I do think we should have some kind of "thread safe bundle"
> concept on the renderer; a singleton object that is safe to access across
> threads, and only has objects that are thread safe (like this
> ThreadSafeExtensionSet). Dispatcher is not thread safe. The WakeEventPage
native
> handler I've been working on will need that sort of thing.

That could work, though I question how much we'd need it for.  For instance,
does the WakeEventPage native handler really need to be thread safe?  Or can it
instead be a collection of static methods that can access thread-safe objects
(like the extension set)?  This is probably best talked about in person, at this
point...

[annekao, 2015-08-14 23:41:07]

On 2015/08/14 18:21:04, Devlin wrote:
> On 2015/08/14 17:18:01, kalman wrote:
> > > I think this is actually pretty easy.  Short-term fix is just to make a
lazy
> > > instance (which is by default thread safe) that has a lock and an
extension
> > set,
> > > and a few forwarding methods to the extension set that acquire the lock so
> > that
> > > it handles thread safety.  Isn't this all we need?
> > 
> > I haven't looked at the call sites, but I expect that requiring everything
on
> > the renderer to take a lock out is a churn-y change. It's of course not
> > sustainable.
> 
> That's not what I was suggesting - I was saying make a thread-safe class which
> contains an extension set, and forwards the calls to it.  More verbosely:
> class RendererExtensionRegistry {
>  public:
>   Contains();
>   Insert();
>   ...
>  private:
>   ExtensionSet extensions_;
>   base::Lock lock_;
> };
> 
> RendererExtensionSet::Contains() {
>   AutoLock lock(lock_);
>   return extensions_.Contains();
> }
> 
> RendererExtensionSet::Insert() {
>   AutoLock lock(lock_);
>   return extensions_.Insert();
> }
> 
> I tried to describe this above (even had the class structure), but maybe it
> wasn't clear enough.
> 
> > 
> > Another solution: make a ThreadSafeExtensionSet class which mimics the
> > ExtensionSet interface, delegates everything to an internal ExtensionSet
> > instance, with a method to copy its internal structure for the times when it
> > needs to interface with anything that can't be easily updated to use a
> > ThreadSafeExtensionSet.
> 
> That's a lot more like what I was saying.  I'm not sure of the times when we
> really need to copy the set - do you know if there are any?
> 
> > I don't think you need to create any kind of LazyInstance with this?
> 
> Depends on what you want the lifetime to be like.  Even if the
> ThreadSafeExtensionSet/RendererExtensionRegistry is threadsafe, whatever it's
> owned by (the dispatcher?) won't be.  Of course, most things like the
dispatcher
> basically survive for the life of the process, so maybe not a big deal... I'd
> probably still prefer a LazyInstance, though, so that it's not coupled to the
> dispatcher or any other thread-specific object.

Should the RendererExtensionSet have a function to return its' ExtensionSet?
I ask because in the Dispatcher, there are some classes that are initialized
that take in an ExtensionSet (ScriptContextSet and UserScriptSetManager).

[Devlin, 2015-08-15 00:06:57]

On 2015/08/14 23:41:07, annekao wrote:
> Should the RendererExtensionSet have a function to return its' ExtensionSet?
> I ask because in the Dispatcher, there are some classes that are initialized
> that take in an ExtensionSet (ScriptContextSet and UserScriptSetManager).

Short answer: maybe, but only as an explicit "GetMainThreadExtensionSet" method.

Where possible, I think Ben and I came to the conclusion that this should be
avoided, and just forwarded to the underlying extension set.  If overly
difficult/churny (i.e., if it's not something like
dispatcher->extensions()->SomethingElse() where the SomethingElse can be
directly forwarded), they can use that method.  Otherwise, probably just forward
it.  Does that make sense?

[annekao, 2015-08-17 19:59:37]

Patchset #2 (id:40001) has been deleted

[annekao, 2015-08-17 20:02:32]

Patchset #2 (id:60001) has been deleted

[annekao, 2015-08-17 20:05:02]

Added RendererExtensionRegistry.  Also changed any files that used
dispatcher->extensions() to use GetRegistry().

[not at google - send to devlin, 2015-08-17 20:43:05]

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:206:
RendererExtensionRegistry::GetRegistry()->GetMainThreadExtensionSet(),
It's dangerous to be sending in a thread-unsafe version of the extension set
into these methods. Better to let
ScriptContextSet/UserScriptSetManager/ScriptInjectionManager to grab the
RendererExtensionRegistry themselves, as needed, then decide whether to use the
main thread version or not (hopefully not).

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:710:
CHECK(RendererExtensionRegistry::GetRegistry()->Insert(extension));
You could even just delete LoadExtensionForTest and make
renderer_permissions_policy_delegate_unittest.cc - the only place this is called
- use RendererExtensionRegistry::GetRegistry()->Insert(...) itself.

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:899: if
(!RendererExtensionRegistry::GetRegistry()->Contains(extension->id()))
When you use RendererExtensionRegistry multiple times like this, it's nice to
save a reference to it.

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
File extensions/renderer/renderer_extension_registry.cc (right):

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
extensions/renderer/renderer_extension_registry.cc:7: #include "base/callback.h"
(delete this)

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
extensions/renderer/renderer_extension_registry.cc:17:
g_renderer_extension_registry = LAZY_INSTANCE_INITIALIZER;
Put this in an anonymous namespace, not static.

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
File extensions/renderer/renderer_extension_registry.h (right):

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
extensions/renderer/renderer_extension_registry.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
2015, and in the .cc file.

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
extensions/renderer/renderer_extension_registry.h:9: #include <map>
you can remove this <map> include.

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
extensions/renderer/renderer_extension_registry.h:12: #include "base/callback.h"
and once you delete that modification method, you can delete this include as
well.

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
extensions/renderer/renderer_extension_registry.h:24: // Extensions are
identified by their id.
Let's trim down this comment as well, it's unnecessarily wordy (and you usually
don't need to include details about a class' implementation details in its class
comment):

// Thread safe container for all loaded extensions in this process,
// essentially the renderer counterpart to ExtensionRegistry.

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
extensions/renderer/renderer_extension_registry.h:31: static
RendererExtensionRegistry* GetRegistry();
Typically singleton accessors are just "Get" not "GetRegistry"... no need to
repeat "registry" when calling RendererExtensionRegistry::GetRegistry.

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
extensions/renderer/renderer_extension_registry.h:114:
ExtensionSet::ModificationCallback modification_callback_;
Let's trim this interface a bit. Can we get rid of the modification callback at
least?

[annekao, 2015-08-17 23:33:18]

Patchset #3 (id:100001) has been deleted

[annekao, 2015-08-17 23:44:00]

Patchset #3 (id:120001) has been deleted

[annekao, 2015-08-17 23:48:53]

Patchset #3 (id:140001) has been deleted

[annekao, 2015-08-17 23:51:11]

Had to take out ExtensionSet from the constructor of a lot of classes.  Also
removed any ExtensionSet variables.

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/dis...
File extensions/renderer/dispatcher.cc (right):

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:206:
RendererExtensionRegistry::GetRegistry()->GetMainThreadExtensionSet(),
On 2015/08/17 20:43:05, kalman wrote:
> It's dangerous to be sending in a thread-unsafe version of the extension set
> into these methods. Better to let
> ScriptContextSet/UserScriptSetManager/ScriptInjectionManager to grab the
> RendererExtensionRegistry themselves, as needed, then decide whether to use
the
> main thread version or not (hopefully not).

Done.  The main thread version has to be passed into
GetNonBookMarkAppExtension(), CrossesExtensionProcessBoundary(),
IsExtensionOrSharedModuleWhitelisted() since they are in common.

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:710:
CHECK(RendererExtensionRegistry::GetRegistry()->Insert(extension));
On 2015/08/17 20:43:05, kalman wrote:
> You could even just delete LoadExtensionForTest and make
> renderer_permissions_policy_delegate_unittest.cc - the only place this is
called
> - use RendererExtensionRegistry::GetRegistry()->Insert(...) itself.

Done.

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/dis...
extensions/renderer/dispatcher.cc:899: if
(!RendererExtensionRegistry::GetRegistry()->Contains(extension->id()))
On 2015/08/17 20:43:05, kalman wrote:
> When you use RendererExtensionRegistry multiple times like this, it's nice to
> save a reference to it.

Done.

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
File extensions/renderer/renderer_extension_registry.cc (right):

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
extensions/renderer/renderer_extension_registry.cc:7: #include "base/callback.h"
On 2015/08/17 20:43:05, kalman wrote:
> (delete this)

Done.

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
extensions/renderer/renderer_extension_registry.cc:17:
g_renderer_extension_registry = LAZY_INSTANCE_INITIALIZER;
On 2015/08/17 20:43:05, kalman wrote:
> Put this in an anonymous namespace, not static.

Done.

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
File extensions/renderer/renderer_extension_registry.h (right):

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
extensions/renderer/renderer_extension_registry.h:1: // Copyright 2013 The
Chromium Authors. All rights reserved.
On 2015/08/17 20:43:05, kalman wrote:
> 2015, and in the .cc file.

Done.

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
extensions/renderer/renderer_extension_registry.h:9: #include <map>
On 2015/08/17 20:43:05, kalman wrote:
> you can remove this <map> include.

Done.

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
extensions/renderer/renderer_extension_registry.h:12: #include "base/callback.h"
On 2015/08/17 20:43:05, kalman wrote:
> and once you delete that modification method, you can delete this include as
> well.

Done.

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
extensions/renderer/renderer_extension_registry.h:24: // Extensions are
identified by their id.
On 2015/08/17 20:43:05, kalman wrote:
> Let's trim down this comment as well, it's unnecessarily wordy (and you
usually
> don't need to include details about a class' implementation details in its
class
> comment):
> 
> // Thread safe container for all loaded extensions in this process,
> // essentially the renderer counterpart to ExtensionRegistry.

Done.

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
extensions/renderer/renderer_extension_registry.h:31: static
RendererExtensionRegistry* GetRegistry();
On 2015/08/17 20:43:05, kalman wrote:
> Typically singleton accessors are just "Get" not "GetRegistry"... no need to
> repeat "registry" when calling RendererExtensionRegistry::GetRegistry.

Done.

https://codereview.chromium.org/1293673002/diff/80001/extensions/renderer/ren...
extensions/renderer/renderer_extension_registry.h:114:
ExtensionSet::ModificationCallback modification_callback_;
On 2015/08/17 20:43:05, kalman wrote:
> Let's trim this interface a bit. Can we get rid of the modification callback
at
> least?

Done. Took out functions that weren't used in any of the renderer files.

[Devlin, 2015-08-18 15:40:55]

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/re...
File extensions/renderer/renderer_extension_registry.cc (right):

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.cc:8: #include "base/stl_util.h"
?

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.cc:9: #include
"extensions/common/constants.h"
?

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.cc:10: #include
"extensions/common/extension.h"
might not even need this one...

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.cc:11: #include
"extensions/common/manifest_handlers/sandboxed_page_info.h"
?

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/re...
File extensions/renderer/renderer_extension_registry.h (right):

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.h:27: const ExtensionSet*
GetMainThreadExtensionSet() const;
This should definitely have a comment, and probably a TODO to remove it (I think
it's mostly just a hack that we need it).

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.h:33:
ExtensionSet::const_iterator begin() const { return extensions_.begin(); }
This is thread-unsafe.  If the main thread acquires an iterator, and another
thread inserts an extension, and then the main thread uses the iterator, bad
stuff happens.  I think if something *really* needs an iterator (it should be
rare, I think), it should just grab the full ExtensionSet.

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.h:37: bool Contains(const
std::string& id) const;
I think for these you can (and probably should) just say "Forwards to the
ExtensionSet methods by the same name."  That way, we don't have to update the
comments if/when those change.

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/sc...
File extensions/renderer/script_injection_manager.h (right):

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/sc...
extensions/renderer/script_injection_manager.h:35:
ScriptInjectionManager(UserScriptSetManager* user_script_set_manager);
explicit

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/us...
File extensions/renderer/user_script_set.h (right):

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/us...
extensions/renderer/user_script_set.h:41: explicit UserScriptSet();
not explicit

[annekao, 2015-08-18 17:43:31]

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/re...
File extensions/renderer/renderer_extension_registry.cc (right):

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.cc:8: #include "base/stl_util.h"
On 2015/08/18 15:40:55, Devlin wrote:
> ?

Done.

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/re...
File extensions/renderer/renderer_extension_registry.h (right):

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.h:27: const ExtensionSet*
GetMainThreadExtensionSet() const;
On 2015/08/18 15:40:55, Devlin wrote:
> This should definitely have a comment, and probably a TODO to remove it (I
think
> it's mostly just a hack that we need it).

Done.

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.h:33:
ExtensionSet::const_iterator begin() const { return extensions_.begin(); }
On 2015/08/18 15:40:55, Devlin wrote:
> This is thread-unsafe.  If the main thread acquires an iterator, and another
> thread inserts an extension, and then the main thread uses the iterator, bad
> stuff happens.  I think if something *really* needs an iterator (it should be
> rare, I think), it should just grab the full ExtensionSet.

Done.  Used in IsRuntimeAvailableToConnect() and UpdateContentCapabilities() in
Dispatcher.

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.h:37: bool Contains(const
std::string& id) const;
On 2015/08/18 15:40:55, Devlin wrote:
> I think for these you can (and probably should) just say "Forwards to the
> ExtensionSet methods by the same name."  That way, we don't have to update the
> comments if/when those change.

Done.

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/sc...
File extensions/renderer/script_injection_manager.h (right):

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/sc...
extensions/renderer/script_injection_manager.h:35:
ScriptInjectionManager(UserScriptSetManager* user_script_set_manager);
On 2015/08/18 15:40:55, Devlin wrote:
> explicit

Done.

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/us...
File extensions/renderer/user_script_set.h (right):

https://codereview.chromium.org/1293673002/diff/160001/extensions/renderer/us...
extensions/renderer/user_script_set.h:41: explicit UserScriptSet();
On 2015/08/18 15:40:55, Devlin wrote:
> not explicit

Done.

[not at google - send to devlin, 2015-08-18 18:42:50]

lgtm for my perspective. Wait for Devlin's additional lg as well, of course.

https://codereview.chromium.org/1293673002/diff/180001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1293673002/diff/180001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1243:
*(extensions->GetMainThreadExtensionSet()), url);
*extensions->GetMainThreadExtensionSet() is fine, no need for the extra
parentheses.

https://codereview.chromium.org/1293673002/diff/180001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1418:
*(extensions->GetMainThreadExtensionSet()), old_url, new_url,
Ditto.

https://codereview.chromium.org/1293673002/diff/180001/extensions/renderer/re...
File extensions/renderer/renderer_extension_registry.cc (right):

https://codereview.chromium.org/1293673002/diff/180001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.cc:16: }
Standard style is to have a blank line here, and enclose the end of a namespace
with a comment, so it would look like:

namespace {

base::LazyInstance<RendererExtensionRegistry> ...;

}  // namespace

https://codereview.chromium.org/1293673002/diff/180001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.cc:28: const {
DCHECK(content::RenderThread::Get());

In fact, I'd put this on all of the non-const methods for extra paranoia, then
document that RendererExtensionRegistry can only be modified on the
RenderThread, with a class comment like:

// This can only be modified on the RenderThread, because
// GetMainThreadExtensionSet is inherently thread unsafe.
// Enforcing single-thread modification at least mitigates this.
// TODO(annekao): Remove this restriction once GetMainThreadExtensionSet is
fixed.

https://codereview.chromium.org/1293673002/diff/180001/extensions/renderer/re...
File extensions/renderer/renderer_extension_registry.h (right):

https://codereview.chromium.org/1293673002/diff/180001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.h:29: // TODO: Remove.
I'd reword this:

// Returns the ExtensionSet that underlies this RenderExtensionRegistry.
//
// This is not thread-safe and must only be called on the RenderThread, but
// even so, it's not thread safe because other threads may decide to
// modify this. Don't persist a reference to this.
// 
// TODO(annekao): remove or fix.

https://codereview.chromium.org/1293673002/diff/180001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.h:30: const ExtensionSet*
GetMainThreadExtensionSet() const;
All my comments relating to thread safety aside, I think we can actually make
this thread safe, by giving access to the underlying ExtensionSet in a callback.
The interface would look like:

void GetMainThreadExtensionSet(
    const base::Callback<void(const ExtensionSet&)>& callback);

implemented like:

void RendererExtensionRegistry::GetMainThreadExtensionSet(callback) {
  base::AutoLock(lock_);
  callback.Run(extensions_);
}

and then call sites would be changed from things like:

bool MyFunction() {
  return OtherFunction(
      *RendererExtensionRegistry::Get()->
      GetMainThreadExtensionSet());
}

to:

void RunOtherFunction(
    bool* result, const ExtensionSet& set) {
  *result = OtherFunction(set);
}

bool MyFunction() {
  bool result;
  RendererExtensionRegistry::Get()->
     GetMainThreadExtensionSet(base::Bind(
         &RunOtherFunction, &result));
  return result;
}

quite a bit more wordy, but threadsafe.

For now I'd just make the less invasive change and go with your current
implementation of GetMainThreadExtensionSet, but I think that we should use a
threadsafe version eventually.

You might want to include in the comment, in your TODO, to make
GetMainThreadExtensionSet thead safe and callback-based.

[not at google - send to devlin, 2015-08-18 18:43:03]

... that is, lgtm once my comments have been addressed, but they're pretty
simple.

[annekao, 2015-08-18 20:21:13]

https://codereview.chromium.org/1293673002/diff/180001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1293673002/diff/180001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1243:
*(extensions->GetMainThreadExtensionSet()), url);
On 2015/08/18 18:42:49, kalman wrote:
> *extensions->GetMainThreadExtensionSet() is fine, no need for the extra
> parentheses.

Done.

https://codereview.chromium.org/1293673002/diff/180001/extensions/renderer/re...
File extensions/renderer/renderer_extension_registry.cc (right):

https://codereview.chromium.org/1293673002/diff/180001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.cc:16: }
On 2015/08/18 18:42:50, kalman wrote:
> Standard style is to have a blank line here, and enclose the end of a
namespace
> with a comment, so it would look like:
> 
> namespace {
> 
> base::LazyInstance<RendererExtensionRegistry> ...;
> 
> }  // namespace

Done.

https://codereview.chromium.org/1293673002/diff/180001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.cc:28: const {
On 2015/08/18 18:42:49, kalman wrote:
> DCHECK(content::RenderThread::Get());
> 
> In fact, I'd put this on all of the non-const methods for extra paranoia, then
> document that RendererExtensionRegistry can only be modified on the
> RenderThread, with a class comment like:
> 
> // This can only be modified on the RenderThread, because
> // GetMainThreadExtensionSet is inherently thread unsafe.
> // Enforcing single-thread modification at least mitigates this.
> // TODO(annekao): Remove this restriction once GetMainThreadExtensionSet is
> fixed.

Done.

https://codereview.chromium.org/1293673002/diff/180001/extensions/renderer/re...
File extensions/renderer/renderer_extension_registry.h (right):

https://codereview.chromium.org/1293673002/diff/180001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.h:29: // TODO: Remove.
On 2015/08/18 18:42:50, kalman wrote:
> I'd reword this:
> 
> // Returns the ExtensionSet that underlies this RenderExtensionRegistry.
> //
> // This is not thread-safe and must only be called on the RenderThread, but
> // even so, it's not thread safe because other threads may decide to
> // modify this. Don't persist a reference to this.
> // 
> // TODO(annekao): remove or fix.

Done.

[Devlin, 2015-08-18 20:26:40]

lgtm with nits.

https://codereview.chromium.org/1293673002/diff/200001/extensions/renderer/re...
File extensions/renderer/renderer_extension_registry.h (right):

https://codereview.chromium.org/1293673002/diff/200001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.h:8: #include <iterator>
not needed.

https://codereview.chromium.org/1293673002/diff/200001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.h:12: #include
"extensions/common/extension.h"
not needed.

https://codereview.chromium.org/1293673002/diff/200001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.h:14: #include "url/gurl.h"
Use a forward declaration.

https://codereview.chromium.org/1293673002/diff/200001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.h:41: 
nit: Take out the newlines here so that it's clear the comment applies to all
these functions; i.e.:
// These methods forward to the ExtensionSet methods by the same name.
bool Contains()...
bool Insert()...
...
bool ExtensionBindingsAllowed()...

private:

[annekao, 2015-08-18 20:40:49]

annekao@google.com changed reviewers:
+ thestig@chromium.org

[annekao, 2015-08-18 20:44:02]

thestig@, please take a look at the changes in chrome/renderer/

ExtensionSet is replaced with RendererExtensionRegistry which is a thread-safe
container that forwards its' method calls to ExtensionSet. 
RendererExtensionRegistry is used in the main thread and in worker threads.

https://codereview.chromium.org/1293673002/diff/200001/extensions/renderer/re...
File extensions/renderer/renderer_extension_registry.h (right):

https://codereview.chromium.org/1293673002/diff/200001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.h:8: #include <iterator>
On 2015/08/18 20:26:40, Devlin wrote:
> not needed.

Done.

https://codereview.chromium.org/1293673002/diff/200001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.h:12: #include
"extensions/common/extension.h"
On 2015/08/18 20:26:40, Devlin wrote:
> not needed.

Done.

https://codereview.chromium.org/1293673002/diff/200001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.h:14: #include "url/gurl.h"
On 2015/08/18 20:26:40, Devlin wrote:
> Use a forward declaration.

Done.

https://codereview.chromium.org/1293673002/diff/200001/extensions/renderer/re...
extensions/renderer/renderer_extension_registry.h:41: 
On 2015/08/18 20:26:40, Devlin wrote:
> nit: Take out the newlines here so that it's clear the comment applies to all
> these functions; i.e.:
> // These methods forward to the ExtensionSet methods by the same name.
> bool Contains()...
> bool Insert()...
> ...
> bool ExtensionBindingsAllowed()...
> 
> private:

Done.

[annekao, 2015-08-18 21:37:21]

Patchset #6 (id:220001) has been deleted

[Lei Zhang, 2015-08-18 21:51:37]

non-extensions bits lgtm

https://codereview.chromium.org/1293673002/diff/240001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1293673002/diff/240001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1238: const
extensions::RendererExtensionRegistry* extensions =
This variable probably needs a new name, since it is not the set of extensions
anymore.

https://codereview.chromium.org/1293673002/diff/240001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1376:
extensions::RendererExtensionRegistry* extensions =
Ditto

[annekao, 2015-08-18 22:23:53]

Patchset #7 (id:260001) has been deleted

[annekao, 2015-08-19 03:25:56]

Patchset #7 (id:280001) has been deleted

[annekao, 2015-08-19 16:08:20]

https://codereview.chromium.org/1293673002/diff/240001/chrome/renderer/chrome...
File chrome/renderer/chrome_content_renderer_client.cc (right):

https://codereview.chromium.org/1293673002/diff/240001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1238: const
extensions::RendererExtensionRegistry* extensions =
On 2015/08/18 21:51:37, Lei Zhang wrote:
> This variable probably needs a new name, since it is not the set of extensions
> anymore.

Done.

https://codereview.chromium.org/1293673002/diff/240001/chrome/renderer/chrome...
chrome/renderer/chrome_content_renderer_client.cc:1376:
extensions::RendererExtensionRegistry* extensions =
On 2015/08/18 21:51:37, Lei Zhang wrote:
> Ditto

Done.

[annekao, 2015-08-19 16:08:27]

The CQ bit was checked by annekao@google.com

[annekao, 2015-08-19 16:08:27]

The patchset sent to the CQ was uploaded after l-g-t-m from kalman@chromium.org,
rdevlin.cronin@chromium.org, thestig@chromium.org
Link to the patchset: https://codereview.chromium.org/1293673002/#ps300001
(title: " ")

[commit-bot: I haz the power, 2015-08-19 16:08:45]

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

[commit-bot: I haz the power, 2015-08-19 16:13:43]

Committed patchset #7 (id:300001)

[commit-bot: I haz the power, 2015-08-19 16:14:30]

Patchset 7 (id:??) landed as
https://crrev.com/6572d5c749800cc81808e7628e11202fd40fc4f2
Cr-Commit-Position: refs/heads/master@{#344244}