Extensions: Exclude manifest only schemas from api and bundle

registration.

Manifest-only schemas don't need to go through api_registration (which
generates api function registration) and generated_api_json_strings
(which generates api schema strings) build steps. Exclude them from the
same. This also helps ensure we don't redundantly generated schema
strings for these apis.

BUG=1113513

Change-Id: I69c39cda6a6ac78fd47e6bf01e44110e221bb76a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2380614
Commit-Queue: Karan Bhatia <karandeepb@chromium.org>
Reviewed-by: Devlin <rdevlin.cronin@chromium.org>
Cr-Commit-Position: refs/heads/master@{#810117}

chrome/common/extensions/api/BUILD.gn

@@ -72,7 +72,8 @@ generated_json_strings("generated_api_json_strings") {
 }
 
 generated_types("generated_api_types") {
-  sources = chrome_extensions_api_schema_sources
+  sources = chrome_extensions_api_schema_sources +
+            chrome_extensions_manifest_only_schema_sources
   configs = [ "//build/config:precompiled_headers" ]
   schema_include_rules = chrome_extensions_api_schema_include_rules
 

chrome/common/extensions/api/_api_features.json

@@ -432,7 +432,7 @@
   "extensionsManifestTypes": {
     "internal": true,
     "channel": "stable",
-    "contexts": ["blessed_extension"]
+    "contexts": []
   },
   "fileBrowserHandler": {
     "dependencies": ["permission:fileBrowserHandler"],
@@ -569,7 +569,7 @@
   "manifestTypes": {
     "internal": true,
     "channel": "stable",
-    "contexts": ["blessed_extension"]
+    "contexts": []
   },
   "mediaPlayerPrivate": {
     "dependencies": ["permission:mediaPlayerPrivate"],

chrome/common/extensions/api/_features.md

@@ -211,7 +211,9 @@ The only accepted value is the bool `false` (since true is the default).
 
 The `contexts` property specifies which JavaScript contexts can access the
 feature. All API features must specify at least one context, and only API
-features can specify contexts.
+features can specify contexts. The only exception to this are dummy namespaces
+like `manifestTypes` etc. which can specify an empty list as its `contexts`
+property.
 
 Accepted values are a list of strings from `blessed_extension`,
 `blessed_web_page`, `content_script`, `extension_service_worker`,

chrome/common/extensions/api/api_sources.gni

@@ -41,7 +41,6 @@ schema_sources_ = [
   "image_writer_private.idl",
   "instance_id.json",
   "language_settings_private.idl",
-  "manifest_types.json",
   "notifications.idl",
   "omnibox.json",
   "page_capture.json",
@@ -157,6 +156,9 @@ if (is_chromeos) {
 
 chrome_extensions_api_schema_sources = get_path_info(schema_sources_, "abspath")
 
+chrome_extensions_manifest_only_schema_sources =
+    get_path_info([ "manifest_types.json" ], "abspath")
+
 chrome_extensions_api_uncompiled_sources =
     get_path_info(uncompiled_sources_, "abspath")
 

chrome/renderer/extensions/chrome_extensions_renderer_client.cc

@@ -210,9 +210,6 @@ bool ChromeExtensionsRendererClient::AllowPopup() {
       return true;
     case extensions::Feature::BLESSED_WEB_PAGE_CONTEXT:
       return !current_context->web_frame()->Parent();
-    default:
-      NOTREACHED();
-      return false;
   }
 }
 

extensions/common/api/BUILD.gn

@@ -52,7 +52,7 @@ generated_json_strings("generated_api_json_strings") {
 }
 
 generated_types("generated_api_types") {
-  sources = extensions_api_schema_files
+  sources = extensions_api_schema_files + extensions_manifest_only_schema_files
   root_namespace = extensions_api_root_namespace
   deps = [
     "//base",

extensions/common/api/schema.gni

@@ -22,7 +22,6 @@ extensions_api_schema_files_ = [
   "display_source.idl",
   "dns.idl",
   "events.json",
-  "extensions_manifest_types.json",
   "extension_options_internal.idl",
   "extension_types.json",
   "feedback_private.idl",
@@ -31,7 +30,6 @@ extensions_api_schema_files_ = [
   "management.json",
   "hid.idl",
   "idle.json",
-  "incognito.json",
   "metrics_private.json",
   "mime_handler_private.idl",
   "mime_handler_view_guest_internal.json",
@@ -61,6 +59,11 @@ extensions_api_schema_files_ = [
   "web_view_internal.json",
 ]
 
+extensions_manifest_only_schema_files_ = [
+  "extensions_manifest_types.json",
+  "incognito.json",
+]
+
 if (is_chromeos) {
   extensions_api_schema_files_ += [
     "crash_report_private.idl",
@@ -76,6 +79,9 @@ if (is_chromeos) {
 extensions_api_schema_files =
     get_path_info(extensions_api_schema_files_, "abspath")
 
+extensions_manifest_only_schema_files =
+    get_path_info(extensions_manifest_only_schema_files_, "abspath")
+
 extensions_api_uncompiled_sources =
     get_path_info([ "web_request_internal.json" ], "abspath")
 

extensions/common/features/simple_feature.cc

@@ -310,11 +310,12 @@ std::string SimpleFeature::GetAvailabilityMessage(
               extension_types_.begin(), extension_types_.end())).c_str(),
           GetDisplayName(type).c_str());
     case INVALID_CONTEXT:
+      DCHECK(contexts_);
       return base::StringPrintf(
-          "'%s' is only allowed to run in %s, but this is a %s",
-          name().c_str(),
-          ListDisplayNames(std::vector<Context>(
-              contexts_.begin(), contexts_.end())).c_str(),
+          "'%s' is only allowed to run in %s, but this is a %s", name().c_str(),
+          ListDisplayNames(
+              std::vector<Context>(contexts_->begin(), contexts_->end()))
+              .c_str(),
           GetDisplayName(context).c_str());
     case INVALID_LOCATION:
       return base::StringPrintf(
@@ -653,7 +654,7 @@ Feature::Availability SimpleFeature::GetContextAvailability(
   // extension API calls, since there's no guarantee that the extension is
   // "active" in current renderer process when the API permission check is
   // done.
-  if (!contexts_.empty() && !base::Contains(contexts_, context))
+  if (contexts_ && !base::Contains(*contexts_, context))
     return CreateAvailability(INVALID_CONTEXT, context);
 
   // TODO(kalman): Consider checking |matches_| regardless of context type.

extensions/common/features/simple_feature.h

@@ -136,7 +136,9 @@ class SimpleFeature : public Feature {
     return extension_types_;
   }
   const std::vector<Platform>& platforms() const { return platforms_; }
-  const std::vector<Context>& contexts() const { return contexts_; }
+  const base::Optional<std::vector<Context>>& contexts() const {
+    return contexts_;
+  }
   const std::vector<std::string>& dependencies() const { return dependencies_; }
   const base::Optional<version_info::Channel> channel() const {
     return channel_;
@@ -230,7 +232,7 @@ class SimpleFeature : public Feature {
   std::vector<std::string> dependencies_;
   std::vector<Manifest::Type> extension_types_;
   std::vector<FeatureSessionType> session_types_;
-  std::vector<Context> contexts_;
+  base::Optional<std::vector<Context>> contexts_;
   std::vector<Platform> platforms_;
   URLPatternSet matches_;
 

tools/json_schema_compiler/feature_compiler.py

@@ -163,7 +163,8 @@ FEATURE_GRAMMAR = ({
                 'webui_untrusted': 'Feature::WEBUI_UNTRUSTED_CONTEXT',
                 'unblessed_extension': 'Feature::UNBLESSED_EXTENSION_CONTEXT',
             },
-            'allow_all': True
+            'allow_all': True,
+            'allow_empty': True
         },
     },
     'default_parent': {
@@ -286,6 +287,28 @@ def DoesNotHaveAllProperties(property_names, value):
 def DoesNotHaveProperty(property_name, value):
   return property_name not in value
 
+def IsEmptyContextsAllowed(feature, all_features):
+  # An alias feature wouldn't have the 'contexts' feature value.
+  if feature.GetValue('source'):
+    return True
+
+  if type(feature) is ComplexFeature:
+    for child_feature in feature.feature_list:
+      if not IsEmptyContextsAllowed(child_feature, all_features):
+        return False
+    return True
+
+  contexts = feature.GetValue('contexts')
+  assert contexts, 'contexts must have been specified for the APIFeature'
+
+  allowlisted_empty_context_namespaces = [
+    'manifestTypes',
+    'extensionsManifestTypes',
+    'empty_contexts' # Only added for testing.
+  ]
+  return (contexts != '{}' or
+          feature.name in allowlisted_empty_context_namespaces)
+
 def IsFeatureCrossReference(property_name, reverse_property_name, feature,
                             all_features):
   """ Verifies that |property_name| on |feature| references a feature that
@@ -386,7 +409,7 @@ VALIDATION = ({
   ],
   'APIFeature': [
     (partial(HasProperty, 'contexts'),
-     'APIFeatures must specify at least one context'),
+     'APIFeatures must specify the contexts property'),
     (partial(DoesNotHaveAllProperties, ['alias', 'source']),
      'Features cannot specify both alias and source.')
   ],
@@ -426,8 +449,9 @@ FINAL_VALIDATION = ({
      'property references it back.'),
     (partial(IsFeatureCrossReference, 'source', 'alias'),
      'A feature source property should reference a feature whose alias '
-     'property references it back.')
-
+     'property references it back.'),
+    (IsEmptyContextsAllowed,
+    'An empty contexts list is not allowed for this feature.')
   ],
   'ManifestFeature': [],
   'BehaviorFeature': [],
@@ -596,7 +620,6 @@ class Feature(object):
                                               sub_value)
         if cpp_sub_value:
           cpp_value.append(cpp_sub_value)
-      if cpp_value:
       cpp_value = '{' + ','.join(cpp_value) + '}'
     else:
       cpp_value = self._GetCheckedValue(key, expected_type, expected_values,

tools/json_schema_compiler/feature_compiler_test.py

@@ -102,7 +102,7 @@ class FeatureCompilerTest(unittest.TestCase):
     self._hasError(f, 'Illegal value: "False"')
 
   def testEmptyList(self):
-    f = self._parseFeature({'contexts': []})
+    f = self._parseFeature({'extension_types': []})
     self._hasError(f, 'List must specify at least one element.')
 
   def testEmptyListWithAllowEmpty(self):
@@ -111,11 +111,17 @@ class FeatureCompilerTest(unittest.TestCase):
     self.assertFalse(f.GetErrors())
 
   def testApiFeaturesNeedContexts(self):
-    f = self._parseFeature({'dependencies': 'alpha',
-                            'extension_types': ['extension'],
+    f = self._parseFeature({'extension_types': ['extension'],
                             'channel': 'trunk'})
     f.Validate('APIFeature', {})
-    self._hasError(f, 'APIFeatures must specify at least one context')
+    self._hasError(f, 'APIFeatures must specify the contexts property')
+
+  def testAPIFeaturesCanSpecifyEmptyContexts(self):
+    f = self._parseFeature({'extension_types': ['extension'],
+                            'channel': 'trunk',
+                            'contexts': []})
+    f.Validate('APIFeature', {})
+    self.assertFalse(f.GetErrors())
 
   def testManifestFeaturesNeedExtensionTypes(self):
     f = self._parseFeature({'dependencies': 'alpha', 'channel': 'beta'})
@@ -430,5 +436,36 @@ class FeatureCompilerTest(unittest.TestCase):
     self._hasError(feature,
                    'Hosted apps are not allowed to use restricted features')
 
+  def testEmptyContextsDisallowed(self):
+    compiler = self._createTestFeatureCompiler('APIFeature')
+    compiler._json = {
+      'feature_alpha': {
+        'channel': 'beta',
+        'contexts': [],
+        'extension_types': ['extension']
+      }
+    }
+    compiler.Compile()
+
+    feature = compiler._features.get('feature_alpha')
+    self.assertTrue(feature)
+    self._hasError(feature,
+        'An empty contexts list is not allowed for this feature.')
+
+  def testEmptyContextsAllowed(self):
+    compiler = self._createTestFeatureCompiler('APIFeature')
+    compiler._json = {
+      'empty_contexts': {
+        'channel': 'beta',
+        'contexts': [],
+        'extension_types': ['extension']
+      }
+    }
+    compiler.Compile()
+
+    feature = compiler._features.get('empty_contexts')
+    self.assertTrue(feature)
+    self.assertFalse(feature.GetErrors())
+
 if __name__ == '__main__':
   unittest.main()

tools/json_schema_compiler/test/features_generation_unittest.cc

@@ -23,6 +23,19 @@ void ExpectVectorsEqual(std::vector<T> expected,
   EXPECT_EQ(expected, actual) << name;
 }
 
+template <typename T>
+void ExpectOptionalVectorsEqual(const base::Optional<std::vector<T>>& expected,
+                                const base::Optional<std::vector<T>>& actual,
+                                const std::string& name) {
+  if (expected.has_value() != actual.has_value()) {
+    ADD_FAILURE() << "Mismatched optional vectors for " << name << ": "
+                  << expected.has_value() << " vs " << actual.has_value();
+    return;
+  }
+  if (expected.has_value())
+    ExpectVectorsEqual(*expected, *actual, name);
+}
+
 const bool kDefaultAutoGrant = true;
 const bool kDefaultInternal = false;
 
@@ -41,7 +54,7 @@ struct FeatureComparator {
   std::vector<std::string> allowlist;
   std::vector<std::string> dependencies;
   std::vector<Manifest::Type> extension_types;
-  std::vector<Feature::Context> contexts;
+  base::Optional<std::vector<Feature::Context>> contexts;
   std::vector<Feature::Platform> platforms;
 
   URLPatternSet matches;
@@ -73,7 +86,7 @@ void FeatureComparator::CompareFeature(const SimpleFeature* feature) {
   ExpectVectorsEqual(allowlist, feature->allowlist(), name);
   ExpectVectorsEqual(dependencies, feature->dependencies(), name);
   ExpectVectorsEqual(extension_types, feature->extension_types(), name);
-  ExpectVectorsEqual(contexts, feature->contexts(), name);
+  ExpectOptionalVectorsEqual(contexts, feature->contexts(), name);
   ExpectVectorsEqual(platforms, feature->platforms(), name);
   EXPECT_EQ(matches, feature->matches()) << name;
   EXPECT_EQ(location, feature->location()) << name;
@@ -112,7 +125,8 @@ TEST(FeaturesGenerationTest, FeaturesTest) {
     const SimpleFeature* feature = GetAsSimpleFeature("alpha");
     FeatureComparator comparator("alpha");
     comparator.dependencies = {"permission:alpha"};
-    comparator.contexts = {Feature::BLESSED_EXTENSION_CONTEXT};
+    comparator.contexts =
+        std::vector<Feature::Context>({Feature::BLESSED_EXTENSION_CONTEXT});
     comparator.channel = version_info::Channel::STABLE;
     comparator.max_manifest_version = 1;
     comparator.CompareFeature(feature);
@@ -120,7 +134,8 @@ TEST(FeaturesGenerationTest, FeaturesTest) {
   {
     const SimpleFeature* feature = GetAsSimpleFeature("beta");
     FeatureComparator comparator("beta");
-    comparator.contexts = {Feature::BLESSED_EXTENSION_CONTEXT};
+    comparator.contexts =
+        std::vector<Feature::Context>({Feature::BLESSED_EXTENSION_CONTEXT});
     comparator.channel = version_info::Channel::DEV;
     comparator.extension_types = {Manifest::TYPE_EXTENSION,
                                   Manifest::TYPE_PLATFORM_APP};
@@ -137,7 +152,8 @@ TEST(FeaturesGenerationTest, FeaturesTest) {
     FeatureComparator comparator("gamma");
     comparator.channel = version_info::Channel::BETA;
     comparator.platforms = {Feature::WIN_PLATFORM, Feature::MACOSX_PLATFORM};
-    comparator.contexts = {Feature::BLESSED_EXTENSION_CONTEXT};
+    comparator.contexts =
+        std::vector<Feature::Context>({Feature::BLESSED_EXTENSION_CONTEXT});
     comparator.dependencies = {"permission:gamma"};
     comparator.extension_types = {Manifest::TYPE_EXTENSION};
     comparator.internal = true;
@@ -158,7 +174,8 @@ TEST(FeaturesGenerationTest, FeaturesTest) {
     const SimpleFeature* feature = GetAsSimpleFeature("gamma.unparented");
     FeatureComparator comparator("gamma.unparented");
     comparator.blocklist = {"0123456789ABCDEF0123456789ABCDEF01234567"};
-    comparator.contexts = {Feature::UNBLESSED_EXTENSION_CONTEXT};
+    comparator.contexts =
+        std::vector<Feature::Context>({Feature::UNBLESSED_EXTENSION_CONTEXT});
     comparator.channel = version_info::Channel::DEV;
     comparator.CompareFeature(feature);
   }
@@ -166,7 +183,8 @@ TEST(FeaturesGenerationTest, FeaturesTest) {
     const ComplexFeature* complex_feature =
         GetAsComplexFeature("gamma.complex_unparented");
     FeatureComparator comparator("gamma.complex_unparented");
-    comparator.contexts = {Feature::UNBLESSED_EXTENSION_CONTEXT};
+    comparator.contexts =
+        std::vector<Feature::Context>({Feature::UNBLESSED_EXTENSION_CONTEXT});
     comparator.channel = version_info::Channel::STABLE;
     // We cheat and have both children exactly the same for ease of comparing;
     // complex features are tested more thoroughly below.
@@ -176,8 +194,8 @@ TEST(FeaturesGenerationTest, FeaturesTest) {
   {
     const SimpleFeature* feature = GetAsSimpleFeature("delta");
     FeatureComparator comparator("delta");
-    comparator.contexts = {Feature::BLESSED_EXTENSION_CONTEXT,
-                           Feature::WEBUI_CONTEXT};
+    comparator.contexts = std::vector<Feature::Context>(
+        {Feature::BLESSED_EXTENSION_CONTEXT, Feature::WEBUI_CONTEXT});
     comparator.channel = version_info::Channel::DEV;
     comparator.matches.AddPattern(
         URLPattern(URLPattern::SCHEME_ALL, "*://example.com/*"));
@@ -187,7 +205,8 @@ TEST(FeaturesGenerationTest, FeaturesTest) {
   {
     const SimpleFeature* feature = GetAsSimpleFeature("pi");
     FeatureComparator comparator("pi");
-    comparator.contexts = {Feature::WEBUI_UNTRUSTED_CONTEXT};
+    comparator.contexts =
+        std::vector<Feature::Context>({Feature::WEBUI_UNTRUSTED_CONTEXT});
     comparator.channel = version_info::Channel::STABLE;
     comparator.matches.AddPattern(
         URLPattern(URLPattern::SCHEME_ALL, "chrome-untrusted://foo/*"));
@@ -196,14 +215,12 @@ TEST(FeaturesGenerationTest, FeaturesTest) {
   {
     const SimpleFeature* feature = GetAsSimpleFeature("allEnum");
     FeatureComparator comparator("allEnum");
-    comparator.contexts = {Feature::BLESSED_EXTENSION_CONTEXT,
-                           Feature::BLESSED_WEB_PAGE_CONTEXT,
+    comparator.contexts = std::vector<Feature::Context>(
+        {Feature::BLESSED_EXTENSION_CONTEXT, Feature::BLESSED_WEB_PAGE_CONTEXT,
          Feature::CONTENT_SCRIPT_CONTEXT,
-                           Feature::LOCK_SCREEN_EXTENSION_CONTEXT,
-                           Feature::WEB_PAGE_CONTEXT,
-                           Feature::WEBUI_CONTEXT,
-                           Feature::WEBUI_UNTRUSTED_CONTEXT,
-                           Feature::UNBLESSED_EXTENSION_CONTEXT};
+         Feature::LOCK_SCREEN_EXTENSION_CONTEXT, Feature::WEB_PAGE_CONTEXT,
+         Feature::WEBUI_CONTEXT, Feature::WEBUI_UNTRUSTED_CONTEXT,
+         Feature::UNBLESSED_EXTENSION_CONTEXT});
     comparator.extension_types = {Manifest::TYPE_EXTENSION,
                                   Manifest::TYPE_HOSTED_APP,
                                   Manifest::TYPE_LEGACY_PACKAGED_APP,
@@ -218,7 +235,8 @@ TEST(FeaturesGenerationTest, FeaturesTest) {
     // Omega is imported from a second .json file.
     const SimpleFeature* feature = GetAsSimpleFeature("omega");
     FeatureComparator comparator("omega");
-    comparator.contexts = {Feature::WEB_PAGE_CONTEXT};
+    comparator.contexts =
+        std::vector<Feature::Context>({Feature::WEB_PAGE_CONTEXT});
     comparator.channel = version_info::Channel::DEV;
     comparator.min_manifest_version = 2;
     comparator.CompareFeature(feature);
@@ -256,7 +274,8 @@ TEST(FeaturesGenerationTest, FeaturesTest) {
       // Check the default parent.
       FeatureComparator comparator("complex");
       comparator.channel = version_info::Channel::STABLE;
-      comparator.contexts = {Feature::BLESSED_EXTENSION_CONTEXT};
+      comparator.contexts =
+          std::vector<Feature::Context>({Feature::BLESSED_EXTENSION_CONTEXT});
       comparator.extension_types = {Manifest::TYPE_EXTENSION};
       comparator.CompareFeature(default_parent);
       // Check the child of the complex feature. It should inherit its
@@ -271,7 +290,8 @@ TEST(FeaturesGenerationTest, FeaturesTest) {
       // Finally, check the branch of the complex feature.
       FeatureComparator comparator("complex");
       comparator.channel = version_info::Channel::BETA;
-      comparator.contexts = {Feature::BLESSED_EXTENSION_CONTEXT};
+      comparator.contexts =
+          std::vector<Feature::Context>({Feature::BLESSED_EXTENSION_CONTEXT});
       comparator.extension_types = {Manifest::TYPE_EXTENSION};
       comparator.allowlist = {"0123456789ABCDEF0123456789ABCDEF01234567"};
       comparator.CompareFeature(other_parent);
@@ -282,7 +302,8 @@ TEST(FeaturesGenerationTest, FeaturesTest) {
   {
     const SimpleFeature* feature = GetAsSimpleFeature("alias");
     FeatureComparator comparator("alias");
-    comparator.contexts = {Feature::BLESSED_EXTENSION_CONTEXT};
+    comparator.contexts =
+        std::vector<Feature::Context>({Feature::BLESSED_EXTENSION_CONTEXT});
     comparator.channel = version_info::Channel::STABLE;
     comparator.source = "alias_source";
     comparator.CompareFeature(feature);
@@ -290,7 +311,8 @@ TEST(FeaturesGenerationTest, FeaturesTest) {
   {
     const SimpleFeature* feature = GetAsSimpleFeature("alias_source");
     FeatureComparator comparator("alias_source");
-    comparator.contexts = {Feature::BLESSED_EXTENSION_CONTEXT};
+    comparator.contexts =
+        std::vector<Feature::Context>({Feature::BLESSED_EXTENSION_CONTEXT});
     comparator.channel = version_info::Channel::STABLE;
     comparator.alias = "alias";
     comparator.CompareFeature(feature);
@@ -330,6 +352,13 @@ TEST(FeaturesGenerationTest, FeaturesTest) {
     ASSERT_EQ("", feature->alias());
     ASSERT_EQ("child_source", feature->source());
   }
+  {
+    const SimpleFeature* feature = GetAsSimpleFeature("empty_contexts");
+    FeatureComparator comparator("empty_contexts");
+    comparator.channel = version_info::Channel::BETA;
+    comparator.contexts = std::vector<Feature::Context>();
+    comparator.CompareFeature(feature);
+  }
 }
 
 }  // namespace extensions

tools/json_schema_compiler/test/features_test.json

@@ -150,5 +150,9 @@
     "channel": "beta",
     "contexts": ["blessed_extension"],
     "alias": "alias_parent.child"
+  },
+  "empty_contexts": {
+    "channel": "beta",
+    "contexts": []
   }
 }