Object.freezing proxies should freeze or throw?
Good catch. This appears to be a genuine spec bug.
First, I thought that the problem simply was due to sloppy-mode silent failures, as a call to Object.isFrozen revealed that the proxy is not actually frozen after the call to Object.freeze. When your script is run in strict mode, it fails:
on Chrome: Uncaught TypeError: 'set' on proxy: trap returned falsish for property 'x' on Firefox: TypeError: proxy set handler returned false for property '"x"'
However, this error can easily be avoided by adding a return true to the
set trap. At this point, your example still runs in strict mode, which
worried me.
The culprit is not that the proxy's "x" property can still be updated, but rather that Object.freeze(proxy) doesn't actually freeze the proxy yet still returns without throwing. Thus, a shorter testcase of the problem is:
"use strict";
const target = Object.seal({x: 2});
const proxy = new Proxy(target, {
defineProperty() {
return true;
}
});
Object.freeze(proxy);
console.log(Object.isFrozen(proxy)); // expected true, but is false
Tracing through the ES2015 spec, the spec 'call stack' is roughly:
19.1.2.5 Object.freeze ( O ) calls 7.3.14 SetIntegrityLevel (O, level = "frozen") calls 7.3.7 DefinePropertyOrThrow (O, P = "x", desc = {configurable: false, writable: false}) calls 9.5.6 [[DefineOwnProperty]] (P, Desc) calls 9.1.6.2 IsCompatiblePropertyDescriptor (Extensible, Desc, Current) calls 9.1.6.3 ValidateAndApplyPropertyDescriptor (O, P, extensible, Desc, current) where O = undefined, P = undefined, extensible = false, Desc = {configurable: false, writable: false}, current = { value: 2, writable: true, enumerable: true, configurable: false }
The ValidateAndApplyPropertyDescriptor, when called via
IsCompatiblePropertyDescriptor, essentially checks whether it is legal to
update an existing property descriptor current to a new state described
by Desc, without actually performing the update.
Tracing through the details of that algorithm, it turns out the above property descriptors are indeed compatible. It is legal to update a data property {writable: true, configurable: false} to {writable: false, configurable: false}. IIRC, this is somewhat of an exception as it is the only state transition allowed on a non-configurable data property.
Because IsCompatiblePropertyDescriptor returns true, the
[[DefineOwnProperty]] call on the proxy completes successfully, signaling
to SetIntegrityLevel that the property appears to be successfully updated
to {configurable: false, writable: false} (even though it didn't in this
case, as the proxy's "defineProperty" trap simply returned true without
actually performing the update).
The quick fix appears to be to change the SetIntegrityLevel algorithm as follows, by updating step 10:
- If O is a Proxy, return TestIntegrityLevel(O, "frozen"), otherwise return true.
With this change, the call to SetIntegrityLevel ends with a call to TestIntegrityLevel, which will notice that O is in fact not frozen and thus cause the call to Object.freeze to fail.
To summarize: the error here is not with the proxy invariant checks (the proxy is still unfrozen), but rather that because of the exceptional allowed state transfer from writable,non-configurable to non-writable,non-configurable, SetIntegrityLevel fails to detect that the proxy object being frozen in fact remains unfrozen.
Cheers, Tom
2016-08-06 21:09 GMT+02:00 Raul-Sebastian Mihăilă <raul.mihaila at gmail.com>:
Would be a good idea to do the test for any kind of exotic objects (including host defined ones), not only for proxies?
yes it would. The invariants apply to all objects.
If someone would like to write up the spec bug and fix, I'll happily represent and advance it at upcoming meetings.
Tom,
I.m not sure that I agree with your conclusion and your fix. I agree that there doesn’t appear to be a bug with the implementation of any of the currently specified proxy invariant checks and hence the problem is internal to Object.freeze. But Object.freeze is not a “MOP level” operation. If is just a function, just like any function that a ES programmer might write, that manipulates an object using the MOP and assumes that the objet essential invariants are maintained. Testing an object to see if it is a Proxy is not an operation that is available to ES code via the MOP. If we respecify Object.freeze to make such a test we are using special powers that would not be available to an ES programer who wanted to build a similar operation.
I think the real problem is actually is with the invariants. Note that [[Set]] has two invariants that are predicated by the "previously observed” state of a property’s configurable and writable attributes. Now “observed” has always been a trickle concept to translate into the specification of the actual Proxy internal method. Rather than keeping track of what has been observed we tried to ensure that inconsistent states are unobservable.
In the case of Proxy [[DefineOwnProperty]], it seems to me that returning true on a call that sets a property to non-configurable or non-configurable+non-writable is a form of observation. If the corresponding target property has not been set by the handler to the requested value, then [[DefineOwnProperty]] should not be allowed to return true. The problem with the current spec. is that before allowing true to be returned it only checks that the requested value of configurable has been set in the target. It seems to me that if the target property is non-configurable, then it should also ensure that a request to set writable to false has set actually set writable to false on the target property. If not, it should throw.
“Testing an object to see if it is a Proxy is not an operation that is available to ES code via the MOP”
By the way, why such a test is not available? I think that everything should be testable, but it’s not the case actually.
Here is another test case, with [[GetOwnPropertyDescriptor]]:
var target = Object.seal({x: 2});
var proxy = new Proxy(target, {
getOwnPropertyDescriptor(o, p) {
var d = Reflect.getOwnPropertyDescriptor(o, p)
if (d && 'writable' in d)
d.writable = false
return d
}
});
Object.getOwnPropertyDescriptor(proxy, 'x'); // { value: 2, writable: false, configurable: false }
Object.defineProperty(proxy, 'x', { value: 3 })
Object.getOwnPropertyDescriptor(proxy, 'x'); // { value: 3, writable: false, configurable: false }
IMHO, the most robust fix is the following: Both the [[DefineOwnProperty]] and the [[GetOwnPropertyDescriptor]] contain the following check:
- If IsCompatiblePropertyDescriptor(extensibleTarget, resultDesc, targetDesc) is false, throw a TypeError exception.
I think there should be also the following check (except when targetDesc is undefined, in which case another appropriate check is used):
- If IsCompatiblePropertyDescriptor(extensibleTarget, targetDesc, resultDesc) is false, throw a TypeError exception.
That would replace the weaker adhoc check about the [[Configurable]] attribute (search for settingConfigFalse) in those algorithms.
(Alternatively, in order to avoid double checks, define an AreBothWaysCompatiblePropertyDescriptors(extensibleTarget, desc1, desc2) abstract operation.)
Le 8 août 2016 à 11:02, Claude Pache <claude.pache at gmail.com> a écrit :
Here is another test case, with [[GetOwnPropertyDescriptor]]:
var target = Object.seal({x: 2}); var proxy = new Proxy(target, { getOwnPropertyDescriptor(o, p) { var d = Reflect.getOwnPropertyDescriptor(o, p) if (d && 'writable' in d) d.writable = false return d } }); Object.getOwnPropertyDescriptor(proxy, 'x'); // { value: 2, writable: false, configurable: false } Object.defineProperty(proxy, 'x', { value: 3 }) Object.getOwnPropertyDescriptor(proxy, 'x'); // { value: 3, writable: false, configurable: false }IMHO, the most robust fix is the following: Both the [[DefineOwnProperty]] and the [[GetOwnPropertyDescriptor]] contain the following check:
- If IsCompatiblePropertyDescriptor(extensibleTarget, resultDesc, targetDesc) is false, throw a TypeError exception.
I think there should be also the following check (except when targetDesc is undefined, in which case another appropriate check is used):
- If IsCompatiblePropertyDescriptor(extensibleTarget, targetDesc, resultDesc) is false, throw a TypeError exception.
That would replace the weaker adhoc check about the [[Configurable]] attribute (search for settingConfigFalse) in those algorithms.
(Alternatively, in order to avoid double checks, define an AreBothWaysCompatiblePropertyDescriptors(extensibleTarget, desc1, desc2) abstract operation.)
—Claude
Looking closer, it seems that using IsCompatiblePropertyDescriptor is in fact an overkill, because we probably don’t want the special case of conditionally mutable [[Writable]] attribute for nonconfigurable properties.
That is, I think that the following condition must hold: If either the target has a nonconfigurable property, or the proxy claims to have a nonconfigurable property, then every attribute of the property descriptor claimed by the proxy must be identical to the corresponding attribute of the property descriptor of the target.
Claude's additional example is indeed evidence that Object.freeze is not to blame, but rather that the invariant checks of [[GetOwnPropertyDescriptor]] and [[DefineOwnProperty]] are too weak. The culprit is, as far as I can tell, that we re-used the state transitions allowed by DefineOwnProperty, which allows the transition from non-configurable,writable (-c+w) to non-configurable,non-writable (-c-w) and which is unwanted here, as Claude rightfully concludes *[to better understand the state transitions involved, see MarkM's lucid state transition chart of property attributes <es3.1:attribute_states, es3.1:attribute_states>. I wish
this diagram were in the spec btw, once you know how to read it, it is significantly easier to understand than the DefineOwnProperty algorithm itself]*
I like the simplicity of Claude's suggestion, but I think that check is too tight. Technically if the descriptors are both -c+w data descriptors, their value attributes need not be identical to honor the spec invariants. Instead I would propose to tackle the problem head-on and explicitly disallow a proxy from reporting a -c-w property if the corresponding target property is -c+w. We already have a similar check in place in precisely those two algorithms to test if the configurable attribute matches. It is just a matter of tightening that check to also verify the writable attribute.
This would entail the following changes to the ES2015 spec (new or modified text in bold):
9.5.5 [[GetOwnProperty]] (P) ... 22. If resultDesc.[[Configurable]] is false, then a. If targetDesc is undefined or targetDesc.[[Configurable]] is true, then i. Throw a TypeError exception.
- b. If resultDesc.[[Writable]] is false and targetDesc.[[Writable]] is true, then*
-
i. Throw a TypeError exception.*
NOTE [[GetOwnProperty]] for proxy objects enforces the following invariants:
...
-
- A property cannot be reported as non-configurable, non-writable if it exists as a non-configurable, writable own property on the target object.*
9.5.6 [[DefineOwnProperty]] (P, Desc) ... 20. Else targetDesc is not undefined, a. If IsCompatiblePropertyDescriptor(extensibleTarget, Desc , targetDesc) is false, throw a TypeError exception.
- b. If settingConfigFalse is true*
-
i. If targetDesc.[[Configurable]] is true, throw a TypeError
exception.*
-
ii. If Desc.[[Writable]] is false and targetDesc.[[Writable]] is
true, throw a TypeError exception.*
NOTE [[DefineOwnProperty]] for proxy objects enforces the following invariants:
...
-
- A property cannot be successfully set to non-configurable, non-writable if the corresponding own property of the target object is non-configurable, writable.*
WDYT?
, Tom
2016-08-08 15:37 GMT+02:00 Claude Pache <claude.pache at gmail.com>:
On Mon, Aug 8, 2016 at 1:35 PM, Tom Van Cutsem <tomvc.be at gmail.com> wrote:
Claude's additional example is indeed evidence that Object.freeze is not to blame, but rather that the invariant checks of [[GetOwnPropertyDescriptor]] and [[DefineOwnProperty]] are too weak. The culprit is, as far as I can tell, that we re-used the state transitions allowed by DefineOwnProperty, which allows the transition from non-configurable,writable (-c+w) to non-configurable,non-writable (-c-w) and which is unwanted here, as Claude rightfully concludes [to better understand the state transitions involved, see MarkM's lucid state transition chart of property attributes <es3.1:attribute_states, es3.1:attribute_states>. I wish this diagram were in the spec btw, once you know how to read it, it is significantly easier to understand than the DefineOwnProperty algorithm itself]
I would like to see it in the spec too.
Given a Proxy that pretends to be in state A while its target is observably in state B, and assuming that the target satisfies the Invariants of the Essential Internal Methods 6.1.7.3, I claim that, in order to force the Proxy to satisfy those Invariants, it is necessary and sufficient to check that the two following conditions hold:
- it is legal for an object to pass from state A to state B; and,
- it is legal for an object to pass from state B to state A.
Because I am too lazy to write the proof just now, I cowardly leave it as an exercice to the reader. Meanwhile, that principle may be used to audit the robustness of the Proxy specification. I have found the following bug in Proxy.[Delete] by applying the above principle to:
- state A: nonexistent property on a nonextensible object;
- state B: existent own property on a nonextensible object.
Resurrection of a successfully deleted property on a nonextensible object:
var target = Object.preventExtensions({ x: 1 })
var proxy = new Proxy(target, {
deleteProperty() { return true }
})
Object.isExtensible(proxy) // false
delete proxy.x // true
proxy.hasOwnProperty('x') // true
After a first scan, I haven't found other bugs in the essential methods of Proxy, than that one and the missing nonconfigurable-but-writable check in [[GetOwnPropertyDescriptor]] and [[DefineOwnProperty]] already mentioned in that thread.
I plan to propose a minimal patch (i.e., just adding the missing checks) in a few days.
Le 8 août 2016 à 22:35, Tom Van Cutsem <tomvc.be at gmail.com> a écrit :
Claude's additional example is indeed evidence that Object.freeze is not to blame, but rather that the invariant checks of [[GetOwnPropertyDescriptor]] and [[DefineOwnProperty]] are too weak. The culprit is, as far as I can tell, that we re-used the state transitions allowed by DefineOwnProperty, which allows the transition from non-configurable,writable (-c+w) to non-configurable,non-writable (-c-w) and which is unwanted here, as Claude rightfully concludes [to better understand the state transitions involved, see MarkM's lucid state transition chart of property attributes <es3.1:attribute_states, es3.1:attribute_states>. I wish this diagram were in the spec btw, once you know how to read it, it is significantly easier to understand than the DefineOwnProperty algorithm itself]
I like the simplicity of Claude's suggestion, but I think that check is too tight. Technically if the descriptors are both -c+w data descriptors, their value attributes need not be identical to honor the spec invariants. Instead I would propose to tackle the problem head-on and explicitly disallow a proxy from reporting a -c-w property if the corresponding target property is -c+w. We already have a similar check in place in precisely those two algorithms to test if the configurable attribute matches. It is just a matter of tightening that check to also verify the writable attribute.
This would entail the following changes to the ES2015 spec (new or modified text in bold):
9.5.5 [[GetOwnProperty]] (P) ... 22. If resultDesc.[[Configurable]] is false, then a. If targetDesc is undefined or targetDesc.[[Configurable]] is true, then i. Throw a TypeError exception. b. If resultDesc.[[Writable]] is false and targetDesc.[[Writable]] is true, then i. Throw a TypeError exception.
NOTE [[GetOwnProperty]] for proxy objects enforces the following invariants:
...
- A property cannot be reported as non-configurable, non-writable if it exists as a non-configurable, writable own property on the target object.
9.5.6 [[DefineOwnProperty]] (P, Desc) ... 20. Else targetDesc is not undefined, a. If IsCompatiblePropertyDescriptor(extensibleTarget, Desc , targetDesc) is false, throw a TypeError exception. b. If settingConfigFalse is true i. If targetDesc.[[Configurable]] is true, throw a TypeError exception. ii. If Desc.[[Writable]] is false and targetDesc.[[Writable]] is true, throw a TypeError exception.
Rather:
b (unchanged). If settingConfigFalse is true and targetDesc.[[Configurable]] is true, throw a TypeError exception. c. If targetDesc.[[Configurable]] is false, then i. If targetDesc.[[Writable]] is true and Desc.[[Writable]] is false, throw a TypeError exception.
because we need the additional check on [[Writable]] when the property is nonconfigurable ("targetDesc.[[Configurable]] is false"), not only when the property is made nonconfigurable ("settingConfigFalse is true").
Claude, I don't see how non-extensibility and deleting properties are connected.
Le 9 août 2016 à 18:58, Raul-Sebastian Mihăilă <raul.mihaila at gmail.com> a écrit :
Claude, I don't see how non-extensibility and deleting properties are connected.
The issue is not deleting per se. The issue is that a property appears to be non-existent (because successfully deleted), and later existent again, which should not be allowed on non-extensible objects.
I think too much validation is not a good idea. Let the proxy lie. If you don't, what is the purpose of Proxy? I have a case where I wanted to force every new property to be read-only through a Proxy so that, once created, they can no longer change. But I get "TypeError" because of such a validation:
"use strict";
const proxy = new Proxy({}, {
set(target, property, value, receiver) {
if (Object.prototype.hasOwnProperty.call(target, property)) return false;
Object.defineProperty(target, property, {configurable: false, enumerable: true, writable: false, value: value});
return true;
},
defineProperty(target, property, descriptor) {
if (Object.prototype.hasOwnProperty.call(target, property)) return false;
descriptor = Object.assign({}, descriptor, {configurable: false, enumerable: true});
if (!descriptor.get && !descriptor.set) descriptor.writable = false;
Object.defineProperty(target, property, descriptor);
return true;
},
});
proxy.a = 1;
proxy.a = 2; // TypeError: 'set' on proxy: trap returned falsish for property 'a'
Object.defineProperty(proxy, 'b', {value: 3});
Object.defineProperty(proxy, 'b', {value: 4}); // TypeError: 'defineProperty' on proxy: trap returned falsish for property 'b'
On Tue, Aug 9, 2016 at 10:58 AM, doodad-js Admin <doodadjs at gmail.com> wrote:
I think too much validation is not a good idea. Let the proxy lie. If you don't, what is the purpose of Proxy?
research.google.com/pubs/pub40736.html
Of course, we can still agree that too much, by definition, is not a good idea ;).
I have a case where I wanted to force every new property to be read-only through a Proxy so that, once created, they can no longer change. But I get "TypeError" because of such a validation:
"use strict"; const proxy = new Proxy({}, { set(target, property, value, receiver) { if (Object.prototype.hasOwnProperty.call(target, property)) return false; Object.defineProperty(target, property, {configurable: false, enumerable: true, writable: false, value: value}); return true; }, defineProperty(target, property, descriptor) { if (Object.prototype.hasOwnProperty.call(target, property)) return false; descriptor = Object.assign({}, descriptor, {configurable: false, enumerable: true}); if (!descriptor.get && !descriptor.set) descriptor.writable = false; Object.defineProperty(target, property, descriptor); return true; }, }); proxy.a = 1; proxy.a = 2; // TypeError: 'set' on proxy: trap returned falsish for property 'a' Object.defineProperty(proxy, 'b', {value: 3}); Object.defineProperty(proxy, 'b', {value: 4}); // TypeError: 'defineProperty' on proxy: trap returned falsish for property 'b'
I don't understand the purpose of this code. From your description, don't you want these TypeErrors? Could you show a hypothetical test case that, if passed, demonstrates what you are actually trying to accomplish?
“Of course, we can still agree that too much, by definition, is not a good idea ;)”
Thanks, that was my point. But my example was not appropriated. It occurs that I’ve misread MDN:
Source: developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/handler/set, developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Proxy/handler/set
“If the following invariants are violated, the proxy will throw a developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypeError TypeError:”
But just below :
“In strict mode, a false return value from the set handler will throw a developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/TypeError TypeError exception.”
By reviewing more carefully the Proxy internal methods, I have found the following glitch in [[OwnPropertyKeys]]. tc39.github.io/ecma262/#sec-proxy-object-internal-methods-and-internal-slots-ownpropertykeys, tc39.github.io/ecma262/#sec-proxy-object-internal-methods-and-internal-slots-ownpropertykeys
Testcase:
var base = Object.freeze({x: 1})
var target = new Proxy(base, {
ownKeys(o) {
return ['x', 'x']
}
})
Object.keys(target) // ['x', 'x'] // ok; according to ECMA262, Chrome, and Safari TP; buggy in Firefox
var proxy = new Proxy(target, {
ownKeys(o) {
return ['x']
}
})
Object.keys(proxy) // !!! throws in ECMA262, Chrome, and Safari TP; should be fixed so that it returns ['x']
The issue is in step 17:
- Repeat, for each key that is an element of targetNonconfigurableKeys, a. If key is not an element of uncheckedResultKeys, throw a TypeError exception. b. Remove all occurrences of key from uncheckedResultKeys.
because targetNonconfigurableKeys contains a list with twice the value 'x', and, at the second iteration, a TypeError is thrown in step a.
The solution is simple: step 14 should guard against duplicate keys when constructing targetNonconfigurableKeys and targetConfigurableKeys. Additional text marked in bold:
- Repeat, for each element key of targetKeys, I. If key is not already an element of targetNonconfigurableKeys or targetConfigurableKeys, then a. Let desc be ? target.[GetOwnProperty]. b. If desc is not undefined and desc.[[Configurable]] is false, then i. Append key as an element of targetNonconfigurableKeys. c. Else, ii. Append key as an element of targetConfigurableKeys.
―Claude
Thanks Claude for your careful review, and for trying to articulate a more general principle behind the invariant checks. The lack of such crisp principles makes it (too) difficult to verify whether all necessary checks are in place.
To clarify, in the case of "update" MOP operations such as defineProperty and deleteProperty, your "state B" (the observable state of the target), presumably is the state of the target after the proxy trap has executed, i.e. state B should already reflect the update.
Both the original issue and your new test case with deleteProperty are cases where the proxy pretends to have altered the target but has in fact not. In a later interaction, the proxy reverts to the original state. This violates a client's expectations about when state transitions occur.
Interestingly, the opposite problem, where a proxy does alter the target as requested, but then reports that the update failed, is allowed, even though this technically also violates a client's expectations about what state transitions have occurred. But a failed update leads to a TypeError anyway.
At this point, we should probably iron out the details of the fix in a GitHub issue or on bugs.ecmascript.org.
Cheers, Tom
2016-08-09 14:44 GMT+02:00 Claude Pache <claude.pache at gmail.com>:
A relevant github issue regarding [[OwnPropertyKeys]] is tc39/ecma262#461 . I also wish [[OwnPropertyKeys]] didn't allow duplicates.
Only in a Github issue please :-) we shouldn't be using bugs.ecmascript.org for new issues.
This is my first time participating in a discussion, so I thought I would start off with an easy on to get my feet wet: Reading this I am not familiar with what "MOP" is?
Thank you, Rob
FYI: If you have any prayer requests, add them to your reply and I will be sure to pray for the needs you have.
Rob Simpson @pertrai1 robesimpson.com
*Love is patient, love is kind. It does not envy, it does not boast, it is not proud. It does not dishonor others, it is not self-seeking, it is not easily angered, it keeps no record of wrongs. Love does not delight in evil but rejoices with the truth. It always protects, always trusts, always hopes, always perseveres. Love never fails. - 1 Corinthians 13:4-8a NIV *
See tc39/ecma262#666, tc39/ecma262#666
(I swear that the issue number is just a hazard.)
Initially posted on github (tc39/ecma262#652), but I think the mailing list is more appropriate.
Usually methods on Object that change some internal state of the objects/properties of the objects either succeed or throw. However, Object.freeze can fail to freeze proxies without throwing:
const target = Object.seal({x: 2}); const proxy = new Proxy(target, { defineProperty() { return true; }, set(target, prop, value) { target[prop] = value; } }); Object.freeze(proxy); console.log(proxy.x); // 2 proxy.x = 100; console.log(proxy.x); // 100Should this be allowed?
Initially posted on github (https://github.com/tc39/ecma262/issues/652), but I think the mailing list is more appropriate. Usually methods on Object that change some internal state of the objects/properties of the objects either succeed or throw. However, Object.freeze can fail to freeze proxies without throwing: ```js const target = Object.seal({x: 2}); const proxy = new Proxy(target, { defineProperty() { return true; }, set(target, prop, value) { target[prop] = value; } }); Object.freeze(proxy); console.log(proxy.x); // 2 proxy.x = 100; console.log(proxy.x); // 100 ``` Should this be allowed? -------------- next part -------------- An HTML attachment was scrubbed... URL: <http://mail.mozilla.org/pipermail/es-discuss/attachments/20160806/c5c28d0a/attachment.html>