Bug 529404 - Assignments to a property that has a getter but not a setter should not throw a TypeError per ES5, except in strict mode. r=brendan
authorJeff Walden <jwalden@mit.edu>
Tue, 08 Dec 2009 17:00:42 -0800
changeset 36491 6d7cf54e67f11701df3a3b3a3552d95c5ee9550e
parent 36490 968dc5d58a7f934bed7f08015387617aba6a1a36
child 36492 4a6e89d958573ba1facb046a521ab0d3f1452291
push id1
push userroot
push dateMon, 20 Oct 2014 17:29:22 +0000
reviewersbrendan
bugs529404
milestone1.9.3a1pre
Bug 529404 - Assignments to a property that has a getter but not a setter should not throw a TypeError per ES5, except in strict mode. r=brendan
content/base/test/test_classList.html
content/media/test/seek1.js
content/media/test/test_networkState.html
content/media/test/test_readyState.html
content/xbl/test/test_bug371724.xhtml
js/src/jsapi-tests/Makefile.in
js/src/jsapi-tests/testSetPropertyWithNativeGetterStubSetter.cpp
js/src/jsobj.cpp
js/src/jsobj.h
js/src/jsscope.h
js/src/tests/ecma_5/Object/browser.js
js/src/tests/ecma_5/Types/8.12.5-01.js
js/src/tests/ecma_5/Types/browser.js
js/src/tests/ecma_5/Types/jstests.list
js/src/tests/ecma_5/Types/shell.js
js/src/tests/ecma_5/extensions/8.12.5-01.js
js/src/tests/ecma_5/extensions/browser.js
js/src/tests/ecma_5/extensions/jstests.list
js/src/tests/ecma_5/extensions/shell.js
--- a/content/base/test/test_classList.html
+++ b/content/base/test/test_classList.html
@@ -86,30 +86,47 @@ function checkModification(e, funcName, 
     // string.
     var expectedPrevValue = before === null ? "" : before;
     is(gMutationEvents[0].prevValue, expectedPrevValue,
        "wrong previous value " + contextMsg);
     is(gMutationEvents[0].newValue, after, "wrong new value " + contextMsg);
   }
 }
 
+function assignToClassListStrict(e) {
+  "use strict";
+  try {
+    e.classList = "foo";
+    ok(false, "assigning to classList didn't throw");
+  } catch (e) { }
+}
+
+function assignToClassList(e) {
+  try {
+    var expect = e.classList;
+    e.classList = "foo";
+    is(e.classList, expect, "classList should be unchanged after assignment");
+  } catch (e) {
+    ok(false, "assigning to classList threw");
+  }
+}
+
 function testClassList(e) {
 
   // basic tests
 
   isnot(e.classList, undefined, "no classList attribute");
   is(typeof(e.classList.contains), "function",
      "no classList.contains function");
   is(typeof(e.classList.add), "function", "no classList.add function");
   is(typeof(e.classList.remove), "function", "no classList.remove function");
   is(typeof(e.classList.toggle), "function", "no classList.toggle function");
-  try {
-    e.classList = "foo";
-    ok(false, "assigning to classList didn't throw");
-  } catch (e) { }
+
+  assignToClassListStrict(e);
+  assignToClassList(e);
 
   // length attribute
 
   is(e.classList.length, 0, "wrong classList.length value");
   e.setAttribute("class", "");
   is(e.classList.length, 0, "wrong classList.length value");
   e.setAttribute("class", "   \t  \f");
   is(e.classList.length, 0, "wrong classList.length value");
--- a/content/media/test/seek1.js
+++ b/content/media/test/seek1.js
@@ -7,23 +7,23 @@ var seekFlagEnd = false;
 var readonly = true;
 var completed = false;
 
 function startTest() {
   if (completed)
     return false;
   ok(!v.seeking, "seeking should default to false");
   try {
-    v.seeking = 1;
-    readonly = false;
+    v.seeking = true;
+    readonly = v.seeking === false;
   }
   catch(e) {
-    readonly = true;
+    readonly = "threw exception: " + e;
   }
-  ok(readonly, "seeking should be readonly");
+  is(readonly, true, "seeking should be readonly");
 
   v.play();
   v.currentTime=seekTime;
   seekFlagStart = v.seeking;
   return false;
 }
 
 function seekStarted() {
--- a/content/media/test/test_networkState.html
+++ b/content/media/test/test_networkState.html
@@ -5,23 +5,43 @@
   <script type="text/javascript" src="/MochiKit/packed.js"></script>
   <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
 </head>
 <body>
 <video id='v1' onerror="event.stopPropagation();"></video><audio id='a1' onerror="event.stopPropagation();"></audio>
 <pre id="test">
 <script class="testbody" type="text/javascript">
+"use strict";
 var v1 = document.getElementById('v1');
 var a1 = document.getElementById('a1');
-var passed = true;
+var passed = "truthy";
 
 try {
   v1.networkState = 0;
+} catch (e) {
+  passed = !passed;
+}
+try {
   a1.networkState = 0;
-  passed = false;
-} catch(e) { }
-ok(passed, "Should not be able to set networkState (readonly attribute)");
+} catch (e) {
+  passed = !passed;
+}
+ok(passed === true,
+   "Setting networkState throws in strict mode (readonly attribute)");
+</script>
 
+<script class="testbody" type="text/javascript">
+var v1 = document.getElementById('v1');
+var a1 = document.getElementById('a1');
+var passed = false;
+
+var oldv1ns = v1.networkState, olda1ns = a1.networkState;
+try {
+  v1.networkState = 0;
+  a1.networkState = 0;
+  passed = v1.networkState === oldv1ns && a1.networkState === olda1ns;
+} catch (e) { }
+ok(passed, "Should not be able to modify networkState (readonly attribute)");
 </script>
 </pre>
 </body>
 </html>
--- a/content/media/test/test_readyState.html
+++ b/content/media/test/test_readyState.html
@@ -5,26 +5,48 @@
   <script type="text/javascript" src="/MochiKit/packed.js"></script>
   <script type="text/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
   <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css" />
 </head>
 <body>
 <video id='v1' onerror="event.stopPropagation();"></video><audio id='a1' onerror="event.stopPropagation();"></audio>
 <pre id="test">
 <script class="testbody" type="text/javascript">
+"use strict";
 var v1 = document.getElementById('v1');
 var a1 = document.getElementById('a1');
-var passed = true;
+var passed = "truthy";
 
 is(v1.readyState, 0);
 is(a1.readyState, 0);
 
 try {
   v1.readyState = 0;
+} catch (e) {
+  passed = !passed;
+}
+try {
   a1.readyState = 0;
-  passed = false;
+} catch (e) {
+  passed = !passed;
+}
+ok(passed === true,
+   "Setting readyState throws in strict mode (readonly attribute)");
+</script>
+
+<script class="testbody" type="text/javascript">
+var v1 = document.getElementById('v1');
+var a1 = document.getElementById('a1');
+var passed = false;
+
+is(v1.readyState, 0);
+is(a1.readyState, 0);
+
+try {
+  v1.readyState = 1;
+  a1.readyState = 1;
+  passed = v1.readyState === 0 && a1.readyState === 0;
 } catch(e) { }
 ok(passed, "Should not be able to set readyState (readonly attribute)");
-
 </script>
 </pre>
 </body>
 </html>
--- a/content/xbl/test/test_bug371724.xhtml
+++ b/content/xbl/test/test_bug371724.xhtml
@@ -19,23 +19,41 @@ https://bugzilla.mozilla.org/show_bug.cg
   <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=371724">Mozilla Bug 371724</a>
   <p id="display"></p>
   <div id="content" style="display: none"></div>
   <div id="a" style="-moz-binding:url('#rd');">Success!</div>
   <pre id="test">
     <script class="testbody" type="text/javascript">  
       /** Test for Bug 371724 **/
       SimpleTest.waitForExplicitFinish();
-      function doe() {
-        var isException = false;
+      function checkReadOnly() {
+        var elt = document.getElementById('a');
+        var oldValue = elt.hallo;
+        var actual;
         try {
-          document.getElementById('a').hallo = false;
+          elt.hallo = false;
+          actual = elt.hallo;
         } catch (ex) {
-         isException = ex.name;
+          actual = "" + ex;
         }
-        ok(isException == "TypeError", "Setting a readonly xbl property should throw a TypeError exception");
+        is(actual, true,
+           "Setting a readonly xbl property should do nothing if !strict");
+        checkReadOnlyStrict();
+      }
+      function checkReadOnlyStrict() {
+        "use strict";
+        var elt = document.getElementById('a');
+        var actual;
+        try {
+          elt.hallo = false;
+          actual = "should have thrown";
+        } catch (ex) {
+          actual = ex instanceof TypeError;
+        }
+        is(actual, true,
+           "Setting a readonly xbl property should throw a TypeError exception if strict");
         SimpleTest.finish();
       }
-      setTimeout(doe, 600);
+      addLoadEvent(checkReadOnly);
     </script>
   </pre>
 </body>
 </html>
--- a/js/src/jsapi-tests/Makefile.in
+++ b/js/src/jsapi-tests/Makefile.in
@@ -54,16 +54,17 @@ CPPSRCS = \
   testExtendedEq.cpp \
   testIntString.cpp \
   testLookup.cpp \
   testPropCache.cpp \
   testTrap.cpp \
   testSameValue.cpp \
   testSeal.cpp \
   testXDR.cpp \
+  testSetPropertyWithNativeGetterStubSetter.cpp \
   $(NULL)
 
 DEFINES         += -DEXPORT_JS_API
 
 LIBS      = $(NSPR_LIBS) $(DEPTH)/$(LIB_PREFIX)js_static.$(LIB_SUFFIX)
 
 LOCAL_INCLUDES += -I$(topsrcdir) -I..
 
new file mode 100644
--- /dev/null
+++ b/js/src/jsapi-tests/testSetPropertyWithNativeGetterStubSetter.cpp
@@ -0,0 +1,62 @@
+#include "tests.h"
+#include "jsxdrapi.h"
+
+static JSBool
+nativeGet(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
+{
+    *vp = INT_TO_JSVAL(17);
+    return JS_TRUE;
+}
+
+BEGIN_TEST(testSetPropertyWithNativeGetterStubSetter)
+{
+    jsvalRoot vobj(cx);
+    JSObject *obj = JS_NewObject(cx, NULL, NULL, NULL);
+    CHECK(obj);
+    vobj = OBJECT_TO_JSVAL(obj);
+
+    CHECK(JS_DefineProperty(cx, global, "globalProp", vobj,
+                            JS_PropertyStub, JS_PropertyStub,
+                            JSPROP_ENUMERATE));
+
+    CHECK(JS_DefineProperty(cx, obj, "prop", JSVAL_VOID,
+                            nativeGet, JS_PropertyStub,
+                            JSPROP_SHARED));
+
+    EXEC("'use strict';                                     \n"
+         "var error, passed = false;                        \n"
+         "try                                               \n"
+         "{                                                 \n"
+         "  this.globalProp.prop = 42;                      \n"
+         "  throw new Error('setting property succeeded!'); \n"
+         "}                                                 \n"
+         "catch (e)                                         \n"
+         "{                                                 \n"
+         "  error = e;                                      \n"
+         "  if (e instanceof TypeError)                     \n"
+         "    passed = true;                                \n"
+         "}                                                 \n"
+         "                                                  \n"
+         "if (!passed)                                      \n"
+         "  throw error;                                    \n");
+
+    EXEC("var error, passed = false;                        \n"
+         "try                                               \n"
+         "{                                                 \n"
+         "  this.globalProp.prop = 42;                      \n"
+         "  if (this.globalProp.prop === 17)                \n"
+         "    passed = true;                                \n"
+         "  else                                            \n"
+         "    throw new Error('bad value after set!');      \n"
+         "}                                                 \n"
+         "catch (e)                                         \n"
+         "{                                                 \n"
+         "  error = e;                                      \n"
+         "}                                                 \n"
+         "                                                  \n"
+         "if (!passed)                                      \n"
+         "  throw error;                                    \n");
+
+    return true;
+}
+END_TEST(testSetPropertyWithNativeGetterStubSetter)
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -4385,21 +4385,22 @@ js_NativeSet(JSContext *cx, JSObject *ob
                 return false;
             }
             LOCKED_OBJ_SET_SLOT(obj, slot, *vp);
             return true;
         }
     } else {
         /*
          * Allow API consumers to create shared properties with stub setters.
-         * Such properties lack value storage, so setting them is like writing
-         * to /dev/null.
+         * Such properties effectively function as data descriptors which are
+         * not writable, so attempting to set such a property should do nothing
+         * or throw if we're in strict mode.
          */
-        if (SPROP_HAS_STUB_SETTER(sprop))
-            return true;
+        if (!(sprop->attrs & JSPROP_GETTER) && SPROP_HAS_STUB_SETTER(sprop))
+            return js_ReportGetterOnlyAssignment(cx);
     }
 
     sample = cx->runtime->propertyRemovals;
     JS_UNLOCK_SCOPE(cx, scope);
     JS_PUSH_TEMP_ROOT_SPROP(cx, sprop, &tvr);
     ok = sprop->set(cx, obj, vp);
     JS_POP_TEMP_ROOT(cx, &tvr);
     if (!ok)
@@ -6116,27 +6117,30 @@ js_IsCallable(JSObject *obj, JSContext *
     JS_LOCK_OBJ(cx, obj);
     JSBool callable = (obj->map->ops == &js_ObjectOps)
                       ? HAS_FUNCTION_CLASS(obj) || STOBJ_GET_CLASS(obj)->call
                       : obj->map->ops->call != NULL;
     JS_UNLOCK_OBJ(cx, obj);
     return callable;
 }
 
-void
+JSBool
 js_ReportGetterOnlyAssignment(JSContext *cx)
 {
-    JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL,
-                         JSMSG_GETTER_ONLY, NULL);
+    return JS_ReportErrorFlagsAndNumber(cx,
+                                        JSREPORT_WARNING | JSREPORT_STRICT |
+                                        JSREPORT_STRICT_MODE_ERROR,
+                                        js_GetErrorMessage, NULL,
+                                        JSMSG_GETTER_ONLY);
 }
 
 JS_FRIEND_API(JSBool)
 js_GetterOnlyPropertyStub(JSContext *cx, JSObject *obj, jsval id, jsval *vp)
 {
-    js_ReportGetterOnlyAssignment(cx);
+    JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_GETTER_ONLY);
     return JS_FALSE;
 }
 
 #ifdef DEBUG
 
 /*
  * Routines to print out values during debugging.  These are FRIEND_API to help
  * the debugger find them and to support temporarily hacking js_Dump* calls
--- a/js/src/jsobj.h
+++ b/js/src/jsobj.h
@@ -985,17 +985,17 @@ js_GetWrappedObject(JSContext *cx, JSObj
 extern const char *
 js_ComputeFilename(JSContext *cx, JSStackFrame *caller,
                    JSPrincipals *principals, uintN *linenop);
 
 /* Infallible, therefore cx is last parameter instead of first. */
 extern JSBool
 js_IsCallable(JSObject *obj, JSContext *cx);
 
-void
+extern JSBool
 js_ReportGetterOnlyAssignment(JSContext *cx);
 
 extern JS_FRIEND_API(JSBool)
 js_GetterOnlyPropertyStub(JSContext *cx, JSObject *obj, jsval id, jsval *vp);
 
 #ifdef DEBUG
 JS_FRIEND_API(void) js_DumpChars(const jschar *s, size_t n);
 JS_FRIEND_API(void) js_DumpString(JSString *str);
--- a/js/src/jsscope.h
+++ b/js/src/jsscope.h
@@ -848,20 +848,18 @@ JSScopeProperty::set(JSContext* cx, JSOb
 {
     JS_ASSERT_IF(SPROP_HAS_STUB_SETTER(this), attrs & JSPROP_GETTER);
 
     if (attrs & JSPROP_SETTER) {
         jsval fval = setterValue();
         return js_InternalGetOrSet(cx, obj, id, fval, JSACC_WRITE, 1, vp, vp);
     }
 
-    if (attrs & JSPROP_GETTER) {
-        js_ReportGetterOnlyAssignment(cx);
-        return false;
-    }
+    if (attrs & JSPROP_GETTER)
+        return !!js_ReportGetterOnlyAssignment(cx);
 
     /* See the comment in JSScopeProperty::get as to why we can check for With. */
     if (STOBJ_GET_CLASS(obj) == &js_WithClass)
         obj = obj->map->ops->thisObject(cx, obj);
     return setter(cx, obj, SPROP_USERID(this), vp);
 }
 
 /* Macro for common expression to test for shared permanent attributes. */
--- a/js/src/tests/ecma_5/Object/browser.js
+++ b/js/src/tests/ecma_5/Object/browser.js
@@ -0,0 +1,1 @@
+
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_5/Types/8.12.5-01.js
@@ -0,0 +1,111 @@
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/*
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/
+ * Contributor:
+ *   Jason Orendorff
+ *   Jeff Walden <jwalden+code@mit.edu>
+ */
+
+var gTestfile = "template.js";
+//-----------------------------------------------------------------------------
+var BUGNUMBER = 523846;
+var summary =
+  "Assignments to a property that has a getter but not a setter should not " +
+  "throw a TypeError per ES5 (at least not until strict mode is supported)";
+var actual = "Early failure";
+var expect = "No errors";
+
+
+printBugNumber(BUGNUMBER);
+printStatus(summary);
+
+var o = { get p() { return "a"; } };
+
+function test1()
+{
+  o.p = "b";
+  assertEq(o.p, "a");
+}
+
+function test2()
+{
+  function T() {}
+  T.prototype = o;
+  y = new T();
+  y.p = "b";
+  assertEq(y.p, "a");
+}
+
+function strictTest1()
+{
+  "use strict";
+
+  o.p = "b"; // strict-mode violation here
+  assertEq(o.p, "a");
+}
+
+function strictTest2()
+{
+  "use strict";
+
+  function T() {}
+  T.prototype = o;
+  y = new T;
+  y.p = "b";  // strict-mode violation here
+  assertEq(y.p, "a");
+}
+
+var errors = [];
+try
+{
+  try
+  {
+    test1();
+  }
+  catch (e)
+  {
+    errors.push(e);
+  }
+
+  try
+  {
+    test2();
+  }
+  catch (e)
+  {
+    errors.push(e);
+  }
+
+  try
+  {
+    strictTest1();
+    errors.push("strictTest1 didn't fail");
+  }
+  catch (e)
+  {
+    if (!(e instanceof TypeError))
+      errors.push("strictTest1 didn't fail with a TypeError: " + e);
+  }
+
+  try
+  {
+    strictTest2();
+    errors.push("strictTest2 didn't fail");
+  }
+  catch (e)
+  {
+    if (!(e instanceof TypeError))
+      errors.push("strictTest2 didn't fail with a TypeError: " + e);
+  }
+}
+catch (e)
+{
+  errors.push("Unexpected error: " + e);
+}
+finally
+{
+  actual = errors.length > 0 ? errors.join(", ") : "No errors";
+}
+
+reportCompare(expect, actual, summary);
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_5/Types/browser.js
@@ -0,0 +1,1 @@
+
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_5/Types/jstests.list
@@ -0,0 +1,2 @@
+url-prefix ../../jsreftest.html?test=ecma_5/Types/
+script 8.12.5-01.js
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_5/Types/shell.js
@@ -0,0 +1,1 @@
+gTestsubsuite='Types';
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_5/extensions/8.12.5-01.js
@@ -0,0 +1,98 @@
+/* -*- Mode: C++; tab-width: 2; indent-tabs-mode: nil; c-basic-offset: 2 -*- */
+/*
+ * Any copyright is dedicated to the Public Domain.
+ * http://creativecommons.org/licenses/publicdomain/
+ * Contributor:
+ *   Jason Orendorff
+ *   Jeff Walden <jwalden+code@mit.edu>
+ */
+
+var gTestfile = "template.js";
+//-----------------------------------------------------------------------------
+var BUGNUMBER = 523846;
+var summary =
+  "Assignments to a property that has a getter but not a setter should not " +
+  "throw a TypeError per ES5 (at least not until strict mode is supported)";
+var actual = "Early failure";
+var expect = "No errors";
+
+
+printBugNumber(BUGNUMBER);
+printStatus(summary);
+
+var o = { get p() { return "a"; } };
+
+function test1()
+{
+  o.p = "b"; // strict-mode violation here
+  assertEq(o.p, "a");
+}
+
+function test2()
+{
+  function T() {}
+  T.prototype = o;
+  y = new T();
+  y.p = "b";  // strict-mode violation here
+  assertEq(y.p, "a");
+}
+
+
+var errors = [];
+try
+{
+  try
+  {
+    test1();
+  }
+  catch (e)
+  {
+    errors.push(e);
+  }
+
+  try
+  {
+    test2();
+  }
+  catch (e)
+  {
+    errors.push(e);
+  }
+
+  options("strict");
+  options("werror");
+  try
+  {
+    test1();
+    errors.push("strict+werror didn't make test1 fail");
+  }
+  catch (e)
+  {
+    if (!(e instanceof TypeError))
+      errors.push("test1 with strict+werror failed without a TypeError: " + e);
+  }
+
+  try
+  {
+    test2();
+    errors.push("strict+werror didn't make test2 fail");
+  }
+  catch (e)
+  {
+    if (!(e instanceof TypeError))
+      errors.push("test2 with strict+werror failed without a TypeError: " + e);
+  }
+
+  options("strict");
+  options("werror");
+}
+catch (e)
+{
+  errors.push("Unexpected error: " + e);
+}
+finally
+{
+  actual = errors.length > 0 ? errors.join(", ") : "No errors";
+}
+
+reportCompare(expect, actual, summary);
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_5/extensions/browser.js
@@ -0,0 +1,1 @@
+
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_5/extensions/jstests.list
@@ -0,0 +1,2 @@
+url-prefix ../../jsreftest.html?test=ecma_5/extensions/
+script 8.12.5-01.js
new file mode 100644
--- /dev/null
+++ b/js/src/tests/ecma_5/extensions/shell.js
@@ -0,0 +1,1 @@
+gTestsubsuite='extensions';