Bug 1479445: Update the validation of PageInfo to use validateItemProperties r=mak,Standard8
☠☠ backed out by 6a531a9eb798 ☠ ☠
authorSiddhant085 <dpsrkp.sid@gmail.com>
Tue, 25 Sep 2018 18:21:56 +0000
changeset 438164 d4c858c70bc9870a88d841ad933b9ea74fe547bc
parent 438163 951f04d1bb518e722e13b2a15580921590eb2b44
child 438165 da42d772b73c44b162044348792c5b9e0c658cd4
push id69919
push usermbanner@mozilla.com
push dateTue, 25 Sep 2018 18:22:35 +0000
treeherderautoland@d4c858c70bc9 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersmak, Standard8
bugs1479445
milestone64.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 1479445: Update the validation of PageInfo to use validateItemProperties r=mak,Standard8 Changed the validation function for PageInfo to use a more general validateItemProperties. This changes the error message being thrown. Changed the respective test cases to accomodate the change. Differential Revision: https://phabricator.services.mozilla.com/D5831
browser/components/extensions/test/xpcshell/test_ext_history.js
toolkit/components/places/PlacesUtils.jsm
toolkit/components/places/tests/history/test_insert.js
toolkit/components/places/tests/history/test_insertMany.js
toolkit/components/places/tests/history/test_update.js
--- a/browser/components/extensions/test/xpcshell/test_ext_history.js
+++ b/browser/components/extensions/test/xpcshell/test_ext_history.js
@@ -293,18 +293,18 @@ add_task(async function test_add_url() {
     [{visitTime: new Date()}, "with_date"],
     [{visitTime: Date.now()}, "with_ms_number"],
     [{visitTime: new Date().toISOString()}, "with_iso_string"],
     [{transition: "typed"}, "valid_transition"],
   ];
 
   let failTestData = [
     [{transition: "generated"}, "an invalid transition", "|generated| is not a supported transition for history"],
-    [{visitTime: Date.now() + 1000000}, "a future date", "cannot be a future date"],
-    [{url: "about.config"}, "an invalid url", "about.config is not a valid URL"],
+    [{visitTime: Date.now() + 1000000}, "a future date", "Invalid value"],
+    [{url: "about.config"}, "an invalid url", "Invalid value"],
   ];
 
   async function checkUrl(results) {
     ok((await PlacesTestUtils.isPageInDB(results.details.url)), `${results.details.url} found in history database`);
     ok(PlacesUtils.isValidGuid(results.result.id), "URL was added with a valid id");
     equal(results.result.title, results.details.title, "URL was added with the correct title");
     if (results.details.visitTime) {
       equal(results.result.lastVisitTime,
--- a/toolkit/components/places/PlacesUtils.jsm
+++ b/toolkit/components/places/PlacesUtils.jsm
@@ -299,16 +299,98 @@ const SYNC_BOOKMARK_VALIDATORS = Object.
 const SYNC_CHANGE_RECORD_VALIDATORS = Object.freeze({
   modified: simpleValidateFunc(v => typeof v == "number" && v >= 0),
   counter: simpleValidateFunc(v => typeof v == "number" && v >= 0),
   status: simpleValidateFunc(v => typeof v == "number" &&
                                   Object.values(PlacesUtils.bookmarks.SYNC_STATUS).includes(v)),
   tombstone: simpleValidateFunc(v => v === true || v === false),
   synced: simpleValidateFunc(v => v === true || v === false),
 });
+/**
+ * List PageInfo bookmark object validators.
+ */
+const PAGEINFO_VALIDATORS = Object.freeze({
+  guid: BOOKMARK_VALIDATORS.guid,
+  url: BOOKMARK_VALIDATORS.url,
+  title: v => {
+    if (v == null || v == undefined) {
+      return undefined;
+    } else if (typeof v === "string") {
+      return v;
+    }
+    throw new TypeError(`title property of PageInfo object: ${v} must be a string if provided`);
+  },
+  previewImageURL: v => {
+    if (!v) {
+      return null;
+    }
+    return BOOKMARK_VALIDATORS.url(v);
+  },
+  description: v => {
+    if (typeof v === "string" || v === null) {
+      return v ? v.slice(0, DB_DESCRIPTION_LENGTH_MAX) : null;
+    }
+    throw new TypeError(`description property of pageInfo object: ${v} must be either a string or null if provided`);
+  },
+  annotations: v => {
+    if (typeof v != "object" ||
+        v.constructor.name != "Map") {
+        throw new TypeError("annotations must be a Map");
+      }
+
+      if (v.size == 0) {
+        throw new TypeError("there must be at least one annotation");
+      }
+
+      for (let [key, value] of v.entries()) {
+        if (typeof key != "string") {
+          throw new TypeError("all annotation keys must be strings");
+        }
+        if (typeof value != "string" &&
+            typeof value != "number" &&
+            typeof value != "boolean" &&
+            value !== null &&
+            value !== undefined) {
+          throw new TypeError("all annotation values must be Boolean, Numbers or Strings");
+        }
+      }
+      return v;
+  },
+  visits: v => {
+    if (!Array.isArray(v) || !v.length) {
+      throw new TypeError("PageInfo object must have an array of visits");
+    }
+    let visits = [];
+    for (let inVisit of v) {
+      let visit = {
+        date: new Date(),
+        transition: inVisit.transition || History.TRANSITIONS.LINK,
+      };
+
+      if (!PlacesUtils.history.isValidTransition(visit.transition)) {
+        throw new TypeError(`transition: ${visit.transition} is not a valid transition type`);
+      }
+
+      if (inVisit.date) {
+        PlacesUtils.history.ensureDate(inVisit.date);
+        if (inVisit.date > (Date.now() + TIMERS_RESOLUTION_SKEW_MS)) {
+          throw new TypeError(`date: ${inVisit.date} cannot be a future date`);
+        }
+        visit.date = inVisit.date;
+      }
+
+      if (inVisit.referrer) {
+        visit.referrer = PlacesUtils.normalizeToURLOrGUID(inVisit.referrer);
+      }
+      visits.push(visit);
+    }
+    return visits;
+  },
+});
+
 
 var PlacesUtils = {
   // Place entries that are containers, e.g. bookmark folders or queries.
   TYPE_X_MOZ_PLACE_CONTAINER: "text/x-moz-place-container",
   // Place entries that are bookmark separators.
   TYPE_X_MOZ_PLACE_SEPARATOR: "text/x-moz-place-separator",
   // Place entries that are not containers or separators
   TYPE_X_MOZ_PLACE: "text/x-moz-place",
@@ -628,17 +710,17 @@ var PlacesUtils = {
    *         - fixup: a function invoked when validation fails, takes the input
    *                  object as argument and must fix the property.
    *
    * @return a validated and normalized item.
    * @throws if the object contains invalid data.
    * @note any unknown properties are pass-through.
    */
   validateItemProperties(name, validators, props, behavior = {}) {
-    if (!props)
+    if (typeof props != "object" || !props)
       throw new Error(`${name}: Input should be a valid object`);
     // Make a shallow copy of `props` to avoid mutating the original object
     // when filling in defaults.
     let input = Object.assign({}, props);
     let normalizedInput = {};
     let required = new Set();
     for (let prop in behavior) {
       if (behavior[prop].hasOwnProperty("required") && behavior[prop].required) {
@@ -1031,124 +1113,22 @@ var PlacesUtils = {
 
   /**
    * Validate an input PageInfo object, returning a valid PageInfo object.
    *
    * @param pageInfo: (PageInfo)
    * @return (PageInfo)
    */
   validatePageInfo(pageInfo, validateVisits = true) {
-    let info = {
-      visits: [],
-    };
-
-    if (typeof pageInfo != "object" || !pageInfo) {
-      throw new TypeError("pageInfo must be an object");
-    }
-
-    if (!pageInfo.url) {
-      throw new TypeError("PageInfo object must have a url property");
-    }
-
-    info.url = this.normalizeToURLOrGUID(pageInfo.url);
-
-    if (typeof pageInfo.guid === "string" && this.isValidGuid(pageInfo.guid)) {
-      info.guid = pageInfo.guid;
-    } else if (pageInfo.guid) {
-      throw new TypeError(`guid property of PageInfo object: ${pageInfo.guid} is invalid`);
-    }
-
-    if (typeof pageInfo.title === "string") {
-      info.title = pageInfo.title;
-    } else if (pageInfo.title != null && pageInfo.title != undefined) {
-      throw new TypeError(`title property of PageInfo object: ${pageInfo.title} must be a string if provided`);
-    }
-
-    if ("description" in pageInfo && (typeof pageInfo.description === "string" || pageInfo.description === null)) {
-      info.description = pageInfo.description ? pageInfo.description.slice(0, DB_DESCRIPTION_LENGTH_MAX) : null;
-    } else if (pageInfo.description !== undefined) {
-      throw new TypeError(`description property of pageInfo object: ${pageInfo.description} must be either a string or null if provided`);
-    }
-
-    if ("previewImageURL" in pageInfo) {
-      let previewImageURL = pageInfo.previewImageURL;
-
-      if (!previewImageURL) {
-        info.previewImageURL = null;
-      } else if (typeof(previewImageURL) === "string" && previewImageURL.length <= DB_URL_LENGTH_MAX) {
-        info.previewImageURL = new URL(previewImageURL);
-      } else if (previewImageURL instanceof Ci.nsIURI && previewImageURL.spec.length <= DB_URL_LENGTH_MAX) {
-        info.previewImageURL = new URL(previewImageURL.spec);
-      } else if (previewImageURL instanceof URL && previewImageURL.href.length <= DB_URL_LENGTH_MAX) {
-        info.previewImageURL = previewImageURL;
-      } else {
-        throw new TypeError("previewImageURL property of pageInfo object: ${previewImageURL} is invalid");
-      }
-    }
-
-    if (pageInfo.annotations) {
-      if (typeof pageInfo.annotations != "object" ||
-          pageInfo.annotations.constructor.name != "Map") {
-        throw new TypeError("annotations must be a Map");
-      }
-
-      if (pageInfo.annotations.size == 0) {
-        throw new TypeError("there must be at least one annotation");
-      }
-
-      for (let [key, value] of pageInfo.annotations.entries()) {
-        if (typeof key != "string") {
-          throw new TypeError("all annotation keys must be strings");
-        }
-        if (typeof value != "string" &&
-            typeof value != "number" &&
-            typeof value != "boolean" &&
-            value !== null &&
-            value !== undefined) {
-          throw new TypeError("all annotation values must be Boolean, Numbers or Strings");
-        }
-      }
-
-      info.annotations = pageInfo.annotations;
-    }
-
-    if (!validateVisits) {
-      return info;
-    }
-
-    if (!pageInfo.visits || !Array.isArray(pageInfo.visits) || !pageInfo.visits.length) {
-      throw new TypeError("PageInfo object must have an array of visits");
-    }
-
-    for (let inVisit of pageInfo.visits) {
-      let visit = {
-        date: new Date(),
-        transition: inVisit.transition || History.TRANSITIONS.LINK,
-      };
-
-      if (!PlacesUtils.history.isValidTransition(visit.transition)) {
-        throw new TypeError(`transition: ${visit.transition} is not a valid transition type`);
-      }
-
-      if (inVisit.date) {
-        PlacesUtils.history.ensureDate(inVisit.date);
-        if (inVisit.date > (Date.now() + TIMERS_RESOLUTION_SKEW_MS)) {
-          throw new TypeError(`date: ${inVisit.date} cannot be a future date`);
-        }
-        visit.date = inVisit.date;
-      }
-
-      if (inVisit.referrer) {
-        visit.referrer = this.normalizeToURLOrGUID(inVisit.referrer);
-      }
-      info.visits.push(visit);
-    }
-    return info;
+    return this.validateItemProperties("PageInfo", PAGEINFO_VALIDATORS, pageInfo,
+      { url: { requiredIf: b => { typeof b.guid != "string"; } },
+        guid: { requiredIf: b => { typeof b.url != "string"; } },
+        visits: { requiredIf: b => validateVisits  },
+      });
   },
-
   /**
    * Normalize a key to either a string (if it is a valid GUID) or an
    * instance of `URL` (if it is a `URL`, `nsIURI`, or a string
    * representing a valid url).
    *
    * @throws (TypeError)
    *         If the key is neither a valid guid nor a valid url.
    */
--- a/toolkit/components/places/tests/history/test_insert.js
+++ b/toolkit/components/places/tests/history/test_insert.js
@@ -5,98 +5,98 @@
 
 "use strict";
 
 add_task(async function test_insert_error_cases() {
   const TEST_URL = "http://mozilla.com";
 
   Assert.throws(
     () => PlacesUtils.history.insert(),
-    /TypeError: pageInfo must be an object/,
-    "passing a null into History.insert should throw a TypeError"
+    /Error: PageInfo: Input should be /,
+    "passing a null into History.insert should throw an Error"
   );
   Assert.throws(
     () => PlacesUtils.history.insert(1),
-    /TypeError: pageInfo must be an object/,
-    "passing a non object into History.insert should throw a TypeError"
+    /Error: PageInfo: Input should be/,
+    "passing a non object into History.insert should throw an Error"
   );
   Assert.throws(
     () => PlacesUtils.history.insert({}),
-    /TypeError: PageInfo object must have a url property/,
-    "passing an object without a url to History.insert should throw a TypeError"
+    /Error: PageInfo: The following properties were expected/,
+    "passing an object without a url to History.insert should throw an Error"
   );
   Assert.throws(
     () => PlacesUtils.history.insert({url: 123}),
-    /TypeError: Invalid url or guid: 123/,
-    "passing an object with an invalid url to History.insert should throw a TypeError"
+    /Error: PageInfo: Invalid value for property/,
+    "passing an object with an invalid url to History.insert should throw an Error"
   );
   Assert.throws(
     () => PlacesUtils.history.insert({url: TEST_URL}),
-    /TypeError: PageInfo object must have an array of visits/,
-    "passing an object without a visits property to History.insert should throw a TypeError"
+    /Error: PageInfo: The following properties were expected/,
+    "passing an object without a visits property to History.insert should throw an Error"
   );
   Assert.throws(
     () => PlacesUtils.history.insert({url: TEST_URL, visits: 1}),
-    /TypeError: PageInfo object must have an array of visits/,
-    "passing an object with a non-array visits property to History.insert should throw a TypeError"
+    /Error: PageInfo: Invalid value for property/,
+    "passing an object with a non-array visits property to History.insert should throw an Error"
   );
   Assert.throws(
     () => PlacesUtils.history.insert({url: TEST_URL, visits: []}),
-    /TypeError: PageInfo object must have an array of visits/,
-    "passing an object with an empty array as the visits property to History.insert should throw a TypeError"
+    /Error: PageInfo: Invalid value for property/,
+    "passing an object with an empty array as the visits property to History.insert should throw an Error"
   );
   Assert.throws(
     () => PlacesUtils.history.insert({
       url: TEST_URL,
       visits: [
         {
           transition: TRANSITION_LINK,
           date: "a",
         },
       ]}),
-    /TypeError: Expected a Date, got a/,
-    "passing a visit object with an invalid date to History.insert should throw a TypeError"
+    /PageInfo: Invalid value for property/,
+    "passing a visit object with an invalid date to History.insert should throw an Error"
   );
   Assert.throws(
     () => PlacesUtils.history.insert({
       url: TEST_URL,
       visits: [
         {
           transition: TRANSITION_LINK,
         },
         {
           transition: TRANSITION_LINK,
           date: "a",
         },
       ]}),
-    /TypeError: Expected a Date, got a/,
-    "passing a second visit object with an invalid date to History.insert should throw a TypeError"
+    /PageInfo: Invalid value for property/,
+    "passing a second visit object with an invalid date to History.insert should throw an Error"
   );
   let futureDate = new Date();
   futureDate.setDate(futureDate.getDate() + 1000);
   Assert.throws(
     () => PlacesUtils.history.insert({
       url: TEST_URL,
       visits: [
         {
           transition: TRANSITION_LINK,
           date: futureDate,
         },
       ]}),
-    /cannot be a future date/,
-    "passing a visit object with a future date to History.insert should throw a TypeError"
+    /PageInfo: Invalid value for property/,
+    "passing a visit object with a future date to History.insert should throw an Error"
   );
   Assert.throws(
     () => PlacesUtils.history.insert({
       url: TEST_URL,
       visits: [
         {transition: "a"},
       ]}),
-    /TypeError: transition: a is not a valid transition type/,
-    "passing a visit object with an invalid transition to History.insert should throw a TypeError"
+    /PageInfo: Invalid value for property/,
+    "passing a visit object with an invalid transition to History.insert should throw an Error"
   );
 });
 
 add_task(async function test_history_insert() {
   const TEST_URL = "http://mozilla.com/";
 
   let inserter = async function(name, filter, referrer, date, transition) {
     info(name);
--- a/toolkit/components/places/tests/history/test_insertMany.js
+++ b/toolkit/components/places/tests/history/test_insertMany.js
@@ -20,18 +20,18 @@ add_task(async function test_error_cases
   );
   Assert.throws(
     () => PlacesUtils.history.insertMany([]),
     /TypeError: pageInfos may not be an empty array/,
     "passing an empty array into History.insertMany should throw a TypeError"
   );
   Assert.throws(
     () => PlacesUtils.history.insertMany([validPageInfo, {}]),
-    /TypeError: PageInfo object must have a url property/,
-    "passing a second invalid PageInfo object to History.insertMany should throw a TypeError"
+    /Error: PageInfo: The following properties were expected/,
+    "passing a second invalid PageInfo object to History.insertMany should throw an Error"
   );
 });
 
 add_task(async function test_insertMany() {
   const BAD_URLS = ["about:config", "chrome://browser/content/browser.xul"];
   const GOOD_URLS = [1, 2, 3].map(x => { return `http://mozilla.com/${x}`; });
 
   let makePageInfos = async function(urls, filter = x => x) {
--- a/toolkit/components/places/tests/history/test_update.js
+++ b/toolkit/components/places/tests/history/test_update.js
@@ -5,104 +5,104 @@
 
 // Tests for `History.update` as implemented in History.jsm
 
 "use strict";
 
 add_task(async function test_error_cases() {
   Assert.throws(
     () => PlacesUtils.history.update("not an object"),
-    /TypeError: pageInfo must be/,
-    "passing a string as pageInfo should throw a TypeError"
+    /Error: PageInfo: Input should be a valid object/,
+    "passing a string as pageInfo should throw an Error"
   );
   Assert.throws(
     () => PlacesUtils.history.update(null),
-    /TypeError: pageInfo must be/,
-    "passing a null as pageInfo should throw a TypeError"
+    /Error: PageInfo: Input should be/,
+    "passing a null as pageInfo should throw an Error"
   );
   Assert.throws(
     () => PlacesUtils.history.update({url: "not a valid url string"}),
-    /TypeError: not a valid url string is/,
-    "passing an invalid url should throw a TypeError"
+    /Error: PageInfo: Invalid value for property/,
+    "passing an invalid url should throw an Error"
   );
   Assert.throws(
     () => PlacesUtils.history.update({
       url: "http://valid.uri.com",
       description: 123,
     }),
-    /TypeError: description property of/,
-    "passing a non-string description in pageInfo should throw a TypeError"
+    /Error: PageInfo: Invalid value for property/,
+    "passing a non-string description in pageInfo should throw an Error"
   );
   Assert.throws(
     () => PlacesUtils.history.update({
       url: "http://valid.uri.com",
       guid: "invalid guid",
       description: "Test description",
     }),
-    /TypeError: guid property of/,
-    "passing a invalid guid in pageInfo should throw a TypeError"
+    /Error: PageInfo: Invalid value for property/,
+    "passing a invalid guid in pageInfo should throw an Error"
   );
   Assert.throws(
     () => PlacesUtils.history.update({
       url: "http://valid.uri.com",
       previewImageURL: "not a valid url string",
     }),
-    /TypeError: not a valid url string is/,
-    "passing an invlid preview image url in pageInfo should throw a TypeError"
+    /Error: PageInfo: Invalid value for property/,
+    "passing an invlid preview image url in pageInfo should throw an Error"
   );
   Assert.throws(
     () => {
       let imageName = "a-very-long-string".repeat(10000);
       let previewImageURL = `http://valid.uri.com/${imageName}.png`;
       PlacesUtils.history.update({
         url: "http://valid.uri.com",
         previewImageURL,
       });
     },
-    /TypeError: previewImageURL property of/,
-    "passing an oversized previewImageURL in pageInfo should throw a TypeError"
+    /Error: PageInfo: Invalid value for property/,
+    "passing an oversized previewImageURL in pageInfo should throw an Error"
   );
   Assert.throws(
     () => PlacesUtils.history.update({ url: "http://valid.uri.com" }),
     /TypeError: pageInfo object must at least/,
     "passing a pageInfo with neither description, previewImageURL, nor annotations should throw a TypeError"
   );
   Assert.throws(
     () => PlacesUtils.history.update({ url: "http://valid.uri.com", annotations: "asd" }),
-    /TypeError: annotations must be a Map/,
-    "passing a pageInfo with incorrect annotations type should throw a TypeError"
+    /Error: PageInfo: Invalid value for property/,
+    "passing a pageInfo with incorrect annotations type should throw an Error"
   );
   Assert.throws(
     () => PlacesUtils.history.update({ url: "http://valid.uri.com", annotations: new Map() }),
-    /TypeError: there must be at least one annotation/,
-    "passing a pageInfo with an empty annotations type should throw a TypeError"
+    /Error: PageInfo: Invalid value for property/,
+    "passing a pageInfo with an empty annotations type should throw an Error"
   );
   Assert.throws(
     () => PlacesUtils.history.update({
       url: "http://valid.uri.com",
       annotations: new Map([[1234, "value"]]),
     }),
-    /TypeError: all annotation keys must be strings/,
-    "passing a pageInfo with an invalid key type should throw a TypeError"
+    /Error: PageInfo: Invalid value for property/,
+    "passing a pageInfo with an invalid key type should throw an Error"
   );
   Assert.throws(
     () => PlacesUtils.history.update({
       url: "http://valid.uri.com",
       annotations: new Map([["test", ["myarray"]]]),
     }),
-    /TypeError: all annotation values must be Boolean, Numbers or Strings/,
-    "passing a pageInfo with an invalid key type should throw a TypeError"
+    /Error: PageInfo: Invalid value for property/,
+    "passing a pageInfo with an invalid key type should throw an Error"
   );
   Assert.throws(
     () => PlacesUtils.history.update({
       url: "http://valid.uri.com",
       annotations: new Map([["test", {anno: "value"}]]),
     }),
-    /TypeError: all annotation values must be Boolean, Numbers or Strings/,
-    "passing a pageInfo with an invalid key type should throw a TypeError"
+    /Error: PageInfo: Invalid value for property/,
+    "passing a pageInfo with an invalid key type should throw an Error"
   );
 });
 
 add_task(async function test_description_change_saved() {
   await PlacesUtils.history.clear();
 
   let TEST_URL = "http://mozilla.org/test_description_change_saved";
   await PlacesTestUtils.addVisits(TEST_URL);