Backed out 2 changesets (bug 1329324, bug 1178639) for failing wpt tests on window-properties.https.html
authorarthur.iakab <aiakab@mozilla.com>
Tue, 27 Nov 2018 22:37:50 +0200
changeset 504857 db5251bae3d4e09b0cadab5378005fa402841f39
parent 504856 d87b95a8c72d513a21d4d6df44a6b7caa2f567a8
child 504858 125f1466fe9dc3062fcaef9b37d6a0dcda7c4b85
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)
bugs1329324, 1178639
milestone65.0a1
backs out8c8ad4d5dbe94b90b340acd449fac1516b563a07
052522c1bc3b62b1c8589dd58dc8be01d67e239a
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
Backed out 2 changesets (bug 1329324, bug 1178639) for failing wpt tests on window-properties.https.html Backed out changeset 8c8ad4d5dbe9 (bug 1178639) Backed out changeset 052522c1bc3b (bug 1329324)
dom/base/nsGlobalWindowOuter.cpp
dom/base/test/test_window_define_nonconfigurable.html
--- a/dom/base/nsGlobalWindowOuter.cpp
+++ b/dom/base/nsGlobalWindowOuter.cpp
@@ -493,37 +493,16 @@ 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;
@@ -531,106 +510,34 @@ nsOuterWindowProxy::getOwnPropertyDescri
     return false;
   }
   if (found) {
     FillPropertyDescriptor(desc, proxy, true);
     return true;
   }
   // else fall through to js::Wrapper
 
-  bool ok = js::Wrapper::getOwnPropertyDescriptor(cx, proxy, id, desc);
-  if (!ok) {
-    return false;
-  }
-
-#ifndef RELEASE_OR_BETA // To be turned on in bug 1496510.
-  if (!IsNonConfigurableReadonlyPrimitiveGlobalProp(cx, id)) {
-    desc.setConfigurable(true);
-  }
-#endif
-
-  return true;
+  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,
                                    JS::ObjectOpResult &result) const
 {
   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();
   }
 
-  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;
+  return js::Wrapper::defineProperty(cx, proxy, id, desc, result);
 }
 
 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,65 +13,68 @@ 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 });
-  is(retval, false,
-     "Should return false when 'failing' to define non-configurable property via Object.defineProperty.")
+  todo_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");
-  is(desc.configurable, true,
-     "Property 'nosuchprop' should be configurable");
+  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 });
   is(retval, window,
      "Should return object when succesfully defining 'nosuchprop2'");
   desc = Object.getOwnPropertyDescriptor(window, "nosuchprop2");
   is(typeof(desc), "object", "Should have a property 'nosuchprop2' now");
-  is(desc.configurable, true,
-     "Property 'nosuchprop2' should be configurable");
+  todo_is(desc.configurable, true,
+          "Property 'nosuchprop2' should be configurable");
   is(desc.writable, false, "Property 'nosuchprop2' should be readonly");
   is(desc.value, 6, "Property 'nosuchprop2' should have the right value");
 
   retval = Object.defineProperty(window, "nosuchprop3",
                                  { value: 7, configurable: true });
   is(retval, window,
      "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 });
-  is(retval, false,
-     "Should not be able to Reflect.defineProperty if non-configurable");
+  todo_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");
-  is(desc.configurable, true,
-     "Property 'nosuchprop4' should be configurable");
+  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",
                                   { value: 9 });
   is(retval, true,
      "Should be able to Reflect.defineProperty with default configurability");
   desc = Object.getOwnPropertyDescriptor(window, "nosuchprop5");
   is(typeof(desc), "object", "Should have a property 'nosuchprop5' now");
-  is(desc.configurable, true,
-     "Property 'nosuchprop5' should be configurable");
+  todo_is(desc.configurable, true,
+          "Property 'nosuchprop5' should be configurable");
   is(desc.writable, false, "Property 'nosuchprop5' should be readonly");
   is(desc.value, 9, "Property 'nosuchprop5' should have the right value");
 
   retval = Reflect.defineProperty(window, "nosuchprop6",
                                   { value: 10, configurable: true });
   is(retval, true,
      "Should be able to Reflect.defineProperty if configurable");
   desc = Object.getOwnPropertyDescriptor(window, "nosuchprop6");