Bug 1488417 - Even better error message on property access on undefined/null variable. r=Waldo
authorJason Orendorff <jorendorff@mozilla.com>
Wed, 17 Oct 2018 11:01:43 +0300
changeset 499683 0413c9c3d902bf4653569bd4c0e7e6ba1e239ba8
parent 499682 35c61888a49d69506cdd330b81885838ccf45f8c
child 499684 5f2b0964547892553bc6af517074c87c19bd019d
push id1864
push userffxbld-merge
push dateMon, 03 Dec 2018 15:51:40 +0000
treeherdermozilla-release@f040763d99ad [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersWaldo
bugs1488417
milestone64.0
Bug 1488417 - Even better error message on property access on undefined/null variable. r=Waldo Differential Revision: https://phabricator.services.mozilla.com/D5711
dom/browser-element/mochitest/browserElement_ExecuteScript.js
js/src/jit-test/tests/basic/bug827104.js
js/src/jit-test/tests/basic/expression-autopsy.js
js/src/jit-test/tests/basic/iterable-error-messages.js
js/src/jit-test/tests/basic/testBug604210.js
js/src/jit-test/tests/debug/bug1275001.js
js/src/jit-test/tests/ion/bug913749.js
js/src/js.msg
js/src/jsapi-tests/testErrorInterceptor.cpp
js/src/tests/non262/extensions/regress-353116.js
js/src/tests/non262/regress/regress-469625-03.js
js/src/tests/non262/regress/regress-469758.js
js/src/vm/JSContext.cpp
toolkit/components/extensions/test/xpcshell/test_ext_contentscript_create_iframe.js
--- a/dom/browser-element/mochitest/browserElement_ExecuteScript.js
+++ b/dom/browser-element/mochitest/browserElement_ExecuteScript.js
@@ -86,17 +86,17 @@ function runTest() {
       `, {url});
     }).then(bail, (error) => {
       is(error.message, 'Value returned (resolve) by promise is not a valid JSON object', `scriptId: ${scriptId++}`);
       return iframe.executeScript('window.btoa("a")', {url})
     }, bail).then(rv => {
       ok(c(rv, 'YQ=='), `scriptId: ${scriptId++}`);
       return iframe.executeScript('window.wrappedJSObject.btoa("a")', {url})
     }, bail).then(bail, (error) => {
-      is(error.message, `TypeError: window.wrappedJSObject is undefined, can't access property "btoa" of it`, `scriptId: ${scriptId++}`);
+      is(error.message, `TypeError: window.wrappedJSObject is undefined; can't access its "btoa" property`, `scriptId: ${scriptId++}`);
       return iframe.executeScript('42', {})
     }).then(bail, error => {
       is(error.name, 'InvalidAccessError', `scriptId: ${scriptId++}`);
       return iframe.executeScript('42');
     }).then(bail, error => {
       is(error.name, 'InvalidAccessError', `scriptId: ${scriptId++}`);
       return iframe.executeScript('43', { url: 'http://foo.com' });
     }).then(bail, (error) => {
--- a/js/src/jit-test/tests/basic/bug827104.js
+++ b/js/src/jit-test/tests/basic/bug827104.js
@@ -5,9 +5,9 @@ function f() {
     }
     a[i][0] = 0;
 }
 
 var e;
 try {
     f();
 } catch (error) {e = error;}
-assertEq(e.toString(), `TypeError: a[i] is undefined, can't access property 0 of it`);
+assertEq(e.toString(), `TypeError: a[i] is undefined; can't access element at index 0`);
--- a/js/src/jit-test/tests/basic/expression-autopsy.js
+++ b/js/src/jit-test/tests/basic/expression-autopsy.js
@@ -13,17 +13,17 @@ function check_one(expected, f, err) {
         assertEq(s.slice(11, -err.length), expected);
     }
     if (!failed)
         throw new Error("didn't fail");
 }
 ieval = eval;
 function check(expr, expected=expr, testStrict=true) {
     var end, err;
-    for ([end, err] of [[".random_prop", ` is undefined, can't access property \"random_prop" of it`], ["()", " is not a function"]]) {
+    for ([end, err] of [[".random_prop", ` is undefined; can't access its "random_prop" property`], ["()", " is not a function"]]) {
         var statement = "o = {};" + expr + end, f;
         var cases = [
             // Global scope
             function () {
                 ieval("var o, undef;\n" + statement);
             },
             // Function scope
             Function("o", "undef", statement),
@@ -97,17 +97,17 @@ check("o[(~(o + 1))]");
 check("o[(+ (o + 1))]");
 check("o[(- (o + 1))]");
 
 // A few one off tests
 check_one("6", (function () { 6() }), " is not a function");
 check_one("4", (function() { (4||eval)(); }), " is not a function");
 check_one("0", (function () { Array.prototype.reverse.call('123'); }), " is read-only");
 check_one("[...][Symbol.iterator](...).next(...).value",
-          function () { ieval("{ let x; var [a, b, [c0, c1]] = [x, x, x]; }") }, " is undefined, can't access property Symbol.iterator of it");
+          function () { ieval("{ let x; var [a, b, [c0, c1]] = [x, x, x]; }") }, " is undefined; can't access its Symbol.iterator property");
 check_one("(void 1)", function() { (void 1)(); }, " is not a function");
 check_one("(void o[1])", function() { var o = []; (void o[1])() }, " is not a function");
 
 check_one("(typeof 1)", function() { (typeof 1)(); }, " is not a function");
 check_one("(typeof o[1])", function() { var o = []; (typeof o[1])() }, " is not a function");
 
 check_one("(delete foo)",
           function() { (delete foo)(); },
--- a/js/src/jit-test/tests/basic/iterable-error-messages.js
+++ b/js/src/jit-test/tests/basic/iterable-error-messages.js
@@ -10,30 +10,30 @@ function assertThrowsMsg(f, msg) {
 
 // For-of
 function testForOf(val) {
     for (var x of val) {}
 }
 for (v of [{}, Math, new Proxy({}, {})]) {
     assertThrowsMsg(() => testForOf(v), "val is not iterable");
 }
-assertThrowsMsg(() => testForOf(null), "val is null, can't access property Symbol.iterator of it");
+assertThrowsMsg(() => testForOf(null), "val is null; can't access its Symbol.iterator property");
 assertThrowsMsg(() => { for (var x of () => 1) {}}, "() => 1 is not iterable");
 
 // Destructuring
 function testDestr(val) {
     var [a, b] = val;
 }
 for (v of [{}, Math, new Proxy({}, {})]) {
     assertThrowsMsg(() => testDestr(v), "val is not iterable");
 }
-assertThrowsMsg(() => testDestr(null), "val is null, can't access property Symbol.iterator of it");
+assertThrowsMsg(() => testDestr(null), "val is null; can't access its Symbol.iterator property");
 assertThrowsMsg(() => { [a, b] = () => 1; }, "() => 1 is not iterable");
 
 // Spread
 function testSpread(val) {
     [...val];
 }
 for (v of [{}, Math, new Proxy({}, {})]) {
     assertThrowsMsg(() => testSpread(v), "val is not iterable");
 }
-assertThrowsMsg(() => testSpread(null), "val is null, can't access property Symbol.iterator of it");
+assertThrowsMsg(() => testSpread(null), "val is null; can't access its Symbol.iterator property");
 assertThrowsMsg(() => { [...() => 1]; }, "() => 1 is not iterable");
--- a/js/src/jit-test/tests/basic/testBug604210.js
+++ b/js/src/jit-test/tests/basic/testBug604210.js
@@ -1,11 +1,11 @@
 function f() {
     var msg = '';
     try {
         var x = undefined;
         print(x.foo);
     } catch (e) {
         msg = '' + e;
     }
-    assertEq(msg, `TypeError: x is undefined, can't access property "foo" of it`);
+    assertEq(msg, `TypeError: x is undefined; can't access its "foo" property`);
 }
 f();
--- a/js/src/jit-test/tests/debug/bug1275001.js
+++ b/js/src/jit-test/tests/debug/bug1275001.js
@@ -12,19 +12,19 @@ function check_one(expected, f, err) {
     } catch (ex) {
         s = ex.toString()
         assertEq(s.slice(11, -err.length), expected)
     }
 }
 ieval = eval
 function check(expr, expected = expr) {
     var end, err
-    for ([end, err] of[[".random_prop", ` is undefined, can't access property \"random_prop" of it` ]]) 
+    for ([end, err] of [[".random_prop", ` is undefined, can't access its "random_prop" property`]])
          statement = "o = {};" + expr + end;
          cases = [
             function() { return ieval("var undef;" + statement); },
             Function(statement)
         ]
-        for (f of cases) 
+        for (f of cases)
             check_one(expected, f, err)
 }
 check("undef");
 check("o.b");
--- a/js/src/jit-test/tests/ion/bug913749.js
+++ b/js/src/jit-test/tests/ion/bug913749.js
@@ -10,12 +10,12 @@ this.toSource();
 
 y = undefined;
 
 for (var i = 0; i < 3; i++) {
     try {
 	x.toString();
 	assertEq(0, 1);
     } catch (e) {
-	assertEq(e.message === `y is undefined, can't access property "length" of it` ||
+	assertEq(e.message === `y is undefined; can't access its "length" property` ||
 		 e.message === `can't access property "length" of undefined`, true);
     }
 }
--- a/js/src/js.msg
+++ b/js/src/js.msg
@@ -4,31 +4,32 @@
  * 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/. */
 
 /*
  * This is the JavaScript error message file.
  *
  * The format for each JS error message is:
  *
- * MSG_DEF(<SYMBOLIC_NAME>, <ARGUMENT_COUNT>, <EXCEPTION_NAME>,
- *         <FORMAT_STRING>)
+ *     MSG_DEF(<SYMBOLIC_NAME>, <ARGUMENT_COUNT>, <EXCEPTION_NAME>,
+ *             <FORMAT_STRING>)
  *
- * where ;
- * <SYMBOLIC_NAME> is a legal C identifer that will be used in the
- * JS engine source.
+ * where:
+ *
+ *     <SYMBOLIC_NAME> is a legal C identifer that will be used in the
+ *     JS engine source.
  *
- * <ARGUMENT_COUNT> is an integer literal specifying the total number of
- * replaceable arguments in the following format string.
+ *     <ARGUMENT_COUNT> is an integer literal specifying the total number of
+ *     replaceable arguments in the following format string.
  *
- * <EXCEPTION_NAME> is an enum JSExnType value, defined in jsapi.h.
+ *     <EXCEPTION_NAME> is an enum JSExnType value, defined in jsapi.h.
  *
- * <FORMAT_STRING> is a string literal, optionally containing sequences
- * {X} where X  is an integer representing the argument number that will
- * be replaced with a string value when the error is reported.
+ *     <FORMAT_STRING> is a string literal, optionally containing sequences
+ *     {X} where X  is an integer representing the argument number that will
+ *     be replaced with a string value when the error is reported.
  *
  * e.g.
  *
  * MSG_DEF(JSMSG_NOT_A_SUBSPECIES, 2, JSEXN_TYPEERROR,
  *         "{0} is not a member of the {1} family")
  *
  * can be used:
  *
@@ -50,17 +51,18 @@ MSG_DEF(JSMSG_CANT_DELETE,             1
 MSG_DEF(JSMSG_CANT_TRUNCATE_ARRAY,     0, JSEXN_TYPEERR, "can't delete non-configurable array element")
 MSG_DEF(JSMSG_NOT_FUNCTION,            1, JSEXN_TYPEERR, "{0} is not a function")
 MSG_DEF(JSMSG_NOT_CONSTRUCTOR,         1, JSEXN_TYPEERR, "{0} is not a constructor")
 MSG_DEF(JSMSG_CANT_CONVERT_TO,         2, JSEXN_TYPEERR, "can't convert {0} to {1}")
 MSG_DEF(JSMSG_TOPRIMITIVE_NOT_CALLABLE, 2, JSEXN_TYPEERR, "can't convert {0} to {1}: its [Symbol.toPrimitive] property is not a function")
 MSG_DEF(JSMSG_TOPRIMITIVE_RETURNED_OBJECT, 2, JSEXN_TYPEERR, "can't convert {0} to {1}: its [Symbol.toPrimitive] method returned an object")
 MSG_DEF(JSMSG_NO_PROPERTIES,           1, JSEXN_TYPEERR, "{0} has no properties")
 MSG_DEF(JSMSG_PROPERTY_FAIL,           2, JSEXN_TYPEERR, "can't access property {0} of {1}")
-MSG_DEF(JSMSG_PROPERTY_FAIL_EXPR,      3, JSEXN_TYPEERR, "{0} is {1}, can't access property {2} of it")
+MSG_DEF(JSMSG_PROPERTY_FAIL_EXPR,      3, JSEXN_TYPEERR, "{0} is {1}; can't access its {2} property")
+MSG_DEF(JSMSG_ELEMENT_FAIL_EXPR,       3, JSEXN_TYPEERR, "{0} is {1}; can't access element at index {2}")
 MSG_DEF(JSMSG_BAD_REGEXP_FLAG,         1, JSEXN_SYNTAXERR, "invalid regular expression flag {0}")
 MSG_DEF(JSMSG_INVALID_DATA_VIEW_LENGTH, 0, JSEXN_RANGEERR, "invalid data view length")
 MSG_DEF(JSMSG_OFFSET_LARGER_THAN_FILESIZE, 0, JSEXN_RANGEERR, "offset is larger than filesize")
 MSG_DEF(JSMSG_OFFSET_OUT_OF_BUFFER,    0, JSEXN_RANGEERR, "start offset is outside the bounds of the buffer")
 MSG_DEF(JSMSG_OFFSET_OUT_OF_DATAVIEW,  0, JSEXN_RANGEERR, "offset is outside the bounds of the DataView")
 MSG_DEF(JSMSG_SPREAD_TOO_LARGE,        0, JSEXN_RANGEERR, "array too large due to spread operand(s)")
 MSG_DEF(JSMSG_BAD_WEAKMAP_KEY,         0, JSEXN_TYPEERR, "cannot use the given object as a weak map key")
 MSG_DEF(JSMSG_BAD_GETTER_OR_SETTER,    1, JSEXN_TYPEERR, "invalid {0} usage")
--- a/js/src/jsapi-tests/testErrorInterceptor.cpp
+++ b/js/src/jsapi-tests/testErrorInterceptor.cpp
@@ -32,35 +32,35 @@ bool equalStrings(JSContext* cx, JSStrin
     return result == 0;
 }
 }
 
 BEGIN_TEST(testErrorInterceptor)
 {
     // Run the following snippets.
     const char* SAMPLES[] = {
-        "throw new Error('I am an Error')\0",
-        "throw new TypeError('I am a TypeError')\0",
-        "throw new ReferenceError('I am a ReferenceError')\0",
-        "throw new SyntaxError('I am a SyntaxError')\0",
-        "throw 5\0",
-        "undefined[0]\0",
-        "foo[0]\0",
-        "b[\0",
+        "throw new Error('I am an Error')",
+        "throw new TypeError('I am a TypeError')",
+        "throw new ReferenceError('I am a ReferenceError')",
+        "throw new SyntaxError('I am a SyntaxError')",
+        "throw 5",
+        "undefined[0]",
+        "foo[0]",
+        "b[",
     };
     // With the simpleInterceptor, we should end up with the following error:
     const char* TO_STRING[] = {
-        "Error: I am an Error\0",
-        "TypeError: I am a TypeError\0",
-        "ReferenceError: I am a ReferenceError\0",
-        "SyntaxError: I am a SyntaxError\0",
-        "5\0",
-        "TypeError: can't access property 0 of undefined\0",
-        "ReferenceError: foo is not defined\0",
-        "SyntaxError: expected expression, got end of script\0",
+        "Error: I am an Error",
+        "TypeError: I am a TypeError",
+        "ReferenceError: I am a ReferenceError",
+        "SyntaxError: I am a SyntaxError",
+        "5",
+        "TypeError: can't access property 0 of undefined",
+        "ReferenceError: foo is not defined",
+        "SyntaxError: expected expression, got end of script",
     };
     MOZ_ASSERT(mozilla::ArrayLength(SAMPLES) == mozilla::ArrayLength(TO_STRING));
 
 
     // Save original callback.
     JSErrorInterceptor* original = JS_GetErrorInterceptorCallback(cx->runtime());
     gLatestMessage.init(cx);
 
--- a/js/src/tests/non262/extensions/regress-353116.js
+++ b/js/src/tests/non262/extensions/regress-353116.js
@@ -40,31 +40,31 @@ function test()
     null.y;
   }
   catch(ex)
   {
     actual = ex + '';
   }
   reportCompare(expect, actual, summary);
 
-  expect = `TypeError: x is undefined, can't access property "y" of it`;
+  expect = `TypeError: x is undefined; can't access its "y" property`;
   actual = 'No Error';
 
   try
   {
     x = undefined; 
     x.y;
   }
   catch(ex)
   {
     actual = ex + '';
   }
   reportCompare(expect, actual, summary);
 
-  expect = `TypeError: x is null, can't access property "y" of it`;
+  expect = `TypeError: x is null; can't access its "y" property`;
   actual = 'No Error';
 
   try
   {
     x = null; 
     x.y;
   }
   catch(ex)
--- a/js/src/tests/non262/regress/regress-469625-03.js
+++ b/js/src/tests/non262/regress/regress-469625-03.js
@@ -20,17 +20,17 @@ function test()
 {
   printBugNumber(BUGNUMBER);
   printStatus (summary);
  
   function f(x) {
     var [a, b, [c0, c1]] = [x, x, x];
   }
 
-  expect = `TypeError: [...][Symbol.iterator](...).next(...).value is null, can't access property Symbol.iterator of it`;
+  expect = `TypeError: [...][Symbol.iterator](...).next(...).value is null; can't access its Symbol.iterator property`;
   actual = 'No Error';
   try
   {
     f(null);
   }
   catch(ex)
   {
     actual = ex + '';
--- a/js/src/tests/non262/regress/regress-469758.js
+++ b/js/src/tests/non262/regress/regress-469758.js
@@ -4,11 +4,11 @@
 var err;
 try {
     {let i=1}
     {let j=1; [][j][2]}
 } catch (e) {
     err = e;
 }
 assertEq(err instanceof TypeError, true);
-assertEq(err.message, "[][j] is undefined, can't access property 2 of it");
+assertEq(err.message, "[][j] is undefined; can't access element at index 2");
 
 reportCompare(0, 0, 'ok');
--- a/js/src/vm/JSContext.cpp
+++ b/js/src/vm/JSContext.cpp
@@ -996,23 +996,25 @@ js::ReportIsNullOrUndefinedForPropertyAc
     UniqueChars bytes = DecompileValueGenerator(cx, JSDVG_SEARCH_STACK, v, nullptr);
     if (!bytes) {
         return;
     }
 
     if (strcmp(bytes.get(), js_undefined_str) == 0 || strcmp(bytes.get(), js_null_str) == 0) {
         JS_ReportErrorNumberUTF8(cx, GetErrorMessage, nullptr, JSMSG_PROPERTY_FAIL,
                                  keyBytes.get(), bytes.get());
-    } else if (v.isUndefined()) {
-        JS_ReportErrorNumberUTF8(cx, GetErrorMessage, nullptr, JSMSG_PROPERTY_FAIL_EXPR,
-                                 bytes.get(), js_undefined_str, keyBytes.get());
     } else {
-        MOZ_ASSERT(v.isNull());
-        JS_ReportErrorNumberUTF8(cx, GetErrorMessage, nullptr, JSMSG_PROPERTY_FAIL_EXPR,
-                                 bytes.get(), js_null_str, keyBytes.get());
+        const char* actual = v.isUndefined() ? js_undefined_str : js_null_str;
+        if (JSID_IS_INT(key)) {
+            JS_ReportErrorNumberUTF8(cx, GetErrorMessage, nullptr, JSMSG_ELEMENT_FAIL_EXPR,
+                                     bytes.get(), actual, keyBytes.get());
+        } else {
+            JS_ReportErrorNumberUTF8(cx, GetErrorMessage, nullptr, JSMSG_PROPERTY_FAIL_EXPR,
+                                     bytes.get(), actual, keyBytes.get());
+        }
     }
 }
 
 void
 js::ReportMissingArg(JSContext* cx, HandleValue v, unsigned arg)
 {
     char argbuf[11];
     UniqueChars bytes;
--- a/toolkit/components/extensions/test/xpcshell/test_ext_contentscript_create_iframe.js
+++ b/toolkit/components/extensions/test/xpcshell/test_ext_contentscript_create_iframe.js
@@ -120,17 +120,17 @@ add_task(async function test_contentscri
       manifest = win.browser.runtime.getManifest();
     } catch (e) {
       manifestException = e;
     }
 
     Assert.ok(!manifest, "manifest should be undefined");
 
     Assert.equal(String(manifestException),
-                 `TypeError: win.browser.runtime is undefined, can't access property "getManifest" of it`,
+                 `TypeError: win.browser.runtime is undefined; can't access its "getManifest" property`,
                  "expected exception received");
 
     let getManifestException = win.testGetManifestException();
 
     Assert.equal(getManifestException, "TypeError: can't access dead object",
                  "expected exception received");
   });