Object.freezing proxies should freeze or throw?

[Mark S. Miller]

Hi Raul, 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.


On Sun, Aug 7, 2016 at 6:49 AM, Raul-Sebastian Mihăilă <
raul.mihaila at gmail.com> wrote:

> 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
>>>
>>>
>>
>
> _______________________________________________
> es-discuss mailing list
> es-discuss at mozilla.org
> https://mail.mozilla.org/listinfo/es-discuss
>
>


-- 
    Cheers,
    --MarkM
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.mozilla.org/pipermail/es-discuss/attachments/20160807/f9165997/attachment.html>