Bug 1007334 - Clean up the GetOwnPropertyNames/Keys situation for ES6 proxies. (r=jorendorff)
authorEric Faust <efaustbmo@gmail.com>
Thu, 19 Jun 2014 15:34:02 -0700
changeset 189605 cdb97e86cc7423927dfa4f867afab567f748a42b
parent 189604 dcf85de7c8ccc31dc44e0ea2c62998e1620098eb
child 189606 c4a8b899f5204b186a6a6979fc56055f39ecb4e5
push id45112
push userefaustbmo@gmail.com
push dateThu, 19 Jun 2014 22:34:20 +0000
treeherdermozilla-inbound@cdb97e86cc74 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjorendorff
bugs1007334
milestone33.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 1007334 - Clean up the GetOwnPropertyNames/Keys situation for ES6 proxies. (r=jorendorff)
js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames2.js
js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames3.js
js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames4.js
js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames5.js
js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames6.js
js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames7.js
js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames8.js
js/src/jit-test/tests/proxy/testDirectProxyKeys10.js
js/src/jit-test/tests/proxy/testDirectProxyKeys2.js
js/src/jit-test/tests/proxy/testDirectProxyKeys3.js
js/src/jit-test/tests/proxy/testDirectProxyKeys4.js
js/src/jit-test/tests/proxy/testDirectProxyKeys5.js
js/src/jit-test/tests/proxy/testDirectProxyKeys6.js
js/src/jit-test/tests/proxy/testDirectProxyKeys7.js
js/src/jit-test/tests/proxy/testDirectProxyKeys8.js
js/src/jit-test/tests/proxy/testDirectProxyKeys9.js
js/src/jsproxy.cpp
js/src/vm/CommonPropertyNames.h
--- a/js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames2.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames2.js
@@ -1,16 +1,16 @@
 /*
  * Call the trap with the handler as the this value, and the target as the first
  * argument
  */
 var target = {};
 var called = false;
 var handler = {
-    getOwnPropertyNames: function (target1) {
+    ownKeys: function (target1) {
         assertEq(this, handler);
         assertEq(target1, target);
         called = true;
         return [];
     }
 };
 assertEq(Object.getOwnPropertyNames(new Proxy(target, handler)).length, 0);
 assertEq(called, true);
--- a/js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames3.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames3.js
@@ -1,10 +1,10 @@
 load(libdir + "asserts.js");
 
 // Throw a TypeError if the trap does not return an object
 assertThrowsInstanceOf(function () {
     Object.getOwnPropertyNames(new Proxy({}, {
-        getOwnPropertyNames: function (target) {
+        ownKeys: function (target) {
             return;
         }
     }));
 }, TypeError);
--- a/js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames4.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames4.js
@@ -1,10 +1,10 @@
 load(libdir + "asserts.js");
 
 // Throw a TypeError if the trap reports the same property twice
 assertThrowsInstanceOf(function () {
     Object.getOwnPropertyNames(new Proxy({}, {
-        getOwnPropertyNames: function (target) {
+        ownKeys: function (target) {
             return [ 'foo', 'foo' ];
         }
     }));
 }, TypeError);
--- a/js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames5.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames5.js
@@ -3,13 +3,13 @@ load(libdir + "asserts.js");
 /*
  * Throw a TypeError if the trap reports a new own property on a non-extensible
  * object
  */
 var target = {};
 Object.preventExtensions(target);
 assertThrowsInstanceOf(function () {
     Object.getOwnPropertyNames(new Proxy(target, {
-        getOwnPropertyNames: function (target) {
+        ownKeys: function (target) {
             return [ 'foo' ];
         }
     }));
 }, TypeError);
--- a/js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames6.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames6.js
@@ -2,13 +2,13 @@ load(libdir + "asserts.js");
 
 // Throw a TypeError if the trap skips a non-configurable property
 var target = {};
 Object.defineProperty(target, 'foo', {
     configurable: false
 });
 assertThrowsInstanceOf(function () {
     Object.getOwnPropertyNames(new Proxy(target, {
-        getOwnPropertyNames: function (target) {
+        ownKeys: function (target) {
             return [];
         }
     }));
 }, TypeError);
--- a/js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames7.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames7.js
@@ -6,13 +6,13 @@ load(libdir + "asserts.js");
  */
 var target = {};
 Object.defineProperty(target, 'foo', {
     configurable: true
 });
 Object.preventExtensions(target);
 assertThrowsInstanceOf(function () {
     Object.getOwnPropertyNames(new Proxy(target, {
-        getOwnPropertyNames: function (target) {
+        ownKeys: function (target) {
             return [];
         }
     }));
 }, TypeError);
--- a/js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames8.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxyGetOwnPropertyNames8.js
@@ -1,15 +1,15 @@
 // Return the names returned by the trap
 var target = {};
 Object.defineProperty(target, 'foo', {
     configurable: true
 });
 var names = Object.getOwnPropertyNames(new Proxy(target, {
-    getOwnPropertyNames : function (target) {
+    ownKeys: function (target) {
         return [ 'bar' ];
     }
 }));
 assertEq(names.length, 1);
 assertEq(names[0], 'bar');
 
 var names = Object.getOwnPropertyNames(new Proxy(Object.create(Object.create(null, {
     a: {
@@ -25,15 +25,15 @@ var names = Object.getOwnPropertyNames(n
         enumerable: true,
         configurable: true
     },
     d: {
         enumerable: false,
         configurable: true
     }
 }), {
-    getOwnPropertyNames: function (target) {
+    ownKeys: function (target) {
         return [ 'c', 'e' ];
     }
 }));
 assertEq(names.length, 2);
 assertEq(names[0], 'c');
 assertEq(names[1], 'e');
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/proxy/testDirectProxyKeys10.js
@@ -0,0 +1,23 @@
+load(libdir + "asserts.js");
+
+// Allow [[GetOwnPropertyDescriptor]] to spoof enumerability of target object's
+// properties. Note that this also tests that the getOwnPropertyDescriptor is
+// called by Object.keys(), as expected.
+
+var target = {};
+Object.defineProperty(target, "foo", { configurable: true, enumerable: false });
+
+function getPD(target, P) {
+    var targetDesc = Object.getOwnPropertyDescriptor(target, P);
+    // Lie about enumerability
+    targetDesc.enumerable = !targetDesc.enumerable;
+    return targetDesc;
+}
+
+var proxy = new Proxy(target, { getOwnPropertyDescriptor: getPD });
+
+assertDeepEq(Object.keys(proxy), ["foo"]);
+
+Object.defineProperty(target, "foo", {configurable: true, enumerable: true});
+
+assertDeepEq(Object.keys(proxy), []);
--- a/js/src/jit-test/tests/proxy/testDirectProxyKeys2.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxyKeys2.js
@@ -1,16 +1,16 @@
 /*
  * Call the trap with the handler as the this value, and the target as the first
  * argument
  */
 var target = {};
 var called = false;
 var handler = {
-    keys: function (target1) {
+    ownKeys: function (target1) {
         assertEq(this, handler);
         assertEq(target1, target);
         called = true;
         return [];
     }
 };
 Object.keys(new Proxy(target, handler));
 assertEq(called, true);
--- a/js/src/jit-test/tests/proxy/testDirectProxyKeys3.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxyKeys3.js
@@ -1,10 +1,10 @@
 load(libdir + "asserts.js");
 
 // Throw a TypeError if the trap does not return an object
 assertThrowsInstanceOf(function () {
     Object.keys(new Proxy({}, {
-        keys: function (target) {
+        ownKeys: function (target) {
             return;
         }
     }));
 }, TypeError);
--- a/js/src/jit-test/tests/proxy/testDirectProxyKeys4.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxyKeys4.js
@@ -1,10 +1,10 @@
 load(libdir + "asserts.js");
 
 // Throw a TypeError if the trap reports the same property twice
 assertThrowsInstanceOf(function () {
     Object.keys(new Proxy({}, {
-        keys: function (target) {
+        ownKeys: function (target) {
             return [ 'foo', 'foo' ];
         }
     }));
 }, TypeError);
--- a/js/src/jit-test/tests/proxy/testDirectProxyKeys5.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxyKeys5.js
@@ -3,13 +3,13 @@ load(libdir + "asserts.js");
 /*
  * Throw a TypeError if the trap reports a new own property on a non-extensible
  * object
  */
 var target = {};
 Object.preventExtensions(target);
 assertThrowsInstanceOf(function () {
     Object.keys(new Proxy(target, {
-        keys: function (target) {
+        ownKeys: function (target) {
             return [ 'foo' ];
         }
     }));
 }, TypeError);
--- a/js/src/jit-test/tests/proxy/testDirectProxyKeys6.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxyKeys6.js
@@ -2,14 +2,14 @@ load(libdir + "asserts.js");
 
 // Throw a TypeError if the trap skips a non-configurable enumerable property
 var target = {};
 Object.defineProperty(target, 'foo', {
     enumerable: true,
     configurable: false
 });
 assertThrowsInstanceOf(function () {
-    Object.getOwnPropertyNames(new Proxy(target, {
-        getOwnPropertyNames: function (target) {
+    Object.keys(new Proxy(target, {
+        ownKeys: function (target) {
             return [];
         }
     }));
 }, TypeError);
--- a/js/src/jit-test/tests/proxy/testDirectProxyKeys7.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxyKeys7.js
@@ -7,13 +7,13 @@ load(libdir + "asserts.js");
 var target = {};
 Object.defineProperty(target, 'foo', {
     enumerable: true,
     configurable: true
 });
 Object.preventExtensions(target);
 assertThrowsInstanceOf(function () {
     Object.keys(new Proxy(target, {
-        keys: function (target) {
+        ownKeys: function (target) {
             return [];
         }
     }));
 }, TypeError);
--- a/js/src/jit-test/tests/proxy/testDirectProxyKeys8.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxyKeys8.js
@@ -8,13 +8,13 @@ var target = {};
 Object.defineProperty(target, 'foo', {
     enumerable: true,
     configurable: true
 });
 Object.preventExtensions(target);
 var caught = false;
 assertThrowsInstanceOf(function () {
     Object.keys(new Proxy(target, {
-        keys: function (target) {
+        ownKeys: function (target) {
             return [];
         }
     }));
 }, TypeError);
--- a/js/src/jit-test/tests/proxy/testDirectProxyKeys9.js
+++ b/js/src/jit-test/tests/proxy/testDirectProxyKeys9.js
@@ -13,14 +13,14 @@ var names = Object.keys(new Proxy(Object
         enumerable: true,
         configurable: true
     },
     d: {
         enumerable: false,
         configurable: true
     }
 }), {
-    keys: function (target) {
+    ownKeys: function (target) {
         return [ 'e' ];
     }
 }));
 assertEq(names.length, 1);
 assertEq(names[0], 'e');
--- a/js/src/jsproxy.cpp
+++ b/js/src/jsproxy.cpp
@@ -1073,17 +1073,21 @@ class ScriptedDirectProxyHandler : publi
     virtual bool has(JSContext *cx, HandleObject proxy, HandleId id, bool *bp) MOZ_OVERRIDE;
     virtual bool hasOwn(JSContext *cx, HandleObject proxy, HandleId id, bool *bp) MOZ_OVERRIDE {
         return BaseProxyHandler::hasOwn(cx, proxy, id, bp);
     }
     virtual bool get(JSContext *cx, HandleObject proxy, HandleObject receiver, HandleId id,
                      MutableHandleValue vp) MOZ_OVERRIDE;
     virtual bool set(JSContext *cx, HandleObject proxy, HandleObject receiver, HandleId id,
                      bool strict, MutableHandleValue vp) MOZ_OVERRIDE;
-    virtual bool keys(JSContext *cx, HandleObject proxy, AutoIdVector &props) MOZ_OVERRIDE;
+    // Kick keys out to getOwnPropertyName and then filter. [[GetOwnProperty]] could potentially
+    // change the enumerability of the target's properties.
+    virtual bool keys(JSContext *cx, HandleObject proxy, AutoIdVector &props) MOZ_OVERRIDE {
+        return BaseProxyHandler::keys(cx, proxy, props);
+    }
     virtual bool iterate(JSContext *cx, HandleObject proxy, unsigned flags,
                          MutableHandleValue vp) MOZ_OVERRIDE;
 
     /* ES6 Harmony traps */
     virtual bool isExtensible(JSContext *cx, HandleObject proxy, bool *extensible) MOZ_OVERRIDE;
 
     /* Spidermonkey extensions. */
     virtual bool call(JSContext *cx, HandleObject proxy, const CallArgs &args) MOZ_OVERRIDE;
@@ -1639,50 +1643,55 @@ ScriptedDirectProxyHandler::defineProper
         }
     }
 
     // [[DefineProperty]] should return a boolean value, which is used to do things like
     // strict-mode throwing. At present, the engine is not prepared to do that. See bug 826587.
     return true;
 }
 
+// This is secretly [[OwnPropertyKeys]]. SM uses the old wiki name, internally.
+// ES6 (5 April 2014) Proxy.[[OwnPropertyKeys]](O)
 bool
 ScriptedDirectProxyHandler::getOwnPropertyNames(JSContext *cx, HandleObject proxy,
                                                 AutoIdVector &props)
 {
-    // step a
+    // step 1
     RootedObject handler(cx, GetDirectProxyHandlerObject(proxy));
 
-    // step b
+    // TODO: step 2: Implement revocation semantics
+
+    // step 3
     RootedObject target(cx, proxy->as<ProxyObject>().target());
 
-    // step c
+    // step 4-5
     RootedValue trap(cx);
-    if (!JSObject::getProperty(cx, handler, handler, cx->names().getOwnPropertyNames, &trap))
+    if (!JSObject::getProperty(cx, handler, handler, cx->names().ownKeys, &trap))
         return false;
 
-    // step d
+    // step 6
     if (trap.isUndefined())
         return DirectProxyHandler::getOwnPropertyNames(cx, proxy, props);
 
-    // step e
+    // step 7-8
     Value argv[] = {
         ObjectValue(*target)
     };
     RootedValue trapResult(cx);
     if (!Invoke(cx, ObjectValue(*handler), trap, ArrayLength(argv), argv, &trapResult))
         return false;
 
-    // step f
+    // step 9
     if (trapResult.isPrimitive()) {
-        ReportInvalidTrapResult(cx, proxy, cx->names().getOwnPropertyNames);
+        ReportInvalidTrapResult(cx, proxy, cx->names().ownKeys);
         return false;
     }
 
-    // steps g to n are shared
+    // Here we add a bunch of extra sanity checks. It is unclear if they will also appear in
+    // the spec. See step 10-11
     return ArrayToIdVector(cx, proxy, target, trapResult, props, JSITER_OWNONLY | JSITER_HIDDEN,
                            cx->names().getOwnPropertyNames);
 }
 
 // ES6 (5 April 2014) Proxy.[[Delete]](P)
 bool
 ScriptedDirectProxyHandler::delete_(JSContext *cx, HandleObject proxy, HandleId id, bool *bp)
 {
@@ -1967,58 +1976,16 @@ ScriptedDirectProxyHandler::set(JSContex
         }
     }
 
     // step 8
     vp.set(BooleanValue(success));
     return true;
 }
 
-// 15.2.3.14 Object.keys (O), step 2
-bool
-ScriptedDirectProxyHandler::keys(JSContext *cx, HandleObject proxy, AutoIdVector &props)
-{
-    // step a
-    RootedObject handler(cx, GetDirectProxyHandlerObject(proxy));
-
-    // step b
-    RootedObject target(cx, proxy->as<ProxyObject>().target());
-
-    // step c
-    RootedValue trap(cx);
-    if (!JSObject::getProperty(cx, handler, handler, cx->names().keys, &trap))
-        return false;
-
-    // step d
-    if (trap.isUndefined())
-        return DirectProxyHandler::keys(cx, proxy, props);
-
-    // step e
-    Value argv[] = {
-        ObjectOrNullValue(target)
-    };
-    RootedValue trapResult(cx);
-    if (!Invoke(cx, ObjectValue(*handler), trap, ArrayLength(argv), argv, &trapResult))
-        return false;
-
-    // step f
-    if (trapResult.isPrimitive()) {
-        JSAutoByteString bytes;
-        if (!AtomToPrintableString(cx, cx->names().keys, &bytes))
-            return false;
-        RootedValue v(cx, ObjectOrNullValue(proxy));
-        js_ReportValueError2(cx, JSMSG_INVALID_TRAP_RESULT, JSDVG_IGNORE_STACK,
-                             v, js::NullPtr(), bytes.ptr());
-        return false;
-    }
-
-    // steps g-n are shared
-    return ArrayToIdVector(cx, proxy, target, trapResult, props, JSITER_OWNONLY, cx->names().keys);
-}
-
 // ES6 (5 April, 2014) 9.5.3 Proxy.[[IsExtensible]](P)
 bool
 ScriptedDirectProxyHandler::isExtensible(JSContext *cx, HandleObject proxy, bool *extensible)
 {
     RootedObject handler(cx, GetDirectProxyHandlerObject(proxy));
 
     RootedObject target(cx, proxy->as<ProxyObject>().target());
 
--- a/js/src/vm/CommonPropertyNames.h
+++ b/js/src/vm/CommonPropertyNames.h
@@ -117,16 +117,17 @@
     macro(lookupGetter, lookupGetter, "__lookupGetter__") \
     macro(lookupSetter, lookupSetter, "__lookupSetter__") \
     macro(maximumFractionDigits, maximumFractionDigits, "maximumFractionDigits") \
     macro(maximumSignificantDigits, maximumSignificantDigits, "maximumSignificantDigits") \
     macro(message, message, "message") \
     macro(minimumFractionDigits, minimumFractionDigits, "minimumFractionDigits") \
     macro(minimumIntegerDigits, minimumIntegerDigits, "minimumIntegerDigits") \
     macro(minimumSignificantDigits, minimumSignificantDigits, "minimumSignificantDigits") \
+    macro(missingArguments, missingArguments, "missingArguments") \
     macro(module, module, "module") \
     macro(multiline, multiline, "multiline") \
     macro(name, name, "name") \
     macro(NaN, NaN, "NaN") \
     macro(next, next, "next") \
     macro(NFC, NFC, "NFC") \
     macro(NFD, NFD, "NFD") \
     macro(NFKC, NFKC, "NFKC") \
@@ -141,18 +142,18 @@
     macro(objectNumber, objectNumber, "[object Number]") \
     macro(objectObject, objectObject, "[object Object]") \
     macro(objectString, objectString, "[object String]") \
     macro(objectUndefined, objectUndefined, "[object Undefined]") \
     macro(objectWindow, objectWindow, "[object Window]") \
     macro(of, of, "of") \
     macro(offset, offset, "offset") \
     macro(optimizedOut, optimizedOut, "optimizedOut") \
-    macro(missingArguments, missingArguments, "missingArguments") \
     macro(outOfMemory, outOfMemory, "out of memory") \
+    macro(ownKeys, ownKeys, "ownKeys") \
     macro(parseFloat, parseFloat, "parseFloat") \
     macro(parseInt, parseInt, "parseInt") \
     macro(pattern, pattern, "pattern") \
     macro(preventExtensions, preventExtensions, "preventExtensions") \
     macro(propertyIsEnumerable, propertyIsEnumerable, "propertyIsEnumerable") \
     macro(proto, proto, "__proto__") \
     macro(prototype, prototype, "prototype") \
     macro(Reify, Reify, "Reify") \