Bug 1793925 - Warn about complex versioning formats used in manifest. r=robwu

Differential Revision: https://phabricator.services.mozilla.com/D158834

browser/components/extensions/test/xpcshell/test_ext_manifest.js

@@ -121,3 +121,78 @@ add_task(async function test_mv3_scripting_permission_always_enabled() {
 
   Assert.deepEqual(warnings, [], "Got no warnings");
 });
+
+add_task(async function test_simpler_version_format() {
+  const TEST_CASES = [
+    // Valid cases
+    { version: "0", expectWarning: false },
+    { version: "0.0", expectWarning: false },
+    { version: "0.0.0", expectWarning: false },
+    { version: "0.0.0.0", expectWarning: false },
+    { version: "0.0.0.1", expectWarning: false },
+    { version: "0.0.0.999999999", expectWarning: false },
+    { version: "0.0.1.0", expectWarning: false },
+    { version: "0.0.999999999", expectWarning: false },
+    { version: "0.1.0.0", expectWarning: false },
+    { version: "0.999999999", expectWarning: false },
+    { version: "1", expectWarning: false },
+    { version: "1.0", expectWarning: false },
+    { version: "1.0.0", expectWarning: false },
+    { version: "1.0.0.0", expectWarning: false },
+    { version: "1.2.3.4", expectWarning: false },
+    { version: "999999999", expectWarning: false },
+    {
+      version: "999999999.999999999.999999999.999999999",
+      expectWarning: false,
+    },
+    // Invalid cases
+    { version: ".", expectWarning: true },
+    { version: ".999999999", expectWarning: true },
+    { version: "0.0.0.0.0", expectWarning: true },
+    { version: "0.0.0.00001", expectWarning: true },
+    { version: "0.0.0.0010", expectWarning: true },
+    { version: "0.0.00001", expectWarning: true },
+    { version: "0.0.001", expectWarning: true },
+    { version: "0.0.01.0", expectWarning: true },
+    { version: "0.01.0", expectWarning: true },
+    { version: "00001", expectWarning: true },
+    { version: "0001", expectWarning: true },
+    { version: "001", expectWarning: true },
+    { version: "01", expectWarning: true },
+    { version: "01.0", expectWarning: true },
+    { version: "099999", expectWarning: true },
+    { version: "0999999999", expectWarning: true },
+    { version: "1.00000", expectWarning: true },
+    { version: "1.1.-1", expectWarning: true },
+    { version: "1.1000000000", expectWarning: true },
+    { version: "1.1pre1aa", expectWarning: true },
+    { version: "1.2.1000000000", expectWarning: true },
+    { version: "1.2.3.4-a", expectWarning: true },
+    { version: "1.2.3.4.5", expectWarning: true },
+    { version: "1000000000", expectWarning: true },
+    { version: "1000000000.0.0.0", expectWarning: true },
+    { version: "999999999.", expectWarning: true },
+  ];
+
+  for (const { version, expectWarning } of TEST_CASES) {
+    const normalized = await ExtensionTestUtils.normalizeManifest({ version });
+
+    if (expectWarning) {
+      Assert.deepEqual(
+        normalized.errors,
+        [
+          `Warning processing version: version must be a version string ` +
+            `consisting of at most 4 integers of at most 9 digits without ` +
+            `leading zeros, and separated with dots`,
+        ],
+        `expected warning for version: ${version}`
+      );
+    } else {
+      Assert.deepEqual(
+        normalized.errors,
+        [],
+        `expected no warning for version: ${version}`
+      );
+    }
+  }
+});

browser/components/extensions/test/xpcshell/xpcshell.ini

@@ -17,6 +17,7 @@ skip-if = tsan # Times out, bug 1612707
 [test_ext_history.js]
 [test_ext_homepage_overrides_private.js]
 [test_ext_manifest.js]
+skip-if = condprof # Bug 1795285
 [test_ext_manifest_commands.js]
 run-sequentially = very high failure rate in parallel
 [test_ext_manifest_omnibox.js]

testing/profiles/profiles.json

@@ -5,7 +5,7 @@
     "raptor": ["base", "common", "perf", "raptor"],
     "reftest": ["base", "common", "reftest"],
     "talos": ["base", "common", "perf"],
-    "valgrind": ["base", "common", "unittest-required", "unittest-features"],
+    "valgrind": ["base", "common", "unittest-required", "unittest-features", "valgrind"],
     "xpcshell": ["base", "xpcshell"],
     "web-platform-tests": ["base", "common", "unittest-required", "unittest-features", "web-platform"]
 }

testing/profiles/profileserver/user.js

@@ -6,3 +6,6 @@
 /* globals user_pref */
 // Turn off budget throttling for the profile server
 user_pref("dom.timeout.enable_budget_timer_throttling", false);
+// TODO: Bug 1795750 - Re-enable this pref when we have a new version of the
+// Quitter XPI with a simpler version format.
+user_pref("extensions.webextensions.warnings-as-errors", false);

testing/profiles/valgrind/user.js

@@ -0,0 +1,9 @@
+/* This Source Code Form is subject to the terms of the Mozilla Public
+ * License, v. 2.0. If a copy of the MPL was not distributed with this
+ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
+
+// Preferences file for valgrind.
+/* globals user_pref */
+// TODO: Bug 1795750 - Re-enable this pref when we have a new version of the
+// Quitter XPI with a simpler version format.
+user_pref("extensions.webextensions.warnings-as-errors", false);

testing/specialpowers/manifest.json

@@ -1,7 +1,7 @@
 {
   "manifest_version": 2,
   "name": "Special Powers",
-  "version": "2018.06.27",
+  "version": "2018.6.27",
 
   "applications": {
     "gecko": {

toolkit/components/extensions/Schemas.jsm

@@ -574,6 +574,31 @@ class Context {
     }
   }
 
+  /**
+   * Logs a warning. An error might be thrown when we treat warnings as errors.
+   *
+   * @param {string} warningMessage
+   */
+  logWarning(warningMessage) {
+    let error = this.makeError(warningMessage, { warning: true });
+    this.logError(error);
+
+    if (lazy.treatWarningsAsErrors) {
+      // This pref is false by default, and true by default in tests to
+      // discourage the use of deprecated APIs in our unit tests.
+      // If a warning is an expected part of a test, temporarily set the pref
+      // to false, e.g. with the ExtensionTestUtils.failOnSchemaWarnings helper.
+      Services.console.logStringMessage(
+        "Treating warning as error because the preference " +
+          "extensions.webextensions.warnings-as-errors is set to true"
+      );
+      if (typeof error === "string") {
+        error = new Error(error);
+      }
+      throw error;
+    }
+  }
+
   /**
    * Returns the name of the value currently being normalized. For a
    * nested object, this is usually approximately equivalent to the
@@ -1200,6 +1225,27 @@ const FORMATS = {
   manifestShortcutKeyOrEmpty(string, context) {
     return string === "" ? "" : FORMATS.manifestShortcutKey(string, context);
   },
+
+  versionString(string, context) {
+    const parts = string.split(".");
+
+    if (
+      // We accept up to 4 numbers.
+      parts.length > 4 ||
+      // Non-zero values cannot start with 0 and we allow numbers up to 9 digits.
+      parts.some(part => !/^(0|[1-9][0-9]{0,8})$/.test(part))
+    ) {
+      context.logWarning(
+        `version must be a version string consisting of at most 4 integers ` +
+          `of at most 9 digits without leading zeros, and separated with dots`
+      );
+    }
+
+    // The idea is to only emit a warning when the version string does not
+    // match the simple format we want to encourage developers to use. Given
+    // the version is required, we always accept the value as is.
+    return string;
+  },
 };
 
 // Schema files contain namespaces, and each namespace contains types,
@@ -1310,31 +1356,7 @@ class Entry {
       }
     }
 
-    this.logWarning(context, message);
-  }
-
-  /**
-   * @param {Context} context
-   * @param {string} warningMessage
-   */
-  logWarning(context, warningMessage) {
-    let error = context.makeError(warningMessage, { warning: true });
-    context.logError(error);
-
-    if (lazy.treatWarningsAsErrors) {
-      // This pref is false by default, and true by default in tests to
-      // discourage the use of deprecated APIs in our unit tests.
-      // If a warning is an expected part of a test, temporarily set the pref
-      // to false, e.g. with the ExtensionTestUtils.failOnSchemaWarnings helper.
-      Services.console.logStringMessage(
-        "Treating warning as error because the preference " +
-          "extensions.webextensions.warnings-as-errors is set to true"
-      );
-      if (typeof error === "string") {
-        error = new Error(error);
-      }
-      throw error;
-    }
+    context.logWarning(message);
   }
 
   /**
@@ -2023,7 +2045,7 @@ class ObjectType extends Type {
           `not contain an unsupported "${prop}" property`
         );
 
-        this.logWarning(context, forceString(error.error));
+        context.logWarning(forceString(error.error));
         if (this.additionalProperties) {
           // When `additionalProperties` is set to UnrecognizedProperty, the
           // caller (i.e. ObjectType's normalize method) assigns the original
@@ -2074,7 +2096,7 @@ class ObjectType extends Type {
 
     if (error) {
       if (onError == "warn") {
-        this.logWarning(context, forceString(error.error));
+        context.logWarning(forceString(error.error));
       } else if (onError != "ignore") {
         throw error;
       }
@@ -2375,7 +2397,7 @@ class ArrayType extends Type {
       );
       if (element.error) {
         if (this.onError == "warn") {
-          this.logWarning(context, forceString(element.error));
+          context.logWarning(forceString(element.error));
         } else if (this.onError != "ignore") {
           return element;
         }

toolkit/components/extensions/schemas/manifest.json

@@ -53,7 +53,8 @@
 
           "version": {
             "type": "string",
-            "optional": false
+            "optional": false,
+            "format": "versionString"
           },
 
           "homepage_url": {

toolkit/mozapps/extensions/test/xpcshell/test_sideload_scopes.js

@@ -22,6 +22,14 @@
 // We expect new sideloads to fail elsewhere.
 // We expect to be able to change/uninstall legacy sideloads.
 
+// This test uses add-on versions that follow the toolkit version but we
+// started to encourage the use of a simpler format in Bug 1793925. We disable
+// the pref below to avoid install errors.
+Services.prefs.setBoolPref(
+  "extensions.webextensions.warnings-as-errors",
+  false
+);
+
 // IDs for scopes that should sideload when sideloading
 // is not disabled.
 let legacyIDs = [

toolkit/mozapps/extensions/test/xpcshell/test_sideloads_after_rebuild.js

@@ -3,6 +3,14 @@
 
 "use strict";
 
+// This test uses add-on versions that follow the toolkit version but we
+// started to encourage the use of a simpler format in Bug 1793925. We disable
+// the pref below to avoid install errors.
+Services.prefs.setBoolPref(
+  "extensions.webextensions.warnings-as-errors",
+  false
+);
+
 // IDs for scopes that should sideload when sideloading
 // is not disabled.
 let legacyIDs = [

toolkit/mozapps/extensions/test/xpcshell/test_update.js

@@ -6,6 +6,13 @@
 
 // The test extension uses an insecure update url.
 Services.prefs.setBoolPref(PREF_EM_CHECK_UPDATE_SECURITY, false);
+// This test uses add-on versions that follow the toolkit version but we
+// started to encourage the use of a simpler format in Bug 1793925. We disable
+// the pref below to avoid install errors.
+Services.prefs.setBoolPref(
+  "extensions.webextensions.warnings-as-errors",
+  false
+);
 
 const updateFile = "test_update.json";
 

toolkit/mozapps/extensions/test/xpcshell/test_updateversion.js

@@ -45,8 +45,8 @@ add_task(async function test_update_version_mismatch() {
 
   await serverRegisterUpdate({
     id: ID,
-    version: "2.00",
-    actualVersion: "2.000",
+    version: "2.0",
+    actualVersion: "2.0.0",
   });
 
   let addon = await promiseAddonByID(ID);
@@ -59,7 +59,7 @@ add_task(async function test_update_version_mismatch() {
   );
   let install = update.updateAvailable;
   Assert.notEqual(install, false, "Found available update");
-  Assert.equal(install.version, "2.00");
+  Assert.equal(install.version, "2.0");
   Assert.equal(install.state, AddonManager.STATE_AVAILABLE);
   Assert.equal(install.existingAddon, addon);