Bug 750307 - "Assertion failure: isBoolean()" in RegExpObject::ignoreCase after redefining nonconfigurable data property. r=Waldo. Second landing, test change rs=bholley on IRC.
authorJason Orendorff <jorendorff@mozilla.com>
Wed, 13 Jun 2012 03:11:18 -0500
changeset 96559 2ddb6278d1de
parent 96558 3dbdfe9b7565
child 96560 d846a8245199
push id10597
push userjorendorff@mozilla.com
push dateWed, 13 Jun 2012 08:17:14 +0000
treeherdermozilla-inbound@2ddb6278d1de [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersWaldo, bholley
bugs750307
milestone16.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 750307 - "Assertion failure: isBoolean()" in RegExpObject::ignoreCase after redefining nonconfigurable data property. r=Waldo. Second landing, test change rs=bholley on IRC.
js/src/jit-test/tests/basic/bug750307.js
js/src/jsobj.cpp
js/src/jsobj.h
js/src/jsproxy.cpp
toolkit/mozapps/extensions/test/xpinstall/bug645699.html
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/basic/bug750307.js
@@ -0,0 +1,6 @@
+load(libdir + "asserts.js");
+var g = newGlobal('new-compartment');
+var a = g.RegExp("x");
+assertThrowsInstanceOf(function () { Object.defineProperty(a, "ignoreCase", {value: undefined}); },
+                       g.TypeError);
+a.toString();
--- a/js/src/jsobj.cpp
+++ b/js/src/jsobj.cpp
@@ -1898,16 +1898,60 @@ Reject(JSContext *cx, JSObject *obj, uns
 {
     if (throwError)
         return Throw(cx, obj, errorNumber);
 
     *rval = false;
     return JS_TRUE;
 }
 
+// See comments on CheckDefineProperty in jsobj.h.
+//
+// DefinePropertyOnObject has its own implementation of these checks.
+//
+bool
+js::CheckDefineProperty(JSContext *cx, HandleObject obj, HandleId id, HandleValue value,
+                        PropertyOp getter, StrictPropertyOp setter, unsigned attrs)
+{
+    if (!obj->isNative())
+        return true;
+
+    // ES5 8.12.9 Step 1. Even though we know obj is native, we use generic
+    // APIs for shorter, more readable code.
+    AutoPropertyDescriptorRooter desc(cx);
+    if (!GetOwnPropertyDescriptor(cx, obj, id, &desc))
+        return false;
+
+    // This does not have to check obj->isExtensible() when !desc.obj (steps
+    // 2-3) because the low-level methods JSObject::{add,put}Property check
+    // for that.
+    if (desc.obj && (desc.attrs & JSPROP_PERMANENT)) {
+        // Steps 6-11, skipping step 10.a.ii. Prohibit redefining a permanent
+        // property with different metadata, except to make a writable property
+        // non-writable.
+        if (getter != desc.getter ||
+            setter != desc.setter ||
+            (attrs != desc.attrs && attrs != (desc.attrs | JSPROP_READONLY)))
+        {
+            return Throw(cx, id, JSMSG_CANT_REDEFINE_PROP);
+        }
+
+        // Step 10.a.ii. Prohibit changing the value of a non-configurable,
+        // non-writable data property.
+        if ((desc.attrs & (JSPROP_GETTER | JSPROP_SETTER | JSPROP_READONLY)) == JSPROP_READONLY) {
+            bool same;
+            if (!SameValue(cx, value, desc.value, &same))
+                return false;
+            if (!same)
+                return JSObject::reportReadOnly(cx, id);
+        }
+    }
+    return true;
+}
+
 static JSBool
 DefinePropertyOnObject(JSContext *cx, HandleObject obj, HandleId id, const PropDesc &desc,
                        bool throwError, bool *rval)
 {
     /* 8.12.9 step 1. */
     JSProperty *current;
     RootedObject obj2(cx);
     JS_ASSERT(!obj->getOps()->lookupGeneric);
@@ -5179,18 +5223,18 @@ js::CheckUndeclaredVarAssignment(JSConte
     return !!bytes &&
            JS_ReportErrorFlagsAndNumber(cx,
                                         (JSREPORT_WARNING | JSREPORT_STRICT
                                          | JSREPORT_STRICT_MODE_ERROR),
                                         js_GetErrorMessage, NULL,
                                         JSMSG_UNDECLARED_VAR, bytes.ptr());
 }
 
-static bool
-ReportReadOnly(JSContext *cx, jsid id, unsigned report)
+bool
+JSObject::reportReadOnly(JSContext *cx, jsid id, unsigned report)
 {
     return js_ReportValueErrorFlags(cx, report, JSMSG_READ_ONLY,
                                     JSDVG_IGNORE_STACK, IdToValue(id), NULL,
                                     NULL, NULL);
 }
 
 bool
 JSObject::reportNotConfigurable(JSContext *cx, jsid id, unsigned report)
@@ -5249,19 +5293,19 @@ baseops::SetPropertyHelper(JSContext *cx
 
                 if ((pd.attrs & (JSPROP_SHARED | JSPROP_SHADOWABLE)) == JSPROP_SHARED) {
                     return !pd.setter ||
                            CallSetter(cx, obj, id, pd.setter, pd.attrs, pd.shortid, strict, vp);
                 }
 
                 if (pd.attrs & JSPROP_READONLY) {
                     if (strict)
-                        return ReportReadOnly(cx, id, JSREPORT_ERROR);
+                        return JSObject::reportReadOnly(cx, id, JSREPORT_ERROR);
                     if (cx->hasStrictOption())
-                        return ReportReadOnly(cx, id, JSREPORT_STRICT | JSREPORT_WARNING);
+                        return JSObject::reportReadOnly(cx, id, JSREPORT_STRICT | JSREPORT_WARNING);
                     return true;
                 }
             }
 
             prop = NULL;
         }
     } else {
         /* We should never add properties to lexical blocks. */
@@ -5293,19 +5337,19 @@ baseops::SetPropertyHelper(JSContext *cx
             if (shape->hasDefaultSetter())
                 return js_ReportGetterOnlyAssignment(cx);
         } else {
             JS_ASSERT(shape->isDataDescriptor());
 
             if (!shape->writable()) {
                 /* Error in strict mode code, warn with strict option, otherwise do nothing. */
                 if (strict)
-                    return ReportReadOnly(cx, id, JSREPORT_ERROR);
+                    return JSObject::reportReadOnly(cx, id, JSREPORT_ERROR);
                 if (cx->hasStrictOption())
-                    return ReportReadOnly(cx, id, JSREPORT_STRICT | JSREPORT_WARNING);
+                    return JSObject::reportReadOnly(cx, id, JSREPORT_STRICT | JSREPORT_WARNING);
                 return JS_TRUE;
             }
         }
 
         attrs = shape->attributes();
         if (pobj != obj) {
             /*
              * We found id in a prototype object: prepare to share or shadow.
--- a/js/src/jsobj.h
+++ b/js/src/jsobj.h
@@ -694,16 +694,17 @@ struct JSObject : public js::ObjectImpl
      * FIXME: bug 593129 -- slot allocation should be done by object methods
      * after calling object-parameter-free shape methods, avoiding coupling
      * logic across the object vs. shape module wall.
      */
     bool allocSlot(JSContext *cx, uint32_t *slotp);
     void freeSlot(JSContext *cx, uint32_t slot);
 
   public:
+    static bool reportReadOnly(JSContext *cx, jsid id, unsigned report = JSREPORT_ERROR);
     bool reportNotConfigurable(JSContext* cx, jsid id, unsigned report = JSREPORT_ERROR);
     bool reportNotExtensible(JSContext *cx, unsigned report = JSREPORT_ERROR);
 
     /*
      * Get the property with the given id, then call it as a function with the
      * given arguments, providing this object as |this|. If the property isn't
      * callable a TypeError will be thrown. On success the value returned by
      * the call is stored in *vp.
@@ -1395,11 +1396,28 @@ GetFirstArgumentAsObject(JSContext *cx, 
 
 /* Helpers for throwing. These always return false. */
 extern bool
 Throw(JSContext *cx, jsid id, unsigned errorNumber);
 
 extern bool
 Throw(JSContext *cx, JSObject *obj, unsigned errorNumber);
 
+/*
+ * Helper function. To approximate a call to the [[DefineOwnProperty]] internal
+ * method described in ES5, first call this, then call JS_DefinePropertyById.
+ *
+ * JS_DefinePropertyById by itself does not enforce the invariants on
+ * non-configurable properties when obj->isNative(). This function performs the
+ * relevant checks (specified in ES5 8.12.9 [[DefineOwnProperty]] steps 1-11),
+ * but only if obj is native.
+ *
+ * The reason for the messiness here is that ES5 uses [[DefineOwnProperty]] as
+ * a sort of extension point, but there is no hook in js::Class,
+ * js::ProxyHandler, or the JSAPI with precisely the right semantics for it.
+ */
+extern bool
+CheckDefineProperty(JSContext *cx, HandleObject obj, HandleId id, HandleValue value,
+                    PropertyOp getter, StrictPropertyOp setter, unsigned attrs);
+
 }  /* namespace js */
 
 #endif /* jsobj_h___ */
--- a/js/src/jsproxy.cpp
+++ b/js/src/jsproxy.cpp
@@ -395,18 +395,20 @@ IndirectProxyHandler::getOwnPropertyDesc
     return GetOwnPropertyDescriptor(cx, GetProxyTargetObject(proxy), id,
                                     JSRESOLVE_QUALIFIED, desc);
 }
 
 bool
 IndirectProxyHandler::defineProperty(JSContext *cx, JSObject *proxy, jsid id,
                                      PropertyDescriptor *desc)
 {
-    return JS_DefinePropertyById(cx, GetProxyTargetObject(proxy), id,
-                                 desc->value, desc->getter, desc->setter,
+    RootedObject obj(cx, GetProxyTargetObject(proxy));
+    return CheckDefineProperty(cx, obj, RootedId(cx, id), RootedValue(cx, desc->value),
+                               desc->getter, desc->setter, desc->attrs) &&
+           JS_DefinePropertyById(cx, obj, id, desc->value, desc->getter, desc->setter,
                                  desc->attrs);
 }
 
 bool
 IndirectProxyHandler::getOwnPropertyNames(JSContext *cx, JSObject *proxy,
                                           AutoIdVector &props)
 {
     return GetPropertyNames(cx, GetProxyTargetObject(proxy),
--- a/toolkit/mozapps/extensions/test/xpinstall/bug645699.html
+++ b/toolkit/mozapps/extensions/test/xpinstall/bug645699.html
@@ -4,17 +4,23 @@
 <html>
 
 <head>
 <title>InstallTrigger tests</title>
 <script type="text/javascript">
 function startInstall() {
   var whiteUrl = "https://example.org/";
 
-  Object.defineProperty(window, "location", { value : { href : whiteUrl }	});
+  try {
+    Object.defineProperty(window, "location", { value : { href : whiteUrl }	});
+    throw new Error("Object.defineProperty(window, 'location', ...) should have thrown");
+  } catch (exc) {
+    if (!(exc instanceof TypeError))
+      throw exc;
+  }
   Object.defineProperty(document, "documentURIObject", { spec : { href : whiteUrl }	});
 
   InstallTrigger.install({
     "Unsigned XPI": "http://example.com/browser/toolkit/mozapps/extensions/test/xpinstall/unsigned.xpi"
   });
 }
 </script>
 </head>