Bug 1337743 - Document misuse of instanceof in Marionette; r=whimboo, a=test-only
authorAndreas Tolfsen <ato@mozilla.com>
Tue, 14 Feb 2017 16:48:14 +0000
changeset 395554 2d530eb53257031b66189f78cb85c78cb1683f1e
parent 395553 04e6b63a88126a3130ebee1c4dd1b20c734ae066
child 395555 e31ace8cbb9eca44c361286d21cef126dd7ae86f
push id1468
push userasasaki@mozilla.com
push dateMon, 05 Jun 2017 19:31:07 +0000
treeherdermozilla-release@0641fc6ee9d1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerswhimboo, test-only
bugs1337743
milestone54.0a2
Bug 1337743 - Document misuse of instanceof in Marionette; r=whimboo, a=test-only 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") {