Object.freezing proxies should freeze or throw?

[Tom Van Cutsem]

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/2d244dff/attachment.html>