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 <[email protected]> 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ă <[email protected]> > : > >> 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 >> [email protected] >> https://mail.mozilla.org/listinfo/es-discuss >> >> >
_______________________________________________ es-discuss mailing list [email protected] https://mail.mozilla.org/listinfo/es-discuss
