Bug 1337743 - Document misuse of instanceof in Marionette; r?whimboo draft
authorAndreas Tolfsen <ato@mozilla.com>
Tue, 14 Feb 2017 16:48:14 +0000
changeset 551798 183b28a910da31b81b7989b648c3345e1ddb1a90
parent 551797 7da5498a7bc846b51d24fa729b583dd5a611b2dc
child 551799 e4fff0f4e79a077f72ff8675c2ff3348adf1a162
push id51152
push userbmo:ato@mozilla.com
push dateMon, 27 Mar 2017 12:36:48 +0000
reviewerswhimboo
bugs1337743
milestone55.0a1
Bug 1337743 - Document misuse of instanceof in Marionette; r?whimboo MozReview-Commit-ID: IbUyQd0xCAI
testing/marionette/assert.js
testing/marionette/error.js
--- a/testing/marionette/assert.js
+++ b/testing/marionette/assert.js
@@ -282,16 +282,18 @@ assert.string = function (obj, msg = "")
  *     |obj| is returned unaltered.
  *
  * @throws {InvalidArgumentError}
  *     If |obj| is not an object.
  */
 assert.object = function (obj, msg = "") {
   msg = msg || error.pprint`Expected ${obj} to be an object`;
   return assert.that(o => {
+    // unable to use instanceof because LHS and RHS may come from
+    // different globals
     let s = Object.prototype.toString.call(o);
     return s == "[object Object]" || s == "[object nsJSIID]";
   })(obj);
 };
 
 /**
  * Asserts that |prop| is in |obj|.
  *
--- a/testing/marionette/error.js
+++ b/testing/marionette/error.js
@@ -43,20 +43,31 @@ const BUILTIN_ERRORS = new Set([
   "URIError",
 ]);
 
 this.EXPORTED_SYMBOLS = ["error"].concat(Array.from(ERRORS));
 
 this.error = {};
 
 /**
- * Checks if obj is an instance of the Error prototype in a safe manner.
- * Prefer using this over using instanceof since the Error prototype
- * isn't unique across browsers, and XPCOM nsIException's are special
- * snowflakes.
+ * Check if |val| is an instance of the |Error| prototype.
+ *
+ * Because error objects may originate from different globals, comparing
+ * the prototype of the left hand side with the prototype property from
+ * the right hand side, which is what |instanceof| does, will not work.
+ * If the LHS and RHS come from different globals, this check will always
+ * fail because the two objects will not have the same identity.
+ *
+ * Therefore it is not safe to use |instanceof| in any multi-global
+ * situation, e.g. in content across multiple Window objects or anywhere
+ * in chrome scope.
+ *
+ * This function also contains a special check if |val| is an XPCOM
+ * |nsIException| because they are special snowflakes and may indeed
+ * cause Firefox to crash if used with |instanceof|.
  *
  * @param {*} val
  *     Any value that should be undergo the test for errorness.
  * @return {boolean}
  *     True if error, false otherwise.
  */
 error.isError = function (val) {
   if (val === null || typeof val != "object") {