Bug 1304638 - Improve error message for JSMSG_READ_ONLY when we're not in C++ code. r?nbp draft
authorEmilio Cobos Álvarez <ecoal95@gmail.com>
Tue, 11 Oct 2016 15:23:28 +0200
changeset 423643 a4ea922a94e8f1fe5f157a1733d476de0f11a3f0
parent 423262 a6e940b9ca9c902d3b9ac1ae50df276eb9b61caa
child 533493 48aae853b9e55bb2c3dc56a5d3d409605d6e1487
push id31952
push userbmo:ecoal95@gmail.com
push dateTue, 11 Oct 2016 13:27:37 +0000
reviewersnbp
bugs1304638
milestone52.0a1
Bug 1304638 - Improve error message for JSMSG_READ_ONLY when we're not in C++ code. r?nbp MozReview-Commit-ID: 581CP726TzA
js/src/jit-test/tests/basic/expression-autopsy.js
js/src/jsapi.cpp
js/src/jsopcode.cpp
js/src/tests/ecma_3/Object/8.6.1-01.js
--- a/js/src/jit-test/tests/basic/expression-autopsy.js
+++ b/js/src/jit-test/tests/basic/expression-autopsy.js
@@ -95,17 +95,17 @@ check("o[- (o)]");
 
 // A few one off tests
 check_one("6", (function () { 6() }), " is not a function");
 check_one("0", (function () { Array.prototype.reverse.call('123'); }), " is read-only");
 check_one("(intermediate value)[Symbol.iterator](...).next(...).value",
           function () { ieval("{ let x; var [a, b, [c0, c1]] = [x, x, x]; }") }, " is undefined");
 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("0", function() {
+check_one("o[0]", function() {
   "use strict";
   var o = [0];
   Object.freeze(o);
   o[0] = "foo";
 }, " is read-only");
 
 // Manual testing for this case: the only way to trigger an error is *not* on
 // an attempted property access during destructuring, and the error message
--- a/js/src/jsapi.cpp
+++ b/js/src/jsapi.cpp
@@ -143,31 +143,46 @@ static bool
 ErrorTakesObjectArgument(unsigned msg)
 {
     MOZ_ASSERT(msg < JSErr_Limit);
     unsigned argCount = js_ErrorFormatString[msg].argCount;
     MOZ_ASSERT(argCount <= 2);
     return argCount == 2;
 }
 
+static bool
+LastFrameIsFunctionCall(JSContext* cx)
+{
+    FrameIter iter(cx);
+    jsbytecode* pc = iter.pc();
+    return (JSOp)*pc == JSOP_FUNCALL;
+}
+
 JS_PUBLIC_API(bool)
 JS::ObjectOpResult::reportStrictErrorOrWarning(JSContext* cx, HandleObject obj, HandleId id,
                                                bool strict)
 {
     static_assert(unsigned(OkCode) == unsigned(JSMSG_NOT_AN_ERROR),
                   "unsigned value of OkCode must not be an error code");
     MOZ_ASSERT(code_ != Uninitialized);
     MOZ_ASSERT(!ok());
 
     unsigned flags = strict ? JSREPORT_ERROR : (JSREPORT_WARNING | JSREPORT_STRICT);
-    if (code_ == JSMSG_OBJECT_NOT_EXTENSIBLE || code_ == JSMSG_SET_NON_OBJECT_RECEIVER) {
+
+    // Avoid walking the stack if the error is a read-only error and the last frame is a function
+    // call (that means that the error happened in a C++ function like js::array_reverse), since it
+    // produces unintuitive error messages. We fall back to only formatting the property name in
+    // that case.
+    if (code_ == JSMSG_OBJECT_NOT_EXTENSIBLE || code_ == JSMSG_SET_NON_OBJECT_RECEIVER ||
+        (code_ == JSMSG_READ_ONLY && !LastFrameIsFunctionCall(cx))) {
         RootedValue val(cx, ObjectValue(*obj));
         return ReportValueErrorFlags(cx, flags, code_, JSDVG_IGNORE_STACK, val,
                                      nullptr, nullptr, nullptr);
     }
+
     if (ErrorTakesArguments(code_)) {
         RootedValue idv(cx, IdToValue(id));
         RootedString str(cx, ValueToSource(cx, idv));
         if (!str)
             return false;
 
         JSAutoByteString propName;
         if (!propName.encodeUtf8(cx, str))
--- a/js/src/jsopcode.cpp
+++ b/js/src/jsopcode.cpp
@@ -1242,19 +1242,21 @@ ExpressionDecompiler::decompilePC(jsbyte
       }
       case JSOP_GETALIASEDVAR: {
         JSAtom* atom = EnvironmentCoordinateName(cx->caches.envCoordinateNameCache, script, pc);
         MOZ_ASSERT(atom);
         return write(atom);
       }
       case JSOP_LENGTH:
       case JSOP_GETPROP:
+      case JSOP_SETPROP:
       case JSOP_CALLPROP: {
         RootedAtom prop(cx, (op == JSOP_LENGTH) ? cx->names().length : loadAtom(pc));
-        if (!decompilePCForStackOperand(pc, -1))
+        int objIndex = (op == JSOP_SETPROP) ? -2 : -1;
+        if (!decompilePCForStackOperand(pc, objIndex))
             return false;
         if (IsIdentifier(prop)) {
             return write(".") &&
                    quote(prop, '\0');
         }
         return write("[") &&
                quote(prop, '\'') &&
                write("]");
--- a/js/src/tests/ecma_3/Object/8.6.1-01.js
+++ b/js/src/tests/ecma_3/Object/8.6.1-01.js
@@ -10,17 +10,17 @@ var summary = 'In strict mode, setting a
 
 printBugNumber(BUGNUMBER);
 printStatus (summary);
 
 enterFunc (String (BUGNUMBER));
 
 // should throw an error in strict mode
 var actual = '';
-var expect = '"length" is read-only';
+var expect = 's.length is read-only';
 var status = summary + ': Throw if STRICT and WERROR is enabled';
 
 if (!options().match(/strict/))
 {
   options('strict');
 }
 if (!options().match(/werror/))
 {