Object.freezing proxies should freeze or throw?

[Raul-Sebastian Mihăilă]

Would be a good idea to do the test for any kind of exotic objects
(including host defined ones), not only for proxies?

On Sun, Aug 7, 2016 at 4:33 PM, 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>
> :
>
>> 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?
>>
>> _______________________________________________
>> 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/5a26bf2a/attachment-0001.html>