Lazily load extension API schemas

Lazily load extension API schemas

Now all the extensions API schemas are loaded lazily, to
avoid the memory overhead of loading them upfront. Also,
the API feature system now can use the URL to determine
availability of the API. APIs can now be checked for
availability separately, without needing to load all the APIs.


Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=190058

[not at google - send to devlin, 2013-03-14 19:16:46]

At a high level - cool. You should address that TODO that we talked about in
dispatcher.cc so that we can be sure it works from the perf bots.

https://codereview.chromium.org/12522004/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/extension_api.h (right):

https://codereview.chromium.org/12522004/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.h:80: bool UsesFeatureSystem(const
std::string& full_name);
looks like this can be moved back out of public:

https://codereview.chromium.org/12522004/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.h:95: bool
IsNonFeatureAvailable(const std::string& name,
First file I'm reading: this name confuses me.

Also can be moved out of public:

https://codereview.chromium.org/12522004/diff/1/chrome/common/extensions/feat...
File chrome/common/extensions/features/simple_feature.cc (right):

https://codereview.chromium.org/12522004/diff/1/chrome/common/extensions/feat...
chrome/common/extensions/features/simple_feature.cc:318: return
base::StringPrintf("'%s' is not allowed for this url.",
not allowed on %s?

https://codereview.chromium.org/12522004/diff/1/chrome/renderer/extensions/ch...
File chrome/renderer/extensions/chrome_v8_context.cc (right):

https://codereview.chromium.org/12522004/diff/1/chrome/renderer/extensions/ch...
chrome/renderer/extensions/chrome_v8_context.cc:154: // TODO(cduvall): Change
this because Stubs probably fails.
you have permission to change stubs if reasonable :)

[cduvall, 2013-03-21 22:27:55]

Waiting for https://codereview.chromium.org/12647017/ to go in. I ran all the
Extension* tests with that patched in, everything seems to be good.

The only thing I'm worried about is the comment in extension_api.cc

https://codereview.chromium.org/12522004/diff/1/chrome/common/extensions/api/...
File chrome/common/extensions/api/extension_api.h (right):

https://codereview.chromium.org/12522004/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.h:80: bool UsesFeatureSystem(const
std::string& full_name);
On 2013/03/14 19:16:46, kalman wrote:
> looks like this can be moved back out of public:

Done.

https://codereview.chromium.org/12522004/diff/1/chrome/common/extensions/api/...
chrome/common/extensions/api/extension_api.h:95: bool
IsNonFeatureAvailable(const std::string& name,
On 2013/03/14 19:16:46, kalman wrote:
> First file I'm reading: this name confuses me.
> 
> Also can be moved out of public:

renamed to IsNonFeatureAPIAvailable()

https://codereview.chromium.org/12522004/diff/1/chrome/common/extensions/feat...
File chrome/common/extensions/features/simple_feature.cc (right):

https://codereview.chromium.org/12522004/diff/1/chrome/common/extensions/feat...
chrome/common/extensions/features/simple_feature.cc:318: return
base::StringPrintf("'%s' is not allowed for this url.",
On 2013/03/14 19:16:46, kalman wrote:
> not allowed on %s?

This function doesn't have access to the url. Should I plumb it through?

https://codereview.chromium.org/12522004/diff/1/chrome/renderer/extensions/ch...
File chrome/renderer/extensions/chrome_v8_context.cc (right):

https://codereview.chromium.org/12522004/diff/1/chrome/renderer/extensions/ch...
chrome/renderer/extensions/chrome_v8_context.cc:154: // TODO(cduvall): Change
this because Stubs probably fails.
On 2013/03/14 19:16:46, kalman wrote:
> you have permission to change stubs if reasonable :)

Ended up just having to change getApiDefinitions in chrome.test

[not at google - send to devlin, 2013-03-22 18:13:48]

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
File chrome/common/extensions/api/extension_api.cc (right):

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:427:
ResolveDependencies(&dependency_names);
as I mentioned in IRC - I think we can entirely get rid of the dependencies now
that we load types properly - dependency_names, ResolveDependencies, that
spurious looking for loop below (I guess you'd need to just collapse it). That
would be a *really* nice cleanup.

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:598: if (url.is_valid()) {
This is subtly incorrect... if the url is invalid then this falls through and
ends up returning true.

However, I think that the reason the URL was invalid was something to do with it
not being set until after load? In which case I *think* that it's safe to CHECK
this now, since we're lazy. Perhaps that's one of the things that was blocking
us from converting to features properly?

Let's try a CHECK. if it starts crashing we can just cherry-pick a fix :)

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:697:
extension->optional_permission_set()->HasAnyAccessToAPI(name);
Hum, this is interesting actually. Again, since we're injecting lazily, in
theory we can actually do this on demand. However, makes sense for a follow-up
patch.

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:743:
partially_unprivileged_apis_.count(name);
another cleanup I'm looking forward to: not doing this.

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
File chrome/common/extensions/api/extension_api.h (right):

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.h:133: bool
IsNonFeatureAPIAvailable(const std::string& name,
cool, got it.

move this up to line 126 and make that NOTE/TODO by aa imply that it applies
here too?

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
File chrome/common/extensions/features/simple_feature.cc (right):

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
chrome/common/extensions/features/simple_feature.cc:145:
DCHECK(matches->GetString(i, &pattern));
heisenbug!

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
chrome/common/extensions/features/simple_feature.cc:295: contexts_.find(context)
== contexts_.end()) {
will this fit on 1 line?

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
chrome/common/extensions/features/simple_feature.cc:299: return
CreateAvailability(INVALID_CONTEXT);
nit: ternary

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
chrome/common/extensions/features/simple_feature.cc:318: return
base::StringPrintf("'%s' is not allowed for this url.",
yeah, thread the URL through to GetAvailabilityMessage (but perhaps add an
overloaded version that passes in an empty URL).

https://codereview.chromium.org/12522004/diff/19001/chrome/renderer/extension...
File chrome/renderer/extensions/api_definitions_natives.cc (right):

https://codereview.chromium.org/12522004/diff/19001/chrome/renderer/extension...
chrome/renderer/extensions/api_definitions_natives.cc:48: return
dispatcher()->v8_schema_registry()->GetSchemas(apis_to_check);
since all APIs are available all this logic can be a lot simpler. In fact I
think the whole function can be just be lines 34-41.

https://codereview.chromium.org/12522004/diff/19001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/12522004/diff/19001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:953: if (extension &&
!extension->is_platform_app())
I'm happy every time I see this!

https://codereview.chromium.org/12522004/diff/19001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:955: module_system->Require("json"); 
// see paranoid comment in json.js
hm, this might end up being a problem. maybe. I just filed 223170, and you might
want to reference it here.

https://codereview.chromium.org/12522004/diff/19001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:960:
InstallBindings(module_system.get(), v8_context, "webstore");
with this patch these two lines might actually be unnecessary now, since the
features will pick up on it? or very close to?

https://codereview.chromium.org/12522004/diff/19001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/test_custom_bindings.js (right):

https://codereview.chromium.org/12522004/diff/19001/chrome/renderer/resources...
chrome/renderer/resources/extensions/test_custom_bindings.js:294: return
available_apis;;
woo:

return GetExtensionAPIDefinition().filter(function(apiDef) {
  return GetAvailability(apiDef.namespace).is_available;
});

or something like that...

p.s. sheesh why is that not GetExtensionAPIDefinition*s*

[cduvall, 2013-03-22 20:26:45]

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
File chrome/common/extensions/api/extension_api.cc (right):

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:427:
ResolveDependencies(&dependency_names);
On 2013/03/22 18:13:48, kalman wrote:
> as I mentioned in IRC - I think we can entirely get rid of the dependencies
now
> that we load types properly - dependency_names, ResolveDependencies, that
> spurious looking for loop below (I guess you'd need to just collapse it). That
> would be a *really* nice cleanup.

Discussed in IRC, the loop is still needed because feature dependencies still
need to be checked.

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:598: if (url.is_valid()) {
On 2013/03/22 18:13:48, kalman wrote:
> This is subtly incorrect... if the url is invalid then this falls through and
> ends up returning true.
> 
> However, I think that the reason the URL was invalid was something to do with
it
> not being set until after load? In which case I *think* that it's safe to
CHECK
> this now, since we're lazy. Perhaps that's one of the things that was blocking
> us from converting to features properly?
> 
> Let's try a CHECK. if it starts crashing we can just cherry-pick a fix :)

Done.

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
File chrome/common/extensions/api/extension_api.h (right):

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.h:133: bool
IsNonFeatureAPIAvailable(const std::string& name,
On 2013/03/22 18:13:48, kalman wrote:
> cool, got it.
> 
> move this up to line 126 and make that NOTE/TODO by aa imply that it applies
> here too?

Done.

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
File chrome/common/extensions/features/simple_feature.cc (right):

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
chrome/common/extensions/features/simple_feature.cc:295: contexts_.find(context)
== contexts_.end()) {
On 2013/03/22 18:13:48, kalman wrote:
> will this fit on 1 line?

Done.

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
chrome/common/extensions/features/simple_feature.cc:299: return
CreateAvailability(INVALID_CONTEXT);
On 2013/03/22 18:13:48, kalman wrote:
> nit: ternary

Done.

https://codereview.chromium.org/12522004/diff/19001/chrome/common/extensions/...
chrome/common/extensions/features/simple_feature.cc:318: return
base::StringPrintf("'%s' is not allowed for this url.",
On 2013/03/22 18:13:48, kalman wrote:
> yeah, thread the URL through to GetAvailabilityMessage (but perhaps add an
> overloaded version that passes in an empty URL).

Done.

https://codereview.chromium.org/12522004/diff/19001/chrome/renderer/extension...
File chrome/renderer/extensions/api_definitions_natives.cc (right):

https://codereview.chromium.org/12522004/diff/19001/chrome/renderer/extension...
chrome/renderer/extensions/api_definitions_natives.cc:48: return
dispatcher()->v8_schema_registry()->GetSchemas(apis_to_check);
On 2013/03/22 18:13:48, kalman wrote:
> since all APIs are available all this logic can be a lot simpler. In fact I
> think the whole function can be just be lines 34-41.

Done.

https://codereview.chromium.org/12522004/diff/19001/chrome/renderer/extension...
File chrome/renderer/extensions/dispatcher.cc (right):

https://codereview.chromium.org/12522004/diff/19001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:955: module_system->Require("json"); 
// see paranoid comment in json.js
On 2013/03/22 18:13:48, kalman wrote:
> hm, this might end up being a problem. maybe. I just filed 223170, and you
might
> want to reference it here.

Done.

https://codereview.chromium.org/12522004/diff/19001/chrome/renderer/extension...
chrome/renderer/extensions/dispatcher.cc:960:
InstallBindings(module_system.get(), v8_context, "webstore");
On 2013/03/22 18:13:48, kalman wrote:
> with this patch these two lines might actually be unnecessary now, since the
> features will pick up on it? or very close to?

Done.

https://codereview.chromium.org/12522004/diff/19001/chrome/renderer/resources...
File chrome/renderer/resources/extensions/test_custom_bindings.js (right):

https://codereview.chromium.org/12522004/diff/19001/chrome/renderer/resources...
chrome/renderer/resources/extensions/test_custom_bindings.js:294: return
available_apis;;
On 2013/03/22 18:13:48, kalman wrote:
> woo:
> 
> return GetExtensionAPIDefinition().filter(function(apiDef) {
>   return GetAvailability(apiDef.namespace).is_available;
> });
> 
> or something like that...
> 
> p.s. sheesh why is that not GetExtensionAPIDefinition*s*

Done.

https://codereview.chromium.org/12522004/diff/27002/chrome/common/extensions/...
File chrome/common/extensions/api/extension_api.cc (right):

https://codereview.chromium.org/12522004/diff/27002/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:436: if (feature_type == "api" &&
!UsesFeatureSystem(feature_name)) {
This now *should* never happen since non-features shouldn't use dependencies.
Should I CHECK it?

https://codereview.chromium.org/12522004/diff/27002/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:456: Feature* feature =
GetFeatureDependency(full_name);
Still not quite sure what to do here. Check the actual dependency?

[not at google - send to devlin, 2013-03-22 22:10:12]

basically lg just want to check that last set of changes.

https://codereview.chromium.org/12522004/diff/27002/chrome/common/extensions/...
File chrome/common/extensions/api/extension_api.cc (right):

https://codereview.chromium.org/12522004/diff/27002/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:436: if (feature_type == "api" &&
!UsesFeatureSystem(feature_name)) {
On 2013/03/22 20:26:45, cduvall wrote:
> This now *should* never happen since non-features shouldn't use dependencies.
> Should I CHECK it?

Nah just take it out, code will be safe by virtue of being in feature files soon
anyway.

https://codereview.chromium.org/12522004/diff/27002/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:456: Feature* feature =
GetFeatureDependency(full_name);
On 2013/03/22 20:26:45, cduvall wrote:
> Still not quite sure what to do here. Check the actual dependency?

see long IRC essay

https://codereview.chromium.org/12522004/diff/27002/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:456: Feature* feature =
GetFeatureDependency(full_name);
On 2013/03/22 20:26:45, cduvall wrote:
> Still not quite sure what to do here. Check the actual dependency?

IRC resolved this right/

https://codereview.chromium.org/12522004/diff/27002/chrome/common/extensions/...
File chrome/common/extensions/features/simple_feature.cc (right):

https://codereview.chromium.org/12522004/diff/27002/chrome/common/extensions/...
chrome/common/extensions/features/simple_feature.cc:315: case INVALID_URL:
CHECK(url..)?

https://codereview.chromium.org/12522004/diff/27002/chrome/renderer/extension...
File chrome/renderer/extensions/api_definitions_natives.cc (right):

https://codereview.chromium.org/12522004/diff/27002/chrome/renderer/extension...
chrome/renderer/extensions/api_definitions_natives.cc:31: return
dispatcher()->v8_schema_registry()->GetSchemas(available_apis);
Oh I think it can be even simpler (sorry) looks like this is only ever called
with no args so it can be *only* line 31.

https://codereview.chromium.org/12522004/diff/27002/tools/json_schema_compile...
File tools/json_schema_compiler/compiler.py (right):

https://codereview.chromium.org/12522004/diff/27002/tools/json_schema_compile...
tools/json_schema_compiler/compiler.py:51: class TypeNamespaceResolver(object):
pull into separate file?

https://codereview.chromium.org/12522004/diff/27002/tools/json_schema_compile...
tools/json_schema_compiler/compiler.py:61: return None
can this be an assertion?

https://codereview.chromium.org/12522004/diff/27002/tools/json_schema_compile...
tools/json_schema_compiler/compiler.py:71: return None
I think a structure like

real_name = None
for ext in ['json, 'idl']:
  ...
assert(real_name is not None,
       'Cannot find schema for %s' % namespace_name)

or something like that would be a bit nicer.

https://codereview.chromium.org/12522004/diff/27002/tools/json_schema_compile...
tools/json_schema_compiler/compiler.py:75: return None
can this be an assertion?

[cduvall, 2013-03-22 22:52:29]

https://codereview.chromium.org/12522004/diff/27002/chrome/common/extensions/...
File chrome/common/extensions/api/extension_api.cc (right):

https://codereview.chromium.org/12522004/diff/27002/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:436: if (feature_type == "api" &&
!UsesFeatureSystem(feature_name)) {
On 2013/03/22 22:10:12, kalman wrote:
> On 2013/03/22 20:26:45, cduvall wrote:
> > This now *should* never happen since non-features shouldn't use
dependencies.
> > Should I CHECK it?
> 
> Nah just take it out, code will be safe by virtue of being in feature files
soon
> anyway.

Done.

https://codereview.chromium.org/12522004/diff/27002/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:456: Feature* feature =
GetFeatureDependency(full_name);
On 2013/03/22 22:10:12, kalman wrote:
> On 2013/03/22 20:26:45, cduvall wrote:
> > Still not quite sure what to do here. Check the actual dependency?
> 
> IRC resolved this right/

Yep, resolved.

https://codereview.chromium.org/12522004/diff/27002/chrome/common/extensions/...
File chrome/common/extensions/features/simple_feature.cc (right):

https://codereview.chromium.org/12522004/diff/27002/chrome/common/extensions/...
chrome/common/extensions/features/simple_feature.cc:315: case INVALID_URL:
On 2013/03/22 22:10:12, kalman wrote:
> CHECK(url..)?

Done.

https://codereview.chromium.org/12522004/diff/27002/chrome/renderer/extension...
File chrome/renderer/extensions/api_definitions_natives.cc (right):

https://codereview.chromium.org/12522004/diff/27002/chrome/renderer/extension...
chrome/renderer/extensions/api_definitions_natives.cc:31: return
dispatcher()->v8_schema_registry()->GetSchemas(available_apis);
On 2013/03/22 22:10:12, kalman wrote:
> Oh I think it can be even simpler (sorry) looks like this is only ever called
> with no args so it can be *only* line 31.

Done.

https://codereview.chromium.org/12522004/diff/27002/tools/json_schema_compile...
File tools/json_schema_compiler/compiler.py (right):

https://codereview.chromium.org/12522004/diff/27002/tools/json_schema_compile...
tools/json_schema_compiler/compiler.py:51: class TypeNamespaceResolver(object):
On 2013/03/22 22:10:12, kalman wrote:
> pull into separate file?

Done.

https://codereview.chromium.org/12522004/diff/27002/tools/json_schema_compile...
tools/json_schema_compiler/compiler.py:61: return None
On 2013/03/22 22:10:12, kalman wrote:
> can this be an assertion?

Still returning None, to be handled in CppTypeGenerator.

https://codereview.chromium.org/12522004/diff/27002/tools/json_schema_compile...
tools/json_schema_compiler/compiler.py:71: return None
On 2013/03/22 22:10:12, kalman wrote:
> I think a structure like
> 
> real_name = None
> for ext in ['json, 'idl']:
>   ...
> assert(real_name is not None,
>        'Cannot find schema for %s' % namespace_name)
> 
> or something like that would be a bit nicer.

Done.

https://codereview.chromium.org/12522004/diff/27002/tools/json_schema_compile...
tools/json_schema_compiler/compiler.py:75: return None
On 2013/03/22 22:10:12, kalman wrote:
> can this be an assertion?

Same as above.

[not at google - send to devlin, 2013-03-22 23:04:30]

lgtm

https://codereview.chromium.org/12522004/diff/48004/chrome/common/extensions/...
File chrome/common/extensions/api/extension_api.cc (right):

https://codereview.chromium.org/12522004/diff/48004/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:426:
ResolveDependencies(&dependency_names);
shouldn't this come after inserting full_name into dependency_names?

https://codereview.chromium.org/12522004/diff/48004/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:436: kUnavailableMessage);
seems like this could be simplified:

// APIs not using the feature system...
if (!UsesFeatureSystem(full_name)) {
  return IsNonFeatureAPIAvailable(full_name, context, extension, url) ?
      Feature::CreateAvailablity(Feature::IS_AVAILABLE, "") :
      Feature::CreateAvailabilty(Feature::INVALID_CONTEXT,
                                 kUnavailableMessage);
}

std::set<std::string> dependency_names;
...

https://codereview.chromium.org/12522004/diff/48004/chrome/common/extensions/...
File chrome/common/extensions/features/permission_feature.cc (right):

https://codereview.chromium.org/12522004/diff/48004/chrome/common/extensions/...
chrome/common/extensions/features/permission_feature.cc:30:
!extension->optional_permission_set()->HasAnyAccessToAPI(name())) {
could you explain this in a comment?

[cduvall, 2013-03-22 23:20:59]

https://codereview.chromium.org/12522004/diff/48004/chrome/common/extensions/...
File chrome/common/extensions/api/extension_api.cc (right):

https://codereview.chromium.org/12522004/diff/48004/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:426:
ResolveDependencies(&dependency_names);
On 2013/03/22 23:04:30, kalman wrote:
> shouldn't this come after inserting full_name into dependency_names?

Done.

https://codereview.chromium.org/12522004/diff/48004/chrome/common/extensions/...
chrome/common/extensions/api/extension_api.cc:436: kUnavailableMessage);
On 2013/03/22 23:04:30, kalman wrote:
> seems like this could be simplified:
> 
> // APIs not using the feature system...
> if (!UsesFeatureSystem(full_name)) {
>   return IsNonFeatureAPIAvailable(full_name, context, extension, url) ?
>       Feature::CreateAvailablity(Feature::IS_AVAILABLE, "") :
>       Feature::CreateAvailabilty(Feature::INVALID_CONTEXT,
>                                  kUnavailableMessage);
> }
> 
> std::set<std::string> dependency_names;
> ...

Done.

[commit-bot: I haz the power, 2013-03-22 23:22:21]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/12522004/62001

[commit-bot: I haz the power, 2013-03-23 14:57:40]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/cduvall@chromium.org/12522004/62001

[commit-bot: I haz the power, 2013-03-23 18:49:32]

Change committed as 190058