Object.freezing proxies should freeze or throw?
Allen Wirfs-Brock
allen at wirfs-brock.com
Sun Aug 7 22:59:15 UTC 2016
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.
Allen
> On Aug 7, 2016, at 6:33 AM, Tom Van Cutsem <tomvc.be at gmail.com> wrote:
>
> 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:
>
> 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 <mailto:raul.mihaila at gmail.com>>:
> Initially posted on github (https://github.com/tc39/ecma262/issues/652 <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?
>
> _______________________________________________
> es-discuss mailing list
> es-discuss at mozilla.org <mailto:es-discuss at mozilla.org>
> https://mail.mozilla.org/listinfo/es-discuss <https://mail.mozilla.org/listinfo/es-discuss>
>
>
> _______________________________________________
> es-discuss mailing list
> es-discuss at mozilla.org
> https://mail.mozilla.org/listinfo/es-discuss
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.mozilla.org/pipermail/es-discuss/attachments/20160807/4191a368/attachment-0001.html>
More information about the es-discuss
mailing list