600677 - Object.freeze(proxy) does not call the proxy handler's fix() trap

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect, P2)

Description

[Tom Van Cutsem]

Calling any of Object.preventExtensions, Object.seal or Object.freeze on a trapping harmony:proxy does not call the handler's fix() trap. Instead, the handler's getOwnPropertyNames() trap is invoked, and for each returned property name, a getOwnPropertyDescriptor(name) trap is invoked. It appears as if the proxy implementation is trying to shortcut the fix() trap and trying to reconstruct a property descriptor map of the object directly.

Testcase:
var p = Proxy.create({ fix: function() { return {foo:2}; } });
Object.preventExtensions(p);
=> TypeError: getOwnPropertyNames is not a function

Comment 1

[Andreas Gal :gal]

waldo, you guys didn't wire up the proxy's fix infrastructure to the new object op

Comment 2

[Andreas Gal :gal]

(suggesting waldo for this but I can take back after compartments landed if you are overloaded)

        proxy_DeleteProperty,
        NULL,       /* enumerate       */
	NULL,       /* typeof          */
        proxy_TraceObject,
	NULL,       /* fix             */
	NULL,       /* thisObject      */
        proxy_Finalize, /* clear */
    }

Comment 3

[David Bruant]

Is the test case valid? {foo:2} doesn't look like a valid propertyDescriptor map.
Shouldn't we rather test with {foo: {value:2}}?
The bug is still occuring in FF4b11.

Tom's description sounds familiar. It sounds like the called method is the one defined in handlerMaker (http://wiki.ecmascript.org/doku.php?id=harmony:proxies#examplea_no-op_forwarding_proxy):
fix:function() {
     if (Object.isFrozen(obj)) {
       return Object.getOwnPropertyNames(obj).map(function(name) {
         return Object.getOwnPropertyDescriptor(obj, name);
       });
     }
     // As long as obj is not frozen, the proxy won't allow itself to be fixed
     return undefined; // will cause a TypeError to be thrown
   }

Comment 4

[Andreas Gal :gal]

This is a really trivial fix. Can we try to get a patch for this?

Comment 5

[Brendan Eich [:brendan]]

Want this in the release branching next week...

/be

Comment 6

[Josh Matthews [:jdm]]

Attachment: Add fix hook to proxies.

This passes the modified test from comment 3. Dunno if I'm getting the resolved props right, though.

Comment 7

[Andreas Gal :gal]

Comment on attachment 526507 [details] [diff] [review]
Add fix hook to proxies.

You want JSITER_OWNLY and JSITER_HIDDEN, since we want non-enumerable properties as well.

Comment 8

[Josh Matthews [:jdm]]

Attachment: Add fix hook to proxies.

Comment 9

[Josh Matthews [:jdm]]

Attachment: Add fix hook to proxies.

Rebased.

Comment 10

[Mounir Lamouri (:mounir)]

http://hg.mozilla.org/tracemonkey/rev/6947160cc064

Comment 11

[Tom Van Cutsem]

Most of the tests related to fixing proxies on <http://hg.ecmascript.org/tests/harmony/> now pass, except for one, which has to do with recursive fixing:

    var proxy = Proxy.create({
      fix: function() {
        Object.preventExtensions(proxy); // triggers 'fix()' recursively
        return {};
      }
    });
    Object.preventExtensions(proxy); // triggers 'fix'

Result: InternalError: too much recursion
Expected: a TypeError

We reached consensus on this behavior in a TC39 meeting last year. Admittedly, it's well hidden on the semantics page: <http://wiki.ecmascript.org/doku.php?id=harmony:proxies_semantics>

"Note: recursive fixing should be disallowed. If fix() is called on a proxy handler while the same proxy is already being fixed (an earlier call to fix() is already on the stack), a TypeError should be thrown. "

Comment 12

[Josh Matthews [:jdm]]

Tom, thanks for checking that.  Could you file a new bug and CC me, please?

Comment 13

[Tom Van Cutsem]

Follow-up bug is here: https://bugzilla.mozilla.org/show_bug.cgi?id=652803

Comment 14

[Brendan Eich [:brendan]]

Comment on attachment 526530 [details] [diff] [review]
Add fix hook to proxies.

Please carry along r+. Thanks,

/be

Comment 15

[Tom Van Cutsem]

The current patch doesn't seem to fix the issue for function proxies:

Testcase:
var handler = {
  get: function(rcvr, name) {
    if (name === 'isTrapping') return true;
  },
  fix: function() {
    return {
      isTrapping: { value: false }
    };
  }
};
    
var p = Proxy.createFunction(handler, function(){}, function(){});
assert(p.isTrapping);
Object.preventExtensions(p);
assert(! p.isTrapping);

The call to preventExtensions still fails. It works fine for object proxies though (replace Proxy.createFunction by Proxy.create)

Comment 16

[Josh Matthews [:jdm]]

Boo. Looks like I forgot to change

>1146         NULL,             /* fix             */

in FunctionProxyClass. Could someone whip that up?

Comment 17

[Tom S. (please needinfo tschuster)]

Long in
http://hg.mozilla.org/mozilla-central/rev/6947160cc064

Comment 18

[David Bruant]

I'd like to reopen, because the test case is not entirely relevant

(In reply to comment #17)
> http://hg.mozilla.org/mozilla-central/rev/6947160cc064
-----
> var p = Proxy.create({ fix: function() { return {foo: {value: 2} }; } });
> Object.preventExtensions(p);
> reportCompare(p.foo, 2, "property exists");
> p.foo = 3;
> reportCompare(p.foo, 2, "proxy is frozen");
p.foo === 2 the second time not because the proxy is frozen (it's actually not /frozen/, but extension is prevented), but rather because foo is non-writable (implicit in the returned property descriptor).
I think the first test case should stop before "p.foo = 3" and that another test case should test that p is non extensible (a call to Object.isExtensible should be enough).

Comment 19

[Tom S. (please needinfo tschuster)]

Go ahead :) I am always for test cases. I think you should also do something like
p.newProperty = 'bla';
assertEq('newProperty' in p, false);

Comment 20

[Tom S. (please needinfo tschuster)]

(In reply to Josh Matthews [:jdm] from comment #16)
> Boo. Looks like I forgot to change
> 
> >1146         NULL,             /* fix             */
> 
> in FunctionProxyClass. Could someone whip that up?

This still hasen't been fixed, and the tests are incomplete, please somebody with Proxies fix that.

Comment 21

[David Bruant]

I think a better use of everyone's time would be to work on direct proxies (bug 703537) which solves design issues of current proxies.

Comment 22

[Jeff Walden [:Waldo]]

Yeah, I think direct proxies are the way, the truth, and the life here.