Tom Van Cutsem Reporter
	Description • 14 years ago

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

	Andreas Gal :gal
	Comment 1 • 14 years ago

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

	Andreas Gal :gal
	Comment 2 • 14 years ago

(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 */
    }

	David Bruant
	Comment 3 • 14 years ago

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
   }

	Andreas Gal :gal
	Comment 4 • 14 years ago

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

	Brendan Eich [:brendan]
	Comment 5 • 13 years ago

Want this in the release branching next week...

/be

	Josh Matthews [:jdm]
	Comment 6 • 13 years ago

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

	Andreas Gal :gal
	Comment 7 • 13 years ago

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.

	Josh Matthews [:jdm]
	Comment 9 • 13 years ago

Rebased.

	Mounir Lamouri (:mounir)
	Comment 10 • 13 years ago

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

	Tom Van Cutsem Reporter
	Comment 11 • 13 years ago

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. "

	Josh Matthews [:jdm]
	Comment 12 • 13 years ago

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

	Tom Van Cutsem Reporter
	Comment 13 • 13 years ago

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

	Brendan Eich [:brendan]
	Comment 14 • 13 years ago

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

Please carry along r+. Thanks,

/be

	Tom Van Cutsem Reporter
	Comment 15 • 13 years ago

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)

	Josh Matthews [:jdm]
	Comment 16 • 13 years ago

Boo. Looks like I forgot to change

>1146         NULL,             /* fix             */

in FunctionProxyClass. Could someone whip that up?

	Tom S [:evilpie]
	Comment 17 • 13 years ago

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

	David Bruant
	Comment 18 • 13 years ago

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).

	Tom S [:evilpie]
	Comment 19 • 13 years ago

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

	Tom S [:evilpie]
	Comment 20 • 13 years ago

(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.

	David Bruant
	Comment 21 • 13 years ago

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.

	Jeff Walden [:Waldo]
	Comment 22 • 12 years ago

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