Bug 1329324. When trying to define non-configurable properties on a WindowProxy, communicate back that we didn't "really" define them as non-configurable. r=peterv
authorBoris Zbarsky <bzbarsky@mit.edu>
Tue, 27 Nov 2018 14:16:22 -0500
changeset 504859 a9dac00a9eef530202050947dc2cac3ef72541ec
parent 504858 125f1466fe9dc3062fcaef9b37d6a0dcda7c4b85
child 504860 95651672782a47a2031a4ea492f85053c98d5769
push id10290
push userffxbld-merge
push dateMon, 03 Dec 2018 16:23:23 +0000
treeherdermozilla-beta@700bed2445e6 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerspeterv
bugs1329324
milestone65.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 1329324. When trying to define non-configurable properties on a WindowProxy, communicate back that we didn't "really" define them as non-configurable. r=peterv
dom/base/nsGlobalWindowOuter.cpp
dom/base/test/test_window_define_nonconfigurable.html
--- a/dom/base/nsGlobalWindowOuter.cpp
+++ b/dom/base/nsGlobalWindowOuter.cpp
@@ -493,16 +493,37 @@ nsOuterWindowProxy::getPropertyDescripto
 
   if (desc.object()) {
     return true;
   }
 
   return js::Wrapper::getPropertyDescriptor(cx, proxy, id, desc);
 }
 
+/**
+ * IsNonConfigurableReadonlyPrimitiveGlobalProp returns true for
+ * property names that fit the following criteria:
+ *
+ * 1) The ES spec defines a property with that name on globals.
+ * 2) The property is non-configurable.
+ * 3) The property is non-writable (readonly).
+ * 4) The value of the property is a primitive (so doesn't change
+ *    observably on when navigation happens).
+ *
+ * Such properties can act as actual non-configurable properties on a
+ * WindowProxy, because they are not affected by navigation.
+ */
+static bool
+IsNonConfigurableReadonlyPrimitiveGlobalProp(JSContext* cx, JS::Handle<jsid> id)
+{
+  return id == GetJSIDByIndex(cx, XPCJSContext::IDX_NAN) ||
+         id == GetJSIDByIndex(cx, XPCJSContext::IDX_UNDEFINED) ||
+         id == GetJSIDByIndex(cx, XPCJSContext::IDX_INFINITY);
+}
+
 bool
 nsOuterWindowProxy::getOwnPropertyDescriptor(JSContext* cx,
                                              JS::Handle<JSObject*> proxy,
                                              JS::Handle<jsid> id,
                                              JS::MutableHandle<JS::PropertyDescriptor> desc)
                                              const
 {
   bool found;
@@ -510,16 +531,20 @@ nsOuterWindowProxy::getOwnPropertyDescri
     return false;
   }
   if (found) {
     FillPropertyDescriptor(desc, proxy, true);
     return true;
   }
   // else fall through to js::Wrapper
 
+
+  // When we change this to always claim the property is configurable (bug
+  // 1178639), update the comments in nsOuterWindowProxy::defineProperty
+  // accordingly.
   return js::Wrapper::getOwnPropertyDescriptor(cx, proxy, id, desc);
 }
 
 bool
 nsOuterWindowProxy::defineProperty(JSContext* cx,
                                    JS::Handle<JSObject*> proxy,
                                    JS::Handle<jsid> id,
                                    JS::Handle<JS::PropertyDescriptor> desc,
@@ -527,17 +552,78 @@ nsOuterWindowProxy::defineProperty(JSCon
 {
   if (IsArrayIndex(GetArrayIndexFromId(cx, id))) {
     // Spec says to Reject whether this is a supported index or not,
     // since we have no indexed setter or indexed creator.  It is up
     // to the caller to decide whether to throw a TypeError.
     return result.failCantDefineWindowElement();
   }
 
-  return js::Wrapper::defineProperty(cx, proxy, id, desc, result);
+  JS::ObjectOpResult ourResult;
+  bool ok = js::Wrapper::defineProperty(cx, proxy, id, desc, ourResult);
+  if (!ok) {
+    return false;
+  }
+
+  if (!ourResult.ok()) {
+    // It's possible that this failed because the page got the existing
+    // descriptor (which we force to claim to be configurable) and then tried to
+    // redefine the property with the descriptor it got but a different value.
+    // We want to allow this case to succeed, so check for it and if we're in
+    // that case try again but now with an attempt to define a non-configurable
+    // property.
+    if (!desc.hasConfigurable() || !desc.configurable()) {
+      // The incoming descriptor was not explicitly marked "configurable: true",
+      // so it failed for some other reason.  Just propagate that reason out.
+      result = ourResult;
+      return true;
+    }
+
+    JS::Rooted<JS::PropertyDescriptor> existingDesc(cx);
+    ok = js::Wrapper::getOwnPropertyDescriptor(cx, proxy, id, &existingDesc);
+    if (!ok) {
+      return false;
+    }
+    if (!existingDesc.object() || existingDesc.configurable()) {
+      // We have no existing property, or its descriptor is already configurable
+      // (on the Window itself, where things really can be non-configurable).
+      // So we failed for some other reason, which we should propagate out.
+      result = ourResult;
+      return true;
+    }
+
+    JS::Rooted<JS::PropertyDescriptor> updatedDesc(cx, desc);
+    updatedDesc.setConfigurable(false);
+
+    JS::ObjectOpResult ourNewResult;
+    ok = js::Wrapper::defineProperty(cx, proxy, id, updatedDesc, ourNewResult);
+    if (!ok) {
+      return false;
+    }
+
+    if (!ourNewResult.ok()) {
+      // Twiddling the configurable flag didn't help.  Just return this failure
+      // out to the caller.
+      result = ourNewResult;
+      return true;
+    }
+  }
+
+#ifndef RELEASE_OR_BETA // To be turned on in bug 1496510.
+  if (desc.hasConfigurable() && !desc.configurable() &&
+      !IsNonConfigurableReadonlyPrimitiveGlobalProp(cx, id)) {
+    // Give callers a way to detect that they failed to "really" define a
+    // non-configurable property.
+    result.failCantDefineWindowNonConfigurable();
+    return true;
+  }
+#endif
+
+  result.succeed();
+  return true;
 }
 
 bool
 nsOuterWindowProxy::ownPropertyKeys(JSContext *cx,
                                     JS::Handle<JSObject*> proxy,
                                     JS::AutoIdVector &props) const
 {
   // Just our indexed stuff followed by our "normal" own property names.
--- a/dom/base/test/test_window_define_nonconfigurable.html
+++ b/dom/base/test/test_window_define_nonconfigurable.html
@@ -13,18 +13,18 @@ https://bugzilla.mozilla.org/show_bug.cg
   /**
    * Test for Bug 1107443, modified when it was backed out in bug 1329323.
    * This is now testing the _current_ behavior, not the desired one; expect
    * failures in this test and needing to update it when bug 1329324 is
    * fixed.
    */
   var retval = Object.defineProperty(window, "nosuchprop",
                                      { value: 5, configurable: false });
-  todo_is(retval, false,
-          "Should return false when 'failing' to define non-configurable property via Object.defineProperty.")
+  is(retval, false,
+     "Should return false 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");
   todo_is(desc.configurable, true,
           "Property 'nosuchprop' should be configurable");
   is(desc.writable, false, "Property 'nosuchprop' should be readonly");
   is(desc.value, 5, "Property 'nosuchprop' should have the right value");
 
   retval = Object.defineProperty(window, "nosuchprop2", { value: 6 });
@@ -43,23 +43,20 @@ https://bugzilla.mozilla.org/show_bug.cg
      "Should return object when succesfully defining 'nosuchprop3'");
   desc = Object.getOwnPropertyDescriptor(window, "nosuchprop3");
   is(typeof(desc), "object", "Should have a property 'nosuchprop3' now");
   is(desc.configurable, true,
           "Property 'nosuchprop3' should be configurable");
   is(desc.writable, false, "Property 'nosuchprop3' should be readonly");
   is(desc.value, 7, "Property 'nosuchprop3' should have the right value");
 
-  // XXXbz it's not actually entirely clear what behavior the
-  // Reflect.defineProperty bits should have.  Check it carefully once there's a
-  // spec.
   retval = Reflect.defineProperty(window, "nosuchprop4",
                                   { value: 8, configurable: false });
-  todo_is(retval, false,
-          "Should not be able to Reflect.defineProperty if non-configurable");
+  is(retval, false,
+     "Should not be able to Reflect.defineProperty if non-configurable");
   desc = Object.getOwnPropertyDescriptor(window, "nosuchprop4");
   is(typeof(desc), "object", "Should have a property 'nosuchprop4' now");
   todo_is(desc.configurable, true,
           "Property 'nosuchprop4' should be configurable");
   is(desc.writable, false, "Property 'nosuchprop4' should be readonly");
   is(desc.value, 8, "Property 'nosuchprop4' should have the right value");
 
   retval = Reflect.defineProperty(window, "nosuchprop5",