Bug 1521677. Change defineProperties to handle non-configurable properties on WindowProxy like defineProperty does. r=evilpie

We want to indicate "we didn't actually manage to define your properties as
non-configurable" but we don't want to throw exceptions when people try to
Object.defineProperties with a non-configurable property in the list on a
WindowProxy.

Though maybe we should just silently ignore the "can't define non-configurable
on WindowProxy" bits in defineProperties and just return the object anyway?
The reason we do the "return null" bit in defineProperty is so that Caja and
the like can detect it, but if they're not using defineProperties to start with
they may not need to detect this "failure".  On the other hand, Mark Miller
thinks the detection will be needed...

Differential Revision: https://phabricator.services.mozilla.com/D17516

--HG--
extra : moz-landing-system : lando

dom/base/test/test_window_define_nonconfigurable.html

@@ -21,11 +21,11 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=1107443
   var retval = Object.defineProperty(window, "nosuchprop",
                                      { value: 5, configurable: false });
   if (AppConstants.RELEASE_OR_BETA) {
-    todo_is(retval, false,
-            "Should return false when 'failing' to define non-configurable property via Object.defineProperty.")
+    todo_is(retval, null,
+            "Should return null when 'failing' to define non-configurable property via Object.defineProperty.")
   } else {
-    is(retval, false,
-       "Should return false when 'failing' to define non-configurable property via Object.defineProperty.")
+    is(retval, null,
+       "Should return null when 'failing' to define non-configurable property via Object.defineProperty.")
   }
   var desc = Object.getOwnPropertyDescriptor(window, "nosuchprop");
   is(typeof(desc), "object", "Should have a property 'nosuchprop' now");
@@ -39,6 +39,33 @@ https://bugzilla.mozilla.org/show_bug.cgi?id=1107443
   is(desc.writable, false, "Property 'nosuchprop' should be readonly");
   is(desc.value, 5, "Property 'nosuchprop' should have the right value");
 
+  retval = Object.defineProperties(window, {
+    "firstProp": { value: 1 },
+    "secondProp": { value: 2, configurable: false },
+    "thirdProp": { value: 3 },
+  });
+  if (AppConstants.RELEASE_OR_BETA) {
+    todo_is(retval, null,
+            "Should return null when 'failing' to define non-configurable property via Object.defineProperties.")
+  } else {
+    is(retval, null,
+       "Should return null when 'failing' to define non-configurable property via Object.defineProperties.")
+  }
+  // The properties should all be defined.
+  for (var [prop, val] of [["firstProp", 1], ["secondProp", 2], ["thirdProp", 3]]) {
+    desc = Object.getOwnPropertyDescriptor(window, prop);
+    is(typeof(desc), "object", `Should have a property '${prop}' now`);
+    if (AppConstants.RELEASE_OR_BETA) {
+      todo_is(desc.configurable, true,
+              `Property '${prop}' should be configurable`);
+    } else {
+      is(desc.configurable, true,
+	 `Property '${prop}' should be configurable`);
+    }
+    is(desc.writable, false, `Property '${prop}' should be readonly`);
+    is(desc.value, val, `Property '${prop}' should have the right value`);
+  }
+
   retval = Object.defineProperty(window, "nosuchprop2", { value: 6 });
   is(retval, window,
      "Should return object when succesfully defining 'nosuchprop2'");

js/src/builtin/Object.cpp

@@ -1061,7 +1061,8 @@ PlainObject* js::ObjectCreateWithTemplate(JSContext* cx,
 
 // ES 2017 draft 19.1.2.3.1
 static bool ObjectDefineProperties(JSContext* cx, HandleObject obj,
-                                   HandleValue properties) {
+                                   HandleValue properties,
+                                   bool* failedOnWindowProxy) {
   // Step 1. implicit
   // Step 2.
   RootedObject props(cx, ToObject(cx, properties));
@@ -1105,10 +1106,20 @@ static bool ObjectDefineProperties(JSContext* cx, HandleObject obj,
   }
 
   // Step 6.
+  *failedOnWindowProxy = false;
   for (size_t i = 0, len = descriptors.length(); i < len; i++) {
-    if (!DefineProperty(cx, obj, descriptorKeys[i], descriptors[i])) {
+    ObjectOpResult result;
+    if (!DefineProperty(cx, obj, descriptorKeys[i], descriptors[i], result)) {
       return false;
     }
+
+    if (!result.ok()) {
+      if (result.failureCode() == JSMSG_CANT_DEFINE_WINDOW_NC) {
+        *failedOnWindowProxy = true;
+      } else if (!result.checkStrict(cx, obj, descriptorKeys[i])) {
+        return false;
+      }
+    }
   }
 
   return true;
@@ -1145,9 +1156,13 @@ bool js::obj_create(JSContext* cx, unsigned argc, Value* vp) {
 
   // Step 3.
   if (args.hasDefined(1)) {
-    if (!ObjectDefineProperties(cx, obj, args[1])) {
+    // we can't ever end up with failures to define on a WindowProxy
+    // here, because "obj" is never a WindowProxy.
+    bool failedOnWindowProxy = false;
+    if (!ObjectDefineProperties(cx, obj, args[1], &failedOnWindowProxy)) {
       return false;
     }
+    MOZ_ASSERT(!failedOnWindowProxy, "How did we get a WindowProxy here?");
   }
 
   // Step 4.
@@ -1890,12 +1905,11 @@ static bool obj_getOwnPropertySymbols(JSContext* cx, unsigned argc, Value* vp) {
 static bool obj_defineProperties(JSContext* cx, unsigned argc, Value* vp) {
   CallArgs args = CallArgsFromVp(argc, vp);
 
-  /* Steps 1 and 7. */
+  /* Step 1. */
   RootedObject obj(cx);
   if (!GetFirstArgumentAsObject(cx, args, "Object.defineProperties", &obj)) {
     return false;
   }
-  args.rval().setObject(*obj);
 
   /* Step 2. */
   if (!args.requireAtLeast(cx, "Object.defineProperties", 2)) {
@@ -1903,7 +1917,18 @@ static bool obj_defineProperties(JSContext* cx, unsigned argc, Value* vp) {
   }
 
   /* Steps 3-6. */
-  return ObjectDefineProperties(cx, obj, args[1]);
+  bool failedOnWindowProxy = false;
+  if (!ObjectDefineProperties(cx, obj, args[1], &failedOnWindowProxy)) {
+    return false;
+  }
+
+  /* Step 7, but modified to deal with WindowProxy mess */
+  if (failedOnWindowProxy) {
+    args.rval().setNull();
+  } else {
+    args.rval().setObject(*obj);
+  }
+  return true;
 }
 
 // ES6 20141014 draft 19.1.2.15 Object.preventExtensions(O)

js/src/builtin/Object.js

@@ -283,7 +283,7 @@ function ObjectDefineProperty(obj, propertyKey, attributes) {
     // Steps 1-4.
     if (!ObjectOrReflectDefineProperty(obj, propertyKey, attributes, true)) {
         // Not standardized yet: https://github.com/tc39/ecma262/pull/688
-        return false;
+        return null;
     }
 
     // Step 5.