Bug 1426259: Revamp processing of manifest.json for themes r=kmag
authorAndrew Swan <aswan@mozilla.com>
Wed, 20 Dec 2017 16:06:50 -0800
changeset 448994 50192b2fe1d4c7b7c88cd5628591f1df452887ff
parent 448993 81d875f176380a2ab705c32dd9c564264c330a47
child 448995 ce3ba591b61a56154f998226a06c2fca33cbd713
push id8527
push userCallek@gmail.com
push dateThu, 11 Jan 2018 21:05:50 +0000
treeherdermozilla-beta@95342d212a7a [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerskmag
bugs1426259
milestone59.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1426259: Revamp processing of manifest.json for themes r=kmag MozReview-Commit-ID: 6dWy8wwrobY
browser/components/extensions/test/browser/browser_ext_themes_validation.js
toolkit/components/extensions/Extension.jsm
toolkit/components/extensions/ExtensionParent.jsm
toolkit/components/extensions/ExtensionXPCShellUtils.jsm
toolkit/components/extensions/schemas/theme.json
toolkit/components/extensions/test/xpcshell/test_ext_manifest_themes.js
toolkit/components/extensions/test/xpcshell/test_ext_themes_supported_properties.js
toolkit/components/extensions/test/xpcshell/xpcshell.ini
--- a/browser/components/extensions/test/browser/browser_ext_themes_validation.js
+++ b/browser/components/extensions/test/browser/browser_ext_themes_validation.js
@@ -28,28 +28,17 @@ async function testThemeWithInvalidPrope
         manifest[prop] = {"keyword": "test"};
         break;
       default:
         manifest[prop] = {};
     }
   });
 
   let extension = ExtensionTestUtils.loadExtension({manifest});
-
-  SimpleTest.waitForExplicitFinish();
-  let waitForConsole = new Promise(resolve => {
-    SimpleTest.monitorConsole(resolve, [{
-      message: /Reading manifest: Themes defined in the manifest may only contain static resources/,
-    }]);
-  });
-
   await Assert.rejects(extension.startup(), null, "Theme should fail to load if it contains invalid properties");
-
-  SimpleTest.endMonitorConsole();
-  await waitForConsole;
 }
 
 add_task(async function test_that_theme_with_invalid_properties_fails_to_load() {
   let invalidProps = ["page_action", "browser_action", "background", "permissions", "omnibox", "commands"];
   for (let prop in invalidProps) {
     await testThemeWithInvalidProperties([prop]);
   }
   await testThemeWithInvalidProperties(invalidProps);
--- a/toolkit/components/extensions/Extension.jsm
+++ b/toolkit/components/extensions/Extension.jsm
@@ -95,49 +95,16 @@ const {
 
 XPCOMUtils.defineLazyGetter(this, "console", ExtensionUtils.getConsole);
 
 XPCOMUtils.defineLazyGetter(this, "LocaleData", () => ExtensionCommon.LocaleData);
 
 // The maximum time to wait for extension shutdown blockers to complete.
 const SHUTDOWN_BLOCKER_MAX_MS = 8000;
 
-// The list of properties that themes are allowed to contain.
-XPCOMUtils.defineLazyGetter(this, "allowedThemeProperties", () => {
-  Cu.import("resource://gre/modules/ExtensionParent.jsm");
-  let propertiesInBaseManifest = ExtensionParent.baseManifestProperties;
-
-  // The properties found in the base manifest contain all of the properties that
-  // themes are allowed to have. However, the list also contains several properties
-  // that aren't allowed, so we need to filter them out first before the list can
-  // be used to validate themes.
-  return propertiesInBaseManifest.filter(prop => {
-    const propertiesToRemove = ["background", "content_scripts", "permissions"];
-    return !propertiesToRemove.includes(prop);
-  });
-});
-
-/**
- * Validates a theme to ensure it only contains static resources.
- *
- * @param {Array<string>} manifestProperties The list of top-level keys found in the
- *    the extension's manifest.
- * @returns {Array<string>} A list of invalid properties or an empty list
- *    if none are found.
- */
-function validateThemeManifest(manifestProperties) {
-  let invalidProps = [];
-  for (let propName of manifestProperties) {
-    if (propName != "theme" && !allowedThemeProperties.includes(propName)) {
-      invalidProps.push(propName);
-    }
-  }
-  return invalidProps;
-}
-
 /**
  * Classify an individual permission from a webextension manifest
  * as a host/origin permission, an api permission, or a regular permission.
  *
  * @param {string} perm  The permission string to classify
  *
  * @returns {object}
  *          An object with exactly one of the following properties:
@@ -519,25 +486,17 @@ class ExtensionData {
       },
 
       preprocessors: {},
     };
 
     let manifestType = "manifest.WebExtensionManifest";
     if (this.manifest.theme) {
       this.type = "theme";
-      // XXX create a separate manifest type for themes
-      let invalidProps = validateThemeManifest(Object.getOwnPropertyNames(this.manifest));
-
-      if (invalidProps.length) {
-        let message = `Themes defined in the manifest may only contain static resources. ` +
-          `If you would like to use additional properties, please use the "theme" permission instead. ` +
-          `(the invalid properties found are: ${invalidProps})`;
-        this.manifestError(message);
-      }
+      manifestType = "manifest.ThemeManifest";
     } else if (this.manifest.langpack_id) {
       this.type = "langpack";
       manifestType = "manifest.WebExtensionLangpackManifest";
     } else {
       this.type = "extension";
     }
 
     if (this.localeData) {
--- a/toolkit/components/extensions/ExtensionParent.jsm
+++ b/toolkit/components/extensions/ExtensionParent.jsm
@@ -1237,19 +1237,16 @@ function watchExtensionProxyContextLoad(
 
   extension.on("extension-proxy-context-load", listener);
 
   return () => {
     extension.off("extension-proxy-context-load", listener);
   };
 }
 
-// Used to cache the list of WebExtensionManifest properties defined in the BASE_SCHEMA.
-let gBaseManifestProperties = null;
-
 // Manages icon details for toolbar buttons in the |pageAction| and
 // |browserAction| APIs.
 let IconDetails = {
   // WeakMap<Extension -> Map<url-string -> Map<iconType-string -> object>>>
   iconCache: new DefaultWeakMap(() => {
     return new DefaultMap(() => new DefaultMap(() => new Map()));
   }),
 
@@ -1585,30 +1582,16 @@ for (let name of StartupCache.STORE_NAME
 var ExtensionParent = {
   GlobalManager,
   HiddenExtensionPage,
   IconDetails,
   ParentAPIManager,
   StartupCache,
   WebExtensionPolicy,
   apiManager,
-  get baseManifestProperties() {
-    if (gBaseManifestProperties) {
-      return gBaseManifestProperties;
-    }
-
-    let types = Schemas.schemaJSON.get(BASE_SCHEMA).deserialize({})[0].types;
-    let manifest = types.find(type => type.id === "WebExtensionManifest");
-    if (!manifest) {
-      throw new Error("Unable to find base manifest properties");
-    }
-
-    gBaseManifestProperties = Object.getOwnPropertyNames(manifest.properties);
-    return gBaseManifestProperties;
-  },
   promiseExtensionViewLoaded,
   watchExtensionProxyContextLoad,
   DebugUtils,
 };
 
 XPCOMUtils.defineLazyGetter(ExtensionParent, "PlatformInfo", () => {
   return Object.freeze({
     os: (function() {
--- a/toolkit/components/extensions/ExtensionXPCShellUtils.jsm
+++ b/toolkit/components/extensions/ExtensionXPCShellUtils.jsm
@@ -582,33 +582,34 @@ class AOMExtensionWrapper extends Extens
 
     return this._install(xpiFile);
   }
 }
 
 var ExtensionTestUtils = {
   BASE_MANIFEST,
 
-  async normalizeManifest(manifest, baseManifest = BASE_MANIFEST) {
+  async normalizeManifest(manifest, manifestType = "manifest.WebExtensionManifest",
+                          baseManifest = BASE_MANIFEST) {
     await Management.lazyInit();
 
     let errors = [];
     let context = {
       url: null,
 
       logError: error => {
         errors.push(error);
       },
 
       preprocessors: {},
     };
 
     manifest = Object.assign({}, baseManifest, manifest);
 
-    let normalized = Schemas.normalize(manifest, "manifest.WebExtensionManifest", context);
+    let normalized = Schemas.normalize(manifest, manifestType, context);
     normalized.errors = errors;
 
     return normalized;
   },
 
   currentScope: null,
 
   profileDir: null,
--- a/toolkit/components/extensions/schemas/theme.json
+++ b/toolkit/components/extensions/schemas/theme.json
@@ -482,21 +482,30 @@
               }
             },
             "additionalProperties": { "$ref": "UnrecognizedProperty" }
           }
         },
         "additionalProperties": { "$ref": "UnrecognizedProperty" }
       },
       {
-        "$extend": "WebExtensionManifest",
+        "id": "ThemeManifest",
+        "type": "object",
+        "description": "Contents of manifest.json for a static theme",
+        "$import": "manifest.ManifestBase",
         "properties": {
           "theme": {
+            "$ref": "ThemeType"
+          },
+          "icons": {
+            "type": "object",
             "optional": true,
-            "$ref": "ThemeType"
+            "patternProperties": {
+              "^[1-9]\\d*$": { "type": "string" }
+            }
           }
         }
       }
     ]
   },
   {
     "namespace": "theme",
     "description": "The theme API allows customizing of visual elements of the browser.",
--- a/toolkit/components/extensions/test/xpcshell/test_ext_manifest_themes.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_manifest_themes.js
@@ -4,17 +4,17 @@
 
 async function test_theme_property(property) {
   let normalized = await ExtensionTestUtils.normalizeManifest({
     "theme": {
       [property]: {
         "unrecognized_key": "unrecognized_value",
       },
     },
-  });
+  }, "manifest.ThemeManifest");
 
   let expectedWarning;
   if (property === "unrecognized_key") {
     expectedWarning = `Error processing theme.${property}`;
   } else {
     expectedWarning = `Error processing theme.${property}.unrecognized_key`;
   }
   equal(normalized.error, undefined, "Should not have an error");
deleted file mode 100644
--- a/toolkit/components/extensions/test/xpcshell/test_ext_themes_supported_properties.js
+++ /dev/null
@@ -1,70 +0,0 @@
-/* -*- Mode: indent-tabs-mode: nil; js-indent-level: 2 -*- */
-/* vim: set sts=2 sw=2 et tw=80: */
-"use strict";
-
-Cu.import("resource://gre/modules/Schemas.jsm");
-
-const {
-  validateThemeManifest,
-} = Cu.import("resource://gre/modules/Extension.jsm", {});
-
-const BASE_SCHEMA_URL = "chrome://extensions/content/schemas/manifest.json";
-const CATEGORY_EXTENSION_SCHEMAS = "webextension-schemas";
-
-const baseManifestProperties = [
-  "manifest_version",
-  "minimum_chrome_version",
-  "applications",
-  "browser_specific_settings",
-  "name",
-  "short_name",
-  "description",
-  "author",
-  "version",
-  "homepage_url",
-  "icons",
-  "incognito",
-  "background",
-  "options_ui",
-  "content_scripts",
-  "permissions",
-  "web_accessible_resources",
-  "developer",
-];
-
-async function getAdditionalInvalidManifestProperties() {
-  let invalidProperties = [];
-  await Schemas.load(BASE_SCHEMA_URL);
-  for (let [name, url] of XPCOMUtils.enumerateCategoryEntries(CATEGORY_EXTENSION_SCHEMAS)) {
-    if (name !== "theme") {
-      await Schemas.load(url);
-      let types = Schemas.schemaJSON.get(url).deserialize({})[0].types;
-      types.forEach(type => {
-        if (type.$extend == "WebExtensionManifest") {
-          let properties = Object.getOwnPropertyNames(type.properties);
-          invalidProperties.push(...properties);
-        }
-      });
-    }
-  }
-
-  // Also test an unrecognized property.
-  invalidProperties.push("unrecognized_property");
-
-  return invalidProperties;
-}
-
-function checkProperties(actual, expected) {
-  Assert.equal(actual.length, expected.length, `Should have found ${expected.length} invalid properties`);
-  for (let i = 0; i < expected.length; i++) {
-    Assert.ok(actual.includes(expected[i]), `The invalid properties should contain "${expected[i]}"`);
-  }
-}
-
-add_task(async function test_theme_supported_properties() {
-  let additionalInvalidProperties = await getAdditionalInvalidManifestProperties();
-  let actual = validateThemeManifest([...baseManifestProperties, ...additionalInvalidProperties]);
-  let expected = ["background", "permissions", "content_scripts", ...additionalInvalidProperties];
-  checkProperties(actual, expected);
-});
-
--- a/toolkit/components/extensions/test/xpcshell/xpcshell.ini
+++ b/toolkit/components/extensions/test/xpcshell/xpcshell.ini
@@ -40,17 +40,16 @@ tags = webextensions in-process-webexten
 [test_ext_manifest_minimum_opera_version.js]
 [test_ext_manifest_themes.js]
 [test_ext_schemas.js]
 [test_ext_schemas_async.js]
 [test_ext_schemas_allowed_contexts.js]
 [test_ext_schemas_interactive.js]
 [test_ext_schemas_privileged.js]
 [test_ext_schemas_revoke.js]
-[test_ext_themes_supported_properties.js]
 [test_ext_unknown_permissions.js]
 [test_locale_converter.js]
 [test_locale_data.js]
 
 [test_ext_runtime_sendMessage_args.js]
 
 [include:xpcshell-common.ini]
 [include:xpcshell-content.ini]