Bug 546590 (harmony:proxies)

Implement Harmony Proxies [ES6?]

RESOLVED FIXED in mozilla1.9.3a5

Status

()

Core
JavaScript Engine
--
enhancement
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: gal, Assigned: gal)

Tracking

(Depends on: 1 bug, Blocks: 3 bugs, {dev-doc-complete})

Trunk
mozilla1.9.3a5
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: fixed-in-tracemonkey, URL)

Attachments

(2 attachments, 64 obsolete attachments)

101.59 KB, patch
mrbkap
: review+
Details | Diff | Splinter Review
1.83 KB, patch
Details | Diff | Splinter Review
(Assignee)

Description

8 years ago
This is useful for everything from information flow to narcissus to wrappers.

http://wiki.ecmascript.org/doku.php?id=harmony:proxies&s=proxy
(Assignee)

Comment 1

8 years ago
Created attachment 427281 [details] [diff] [review]
very preliminary totally incomplete patch
Assignee: general → gal
(Assignee)

Comment 2

8 years ago
Created attachment 427282 [details] [diff] [review]
this time with missing files, wip
Attachment #427281 - Attachment is obsolete: true
Comment on attachment 427282 [details] [diff] [review]
this time with missing files, wip

>+ * The Initial Developer of the Original Code is
>+ *   Andreas Gal <gal@mozilla.com>
>+ *
>+ * Contributor(s):

This is wrong. Employees of the Mozilla Corporation should always list the original author (initial developer) of any file created because of work as the Mozilla Foundation. You can list yourself under the contributors section.
(Assignee)

Comment 4

8 years ago
You must have a lot of time on your hands. This is a wip patch. The word "Foundation" doesn't appear in js/src/*. I will copy what jstypedarray.cpp does, the latest file to be added to js. I trust vlad that he knows that he is doing (it says "Mozilla Corp").
(Assignee)

Comment 5

8 years ago
Created attachment 427284 [details] [diff] [review]
with new and improved copyright disclaimer
Attachment #427282 - Attachment is obsolete: true
(In reply to comment #4)
> You must have a lot of time on your hands. This is a wip patch. The word
> "Foundation" doesn't appear in js/src/*. I will copy what jstypedarray.cpp
> does, the latest file to be added to js. I trust vlad that he knows that he is
> doing (it says "Mozilla Corp").

See bug 507387.
(Assignee)

Comment 7

8 years ago
Created attachment 427289 [details] [diff] [review]
patch with spanking new and yet better copyright header, and also switch to JSObjectOps since the JSClass interface isn't powerful enough
Attachment #427284 - Attachment is obsolete: true
(Assignee)

Comment 8

8 years ago
Created attachment 427301 [details] [diff] [review]
mostly complete proxy implementation, but only partially debugged
Attachment #427289 - Attachment is obsolete: true
(Assignee)

Comment 9

8 years ago
Function proxies are not implemented yet. Shouldn't be too hard.

for (x in p) does a get __iterator__. I have no clue why. Shouldn't that all go through ->enumerate() ? Our iterator protocol continues to amaze me.

Otherwise this is surprisingly simple.

Here is some example code:

var handler = {
    defineOwnProperty: function(name, pd) {
	print("defineOwnProperty " + name);
	print(uneval(pd));
    },
    delete: function(name) {
	print("delete " + name);
	return true;
    },
    get: function(obj,name) {
	print("get " + name);
	return "foo";
    },
    enumerate: function() {
	return ["foo", "bar"];
    }
};

var p = Proxy.create(handler);

p.x = 1;
print("p.x = " + p.x);
delete p.x;
print("x" in p);
for (x in p)
    print(x);


More tomorrow.
(Assignee)

Comment 10

8 years ago
Created attachment 427369 [details] [diff] [review]
make enumeration work
Attachment #427301 - Attachment is obsolete: true
(Assignee)

Comment 11

8 years ago
Created attachment 427381 [details] [diff] [review]
implement function proxies (largly untested)

invoke trap is still missing
Attachment #427369 - Attachment is obsolete: true
(Assignee)

Comment 12

8 years ago
Have to talk to Brendan about ->invoke, that looks a bit weird and unnecessary (could just return a generated proxy that intercepts it, right now I have to generate the proxy on the fly to implement invoke).

Also, so far this looks largely overhead-free (the spec is asking this question).
(In reply to comment #12)
> Have to talk to Brendan about ->invoke, that looks a bit weird and unnecessary

For a proxy p, p.m() should not have to cons up a new proxy for m that implements the call trap, for every possible m (there could be many; there could be an unknown number based on reflecting methods from a peer language VM or system).

> (could just return a generated proxy that intercepts it, right now I have to
> generate the proxy on the fly to implement invoke).

Not sure what you mean here.

> Also, so far this looks largely overhead-free (the spec is asking this
> question).

Making a new callable proxy for every p.m given a proxy p obviously costs more than making zero proxies and invoking some underlying method identified by m.

Did you use the jsclock.{h,cpp} code from the ssllab.org clone of tracemonkey? I guess that was easy enough to recreate, but I hate to waste my own time on a wheel that gets reinvented...

/be
Er, jscloak.{h,cpp}.

/be
Now that I remember more, jscloak is all about value types, or wants to be. Information flow wants to label arbitrary values, not just objects. Probably we should just have you hack up a value types implementation overnight :-).

/be
This is cool -- fast work. I will review after you show up for lunch.

/be
(Assignee)

Comment 17

8 years ago
Created attachment 427469 [details] [diff] [review]
patch

- move call and construct hooks into the handler
- stick with create/createFunction (this mixes the two API proposals, keeping typeof sane, but also unifying the way traps are invoked)
- given up on ->invoke() for now, it looks too dangerous wrt evaluation order
Attachment #427381 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Depends on: 518663
(Assignee)

Comment 18

8 years ago
We don't implement getOwnPropertyNames(aProxy), I will wire it up once that's fixed.
Alias: harmony:proxies
(Assignee)

Comment 19

8 years ago
Created attachment 427489 [details] [diff] [review]
patch

Wire up reflective API in Object.* to call the proper proxy handler traps.
Attachment #427469 - Attachment is obsolete: true
(Assignee)

Comment 20

8 years ago
I am trying to implement fix. There is a slight issue with setting __proto__ from the recipe returned by fix. A handler could return cyclic parent or proto relationships in the recipe so we might want to check for that. This is expensive and requires a GC heap walk because we maintain the identity of the proxy (someone might already have a reference to it, causing the cycle).

newobj = fix(proxy)

would be a lot easier to do.

This is in general a problem for proxies. get(__proto__) can also produce a cycle, but we can insert a recursion backstop in the proxy code. If we fix the proxy to a regular JS object (ObjectClass), the special case code is no longer hit and our generic hot path assumes this can't happen, because setting proto or parent is supposed to check for cycles. If we invoke that code for every fix, fixing proxies will be really really slow.

An alternative would be to leave proxies unchanged and merely instantiate a new special fixed handler for them. That would make proxy objects behave as intended, and check for their own cycles without affecting the native JS object fast path. However, this would also make proxy objects as slow as trapping proxies even after fixing.
(Assignee)

Comment 21

8 years ago
Created attachment 427506 [details] [diff] [review]
patch

Fixed a couple rooting bugs and cleanup.
Attachment #427489 - Attachment is obsolete: true
(Assignee)

Comment 22

8 years ago
Created attachment 427512 [details] [diff] [review]
add error reporting
Attachment #427506 - Attachment is obsolete: true
(Assignee)

Comment 23

8 years ago
Created attachment 427519 [details] [diff] [review]
patch

Implemented fixing of proxies (not wired up yet).
Attachment #427512 - Attachment is obsolete: true
(Assignee)

Comment 24

8 years ago
Created attachment 427520 [details] [diff] [review]
patch

Looks like we don't implement seal/freeze/preventExtension yet, so I added Proxy.fix(proxy) to test the code. Also added Proxy.isTrapping(obj) as descriped in the spec. This is all mostly untested, so use with care.
Attachment #427519 - Attachment is obsolete: true
(Assignee)

Comment 25

8 years ago
Created attachment 427525 [details] [diff] [review]
patch

Insist on parameters being objects instead of going the valueOf() route. The latter will need some fixing. Added the NoopHandler test case from the spec (run test-proxy.js to see proxies in action).
Attachment #427520 - Attachment is obsolete: true
(Assignee)

Comment 26

8 years ago
Its unclear to me how fix() is supposed to handle the [[Call]] property of functions, in other words how does the code get handed around/copied into the fixed version?
(Assignee)

Comment 27

8 years ago
(Note: currently fix always produces an object, and never functions, until #26 is clarified).
(In reply to comment #20)
> I am trying to implement fix. There is a slight issue with setting __proto__
> from the recipe returned by fix.

Don't support setting __proto__, please. It's an abomination unless done at the birth of an object, a la Object.create (say, in an object initialiser).

> A handler could return cyclic parent or proto
> relationships in the recipe so we might want to check for that.

So don't support setting __proto__, and make getting __proto__ turn into Object.getPrototypeOf, which per the spec returns the proto parameter to Proxy.create (or null, or Function.prototype, accordingly).

/be
(Assignee)

Comment 29

8 years ago
Created attachment 427538 [details] [diff] [review]
patch

Keep around proto that was originally selected in create() (or the value of Function.prototype at the time createFunction() is called), and use it for fix(). Inherit the proxy's original parent into the fixed object.

Note that if getOwnPropertyNames returns __proto__ and __parent__, these would be overwritten, with all then negative performance impact described above. Maybe we should filter those? Or let the handler filter them?

I am still not clear how to handle functions properly here.
Attachment #427525 - Attachment is obsolete: true
(In reply to comment #29)
> Keep around proto that was originally selected in create() (or the value of
> Function.prototype at the time createFunction() is called), and use it for
> fix(). Inherit the proxy's original parent into the fixed object.

Aren't you supposed to keep that proto parameter around anyway? How would you ever lose that if __proto__ setting doesn't work?

IOW, no need for JSSLOT_FIX_PROTO given JSSLOT_PROTO.

> Note that if getOwnPropertyNames returns __proto__ and __parent__, these would
> be overwritten,

These are only in Object.prototype, and they're non-configurable. How would they be overwritten?

> with all then negative performance impact described above.
> Maybe we should filter those? Or let the handler filter them?

Certainly could filter them!

> I am still not clear how to handle functions properly here.

We talked on IRC, resolution AIUI is to have fix create a callable object that masquerades as a function object (it is of an uninitialized, anonymous class whose name is "Function"), having the call and construct traps hooked as its internal [[Call]] and [[Construct]], as it were, and of course frozen as if by Object.freeze from birth.

/be
(Assignee)

Comment 31

8 years ago
Created attachment 427552 [details] [diff] [review]
patch

Fixed the parent/proto mess. Both are now set at construction time and stay fixed during fix(). I also have to remove the __proto__ lookup and use getProto() instead. Patch coming up.
Attachment #427538 - Attachment is obsolete: true
(Assignee)

Comment 32

8 years ago
Created attachment 427554 [details] [diff] [review]
patch

Rely on the proto supplied during proxy construction. This eliminates the double trap in lookupProperty, which was really nasty.
Attachment #427552 - Attachment is obsolete: true
(Assignee)

Comment 33

8 years ago
#30:

1) I am not going to try to filter them for now. A handler would have to explicitly name them. I am assuming our forthcoming getOwnPropertyNames() implementation will not enumerate __proto__ and __parent__ (here is hoping).

2) I plan on making fixed function proxies cloned functions pointing to a callable object. There should be no way for the callable object to leak out, so I think they actually don't have to masquerade as functions. You can't do typeof on them, only on the now fixed proxy, which is of class FunctionClass. I have to implement and debug through this to confirm.

There is some enormously ugly code in that path that is totally unclear to me:

        /*
         * XXX this makes no sense -- why convert to function if clasp->call?
         * XXX better to call that hook without converting
         *
         * FIXME bug 408416: try converting to function, for API compatibility
         * if there is a call op defined.
         */
        if ((ops == &js_ObjectOps) ? clasp->call : ops->call) {
            ok = clasp->convert(cx, funobj, JSTYPE_FUNCTION, &v);
            if (!ok)
                goto out2;

            if (VALUE_IS_FUNCTION(cx, v)) {
                /* Make vp refer to funobj to keep it available as argv[-2]. */
                *vp = v;
                funobj = JSVAL_TO_OBJECT(v);
                parent = OBJ_GET_PARENT(cx, funobj);
                goto have_fun;
            }
        }

I am not exactly sure what the ramifications of this are for what I am trying to do, but we should nuke this code from orbit or at least come up with an explanation why its there.
(Assignee)

Comment 34

8 years ago
Created attachment 427567 [details] [diff] [review]
patch

Working callable objects. Example see test-proxy.js. We still have tons of VALUE_IS_FUNCTION() specific code paths though. I assume most Function-specific methods like call/apply don't work with callable objects (they probably don't work with function proxies either). I will play around with it more tomorrow.
Attachment #427554 - Attachment is obsolete: true
At least js_fun_call and js_fun_apply do not follow the spec by using IsCallable and not insisting on a function (or an object that converts by defaultValue to a function). Please file blocking bug(s) as needed.

/be
(Assignee)

Updated

8 years ago
Depends on: 492849
(Assignee)

Comment 36

8 years ago
Created attachment 427621 [details] [diff] [review]
patch

Fix a bug in the Function.prototype lookup in createFunction, and implement custom trace hook since Object/FunctionProxy are non-native objects.
Attachment #427567 - Attachment is obsolete: true
(Assignee)

Comment 37

8 years ago
Looks like the VALUE_IS_FUNCTION() issue is a known bug and those places should use js_IsCallable() instead. I will link the bug # if I can find it.
(Assignee)

Updated

8 years ago
Depends on: 547046
(Assignee)

Comment 38

8 years ago
Filed as bug 547046.
(Assignee)

Comment 39

8 years ago
Created attachment 427656 [details] [diff] [review]
patch

Follow the spec and make call and construct arguments to createFunction and store them in the proxy, not the handler.
Attachment #427621 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Depends on: 547086
(Assignee)

Comment 40

8 years ago
Created attachment 427668 [details] [diff] [review]
patch

If no construct trap is given, transparently fall back and allocate a new object using proxy.prototype as proto and run call on that, throwing away the return value if its primitive, otherwise keeping that as newobj.
Attachment #427656 - Attachment is obsolete: true
Blocks: 520778
(Assignee)

Comment 41

8 years ago
Created attachment 427714 [details] [diff] [review]
patch

Add support for invoke trap.

Implemented similar to NoSuchMethod using an object of class MethodTokenClass that gets injected and is caught after the arguments have been evaluated.
Attachment #427668 - Attachment is obsolete: true
(Assignee)

Comment 42

8 years ago
bug 547046 makes call and apply use js_IsCallable() and allows them to be used with proxies.
(Assignee)

Updated

8 years ago
Depends on: 547314
(Assignee)

Comment 43

8 years ago
To write test code for this patch, please use the latest patch in this bug, and the latest patch from bug 547046. All other patches are already in the hg.mozilla.org/tracemonkey tree.
(Assignee)

Comment 44

8 years ago
Created attachment 427908 [details] [diff] [review]
patch

Refreshed patch to tracemonkey-tip.
Attachment #427714 - Attachment is obsolete: true
(Assignee)

Comment 45

8 years ago
I pushed a patch for bug 547046 into the tracemonkey tree, so no other patches are needed any more to make proxies work, just the patch in this bug.
(Assignee)

Comment 46

8 years ago
Created attachment 428320 [details] [diff] [review]
patch

Use the new typeOf hook provided by bug 547314.
Attachment #427908 - Attachment is obsolete: true
(Assignee)

Comment 47

8 years ago
Created attachment 428512 [details] [diff] [review]
patch with first round of fixes for bugs reported by Tom
Attachment #428320 - Attachment is obsolete: true
(Assignee)

Comment 48

8 years ago
Created attachment 428553 [details] [diff] [review]
patch

suppress unexpected __iterator__ property access during iteration
Attachment #428512 - Attachment is obsolete: true
(Assignee)

Comment 49

8 years ago
Created attachment 428834 [details] [diff] [review]
drop invoke() and renaming to match latest spec update
Attachment #428553 - Attachment is obsolete: true
(Assignee)

Comment 50

8 years ago
Created attachment 428836 [details] [diff] [review]
patch

Remove accidental indentation changes to reduce patch footprint.
Attachment #428834 - Attachment is obsolete: true

Comment 51

8 years ago
A test suite for harmony proxies now lives at
http://es-lab.googlecode.com/svn/trunk/tests/harmony

Comment 52

8 years ago
Update: test suite now also available at hg.ecmascript.org/tests/harmony/proxies

Running the test suite on tracemonkey with latest patch results causes the following tests to fail
(I annotated the output with my guess as to why they fail)

[fail] denyFix
 => Proxy.fix doesn't return 'false' when fix() trap returns 'undefined'

[ err] doublelifting-enumerate (ReferenceError: reference to undefined property enumerateOwn)
 => See 'enumerate' test

[ err] doublelifting-getOwnPropertyNames (TypeError: Object.getOwnPropertyNames is not a function)
 => known issue: Object.getOwnPropertyNames does not exist

[skip] doublelifting-getProperty
 => skipped because Object.getPropertyDescriptor does not exist

[ err] doublelifting-invoke (ReferenceError: reference to undefined property get)
 => fails because 'invoke' no longer exists. Not sure whether to remove or not.

[ err] enumeration (TypeError: enumerateOwn is not a function)
 => for-in loops trigger enumerateOwn rather than enumerate. Once a for-in loop hits a proxy,
    it should call its enumerate trap and enumerate those properties. It should not thereafter enumerate the proxy's prototype.

[skip] fixidentity
 => skipped because Object.freeze does not exist

[fail] handlerthis (fix result: expected: false, got: true)
 => See 'denyFix' test

[fail] instanceof (objProxy instanceof TestType failed)
 => objProxy instanceof TestType fails even though objProxy's prototype is TestType.prototype

[ err] invoke (TypeError: get is not a function)
 => fails because 'invoke' no longer exists.

[ err] invokeDelegator (TypeError: has is not a function)
 => apparently, it is not sufficient for a proxy to only provide a 'get' trap when its properties are accessed via an object that delegates to the proxy?

[fail] nonconfigurable (getOwnPropertyDescriptor throws)
 => proxies should not allow get{Own}PropertyDescriptor to return non-configurable property descriptors

[fail] proxytypes ([[Class]] of object proxy is "Object": expected: [object Object], got: [object ObjectProxy])
 => The [[Class]] of an object proxy is "Object". Hence, object proxies should print as [object Object], not as [object ObjectProxy]. Similar for function proxies. Clients should not be able to detect proxies based on the default toString implementation.

[fail] sink (fix sink: expected: false, got: true)
 => See 'denyFix' test

[fail] stratification (prototype of proxy is Object.prototype failed)
 => the return value of Object.getPrototypeOf(proxy) does not correspond to the prototype provided in Proxy.create(handler, proto).

[ err] args-enumerate (TypeError: enumerateOwn is not a function)
 => See 'enumerate' test

[ err] args-getOwnPropertyNames (TypeError: Object.getOwnPropertyNames is not a function)
 => See 'doublelifting-getOwnPropertyNames' test

[skip] args-getProperty
 => See 'doublelifting-getProperty' test

[ err] args-invoke (TypeError: get is not a function)
 => See 'invoke' test

[ err] throws-enumerate (TypeError: enumerateOwn is not a function)
 => See 'enumerate' test

[ err] throws-getOwnPropertyNames (TypeError: Object.getOwnPropertyNames is not a function)
 => See 'doublelifting-getOwnPropertyNames' test

[skip] throws-getProperty
 => See 'doublelifting-getProperty' test

[ err] throws-invoke (TypeError: get is not a function)
 => See 'invoke' test

[done] error: 12  fail: 7  skip: 4  pass: 36  total: 59
(Assignee)

Comment 53

8 years ago
Thanks a lot Tom. I will import the tests and start working my way through them.
(Assignee)

Comment 54

8 years ago
Created attachment 431489 [details] [diff] [review]
patch

Import proxy test suite. Make Proxy.fix return true/false properly.
Attachment #428836 - Attachment is obsolete: true
(Assignee)

Comment 55

8 years ago
> [fail] denyFix
>  => Proxy.fix doesn't return 'false' when fix() trap returns 'undefined'

Fixed.

> [ err] enumeration (TypeError: enumerateOwn is not a function)
>  => for-in loops trigger enumerateOwn rather than enumerate. Once a for-in loop
> hits a proxy,
>     it should call its enumerate trap and enumerate those properties. It should
> not thereafter enumerate the proxy's prototype.

Fixed.

> [fail] handlerthis (fix result: expected: false, got: true)
>  => See 'denyFix' test

Currently 'this' must be an object in our engine. This will change "soon".

> 
> [fail] instanceof (objProxy instanceof TestType failed)
>  => objProxy instanceof TestType fails even though objProxy's prototype is
> TestType.prototype

Fixed. This was a really stupid typo. Nice catch.

> 
> [ err] invoke (TypeError: get is not a function)
>  => fails because 'invoke' no longer exists.
> 
> [ err] invokeDelegator (TypeError: has is not a function)
>  => apparently, it is not sufficient for a proxy to only provide a 'get' trap
> when its properties are accessed via an object that delegates to the proxy?

There is no invoke any more. Otherwise I think this works.

> 
> [fail] nonconfigurable (getOwnPropertyDescriptor throws)
>  => proxies should not allow get{Own}PropertyDescriptor to return
> non-configurable property descriptors
> 
> [fail] proxytypes ([[Class]] of object proxy is "Object": expected: [object
> Object], got: [object ObjectProxy])
>  => The [[Class]] of an object proxy is "Object". Hence, object proxies should
> print as [object Object], not as [object ObjectProxy]. Similar for function
> proxies. Clients should not be able to detect proxies based on the default
> toString implementation.

Brendan and I discussed this. If you want this to be truly transparent, we need another trap, otherwise you can always tell apart [] from a wrapped [] by doing Object.prototype.toString.call on it (wrapped one will return "[object Object]", unwrapped "[object Array]"). I will leave this at ProxyObject for now until we have consensus what to do with it.

> 
> [fail] sink (fix sink: expected: false, got: true)
>  => See 'denyFix' test

Fixed.

> 
> [fail] stratification (prototype of proxy is Object.prototype failed)
>  => the return value of Object.getPrototypeOf(proxy) does not correspond to the
> prototype provided in Proxy.create(handler, proto).

Fixed.

> 
> [ err] args-enumerate (TypeError: enumerateOwn is not a function)
>  => See 'enumerate' test
> 
> [ err] args-getOwnPropertyNames (TypeError: Object.getOwnPropertyNames is not a
> function)
>  => See 'doublelifting-getOwnPropertyNames' test
> 
> [skip] args-getProperty
>  => See 'doublelifting-getProperty' test
> 
> [ err] args-invoke (TypeError: get is not a function)
>  => See 'invoke' test
> 
> [ err] throws-enumerate (TypeError: enumerateOwn is not a function)
>  => See 'enumerate' test
> 
> [ err] throws-getOwnPropertyNames (TypeError: Object.getOwnPropertyNames is not
> a function)
>  => See 'doublelifting-getOwnPropertyNames' test
> 
> [skip] throws-getProperty
>  => See 'doublelifting-getProperty' test
> 
> [ err] throws-invoke (TypeError: get is not a function)
>  => See 'invoke' test
> 
> [done] error: 12  fail: 7  skip: 4  pass: 36  total: 59
(Assignee)

Comment 56

8 years ago
Created attachment 431510 [details] [diff] [review]
patch
Attachment #431489 - Attachment is obsolete: true
(Assignee)

Comment 57

8 years ago
I think the iteration test has a test:

         return delete this.keys[name];
       },
-      enumerate: function() { return keys; }
+      enumerate: function() { return this.keys; }
     }, Object.prototype);
     var proxyResult = iterate(proxy, function(name) {
       delete proxy.b;
     });

We also currently check at every iteration whether the property is still there (has) to support deletion during enumeration. We probably want to stop doing that for performance reasons in general, not just for proxies.
(Assignee)

Comment 58

8 years ago
Actually I take back what I said in the 2nd part of #57. Checking whether the property is still there using has is necessary to be able to proxy possibly legal behavior of objects wrt to property deletion and shadowing changes during iteration.
(In reply to comment #58)
> Actually I take back what I said in the 2nd part of #57. Checking whether the
> property is still there using has is necessary to be able to proxy possibly
> legal behavior of objects wrt to property deletion and shadowing changes during
> iteration.

See ECMA-262 Edition 5, 12.6.4 final two paragraphs, specifically "If a property that has not yet been visited during enumeration is deleted, then it will not be visited".

/be
(Assignee)

Comment 60

8 years ago
Created attachment 431728 [details] [diff] [review]
refreshed for TM tip
Attachment #431510 - Attachment is obsolete: true
(Assignee)

Comment 61

8 years ago
Created attachment 431737 [details] [diff] [review]
patch

Proxy.create() now takes an optional 3rd parameter that specifies the [[Class]] name the proxy should masquerade as. Proxy.createFunction always sets this to "Function".
Attachment #431728 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Blocks: 551595
(Assignee)

Comment 62

8 years ago
Created attachment 432522 [details] [diff] [review]
patch

- use ValueToBoolean conversion path instead of custom error message for the return value of the has trap
- use ValueToObject conversion for enumerate trap (object no longer has to be an array, its ok if it behaves array like and has length and appropriate indexed properties)
- use ValueToId conversion during enumeration, so ["1", 2, "three"] should work as expected.

This patch currently makes the engine-internal distinction between indexed and named properties visible to the handler. I need feedback from Tom & Mark whether this is a good idea.
Attachment #431737 - Attachment is obsolete: true

Comment 63

8 years ago
I updated the Harmony proposal wiki page with a small change to the return value of the 'defineProperty' trap.
Currently, invoking Object.defineProperty(aProxy, 'foo', pd) returns whatever the 'defineProperty' trap returns. This is wrong. The ES5 spec (section 15.2.3.6) guarantees that Object.defineProperty(O, ...) always returns O. This is a useful guarantee if you want to chain defineProperty invocations.

The correct behavior for trapping proxies is that the return value of the 'defineProperty' trap should be ignored, and the proxy itself should be returned. That is, aProxy === Object.defineProperty(aProxy,'foo',pd) regardless of the return value of the trap.

I updated the test 'trap-arguments/defineProperty.js' to verify this behavior.
Created attachment 432617 [details] [diff] [review]
Noop C++ proxy implementation

This is a first stab... It puts the proxy in js/src/ and adds a way to create proxies from the JS shell.
Created attachment 432622 [details] [diff] [review]
Generalization of C++ proxies

This applies on top of attachment 432617 [details] [diff] [review]. It creates a C++ API based on the proxy API and implements SOWs. Note that in this patch and the previous one, there are a couple of fixes to the proxy code itself that we should pull into the base patch. In particular:

  * The enumeration implementation can't assert that ipd is null. This patch just sets it to JSVAL_ZERO to indicate that it doesn't know ahead of times how many properties it will enumerate.
  * I made js_ObjectProxyClass a FRIEND so that XPConnect could cheaply unwrap proxies. There might be a better way to do this.
(Assignee)

Updated

8 years ago
No longer blocks: 551595
Depends on: 551595
(Assignee)

Comment 66

8 years ago
Created attachment 432676 [details] [diff] [review]
patch

- flip dependency on ES5 JSAPI functions (I expect those to land first now)
- intercept defineProperty trap further down in the call hierarchy, this makes sure we catch all cases with one hook
Attachment #432522 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Depends on: 552552
(Assignee)

Comment 67

8 years ago
Created attachment 432697 [details] [diff] [review]
patch

Add support to proxy existing objects (JS API only).
Attachment #432676 - Attachment is obsolete: true
(Assignee)

Comment 68

8 years ago
Created attachment 432704 [details] [diff] [review]
patch

Expose sufficient internals to make wrappers in xpconnect possible.
Attachment #432697 - Attachment is obsolete: true
(Assignee)

Comment 69

8 years ago
Created attachment 432837 [details] [diff] [review]
always stringify property names
Attachment #432704 - Attachment is obsolete: true
(Assignee)

Comment 70

8 years ago
Created attachment 433117 [details] [diff] [review]
refreshed patch
Attachment #432837 - Attachment is obsolete: true
(Assignee)

Updated

8 years ago
Attachment #433117 - Flags: review?(igor)
(Assignee)

Comment 71

8 years ago
Comment on attachment 433117 [details] [diff] [review]
refreshed patch

Please review the C++ part only. The tests are maintained by google and need some more work. I am on it.

Comment 72

8 years ago
Comment on attachment 433117 [details] [diff] [review]
refreshed patch

>diff --git a/js/src/jscntxt.h b/js/src/jscntxt.h
>--- a/js/src/jscntxt.h
>+++ b/js/src/jscntxt.h
>@@ -57,16 +57,17 @@
> #include "jsprvtd.h"
> #include "jspubtd.h"
> #include "jsregexp.h"
> #include "jsutil.h"
> #include "jsarray.h"
> #include "jstask.h"
> #include "jsvector.h"
> #include "jshashtable.h"
>+#include "jscntxt.h"

A self-include here

>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp

>@@ -1625,17 +1631,23 @@ js_HasOwnPropertyHelper(JSContext *cx, J
> {
>     jsid id;
>     if (!JS_ValueToId(cx, argc != 0 ? vp[2] : JSVAL_VOID, &id))
>         return JS_FALSE;
> 
>     JSObject *obj = JS_THIS_OBJECT(cx, vp);
>     JSObject *obj2;
>     JSProperty *prop;
>-    if (!obj || !js_HasOwnProperty(cx, lookup, obj, id, &obj2, &prop))
>+    if (!obj)
>+        return false;
>+    if (obj->isProxy()) {
>+        jsval argv[] = { ID_TO_VALUE(id) };
>+        return js_TrapProxyHandler(cx, obj, ATOM(hasOwn), 1, argv, vp);

id is unrooted here. js_TrapProxyHandler calls GetTrap that in turn calls handler->getPropert. Via getter or even recursove proxy that can run arbitrary JS code before js_TrapProxyHandler calls js_InternalCall. I guess all call sites should use an explicit auto-root or js_TrapProxyHandler should roots its argument array.

>@@ -3492,16 +3544,42 @@ js_DefineBlockVariable(JSContext *cx, JS
>+/*
>+ * Use this method with extreme caution. It trades the guts of two objects and updates
>+ * scope ownership. This operation is not thread-safe, just as fast array to slow array
>+ * transitions are inherently not thread-safe. Don't perform a swap operation on objects
>+ * shared across threads or, or bad things will happen. You have been warned.
>+ */
>+void
>+JSObject::swap(JSObject *other)

The method is very problematic if objects have different classes and ObjectOps. It breaks an important invariant that class stays the same for the object no matter what. For example, consider the enumeration protocol. It calls obj->enumerate(JSENUMERATE_INIT) to allocate class-specific enumerator state and then obj->enumerate(JSENUMERATE_DESTROY) to release one. So if the class mutates (like changing proxy into ordinary object through calling the fix method), that would call JSENUMERATE_DESTROY on the incompatible state (like reinterpreting ProxyIterClass instance as JSNativeEnumerator in js_Enumerate).

I am not sure that the enumeration protocol is the only thing that depends on this assumption that ObjectOps and Class stays the same. We would need to monitor the whlecode base to ensure that nothing else is broken.

>diff --git a/js/src/jsproxy.cpp b/js/src/jsproxy.cpp
>+JS_PUBLIC_API(JSBool)
>+JS_GetProxyObjectClass(JSContext *cx, JSObject *proxy, const char **namep)
...
>+    if (proxy->isFunctionProxy()) {
>+        *namep = "Function";

Nit: use js_Function_str here and js_Object_str below.

>+proxy_LookupProperty(JSContext *cx, JSObject *obj, jsid id, JSObject **objp,
>+                     JSProperty **propp)
>+{
>+    if (!js_TrapProxyHandler(cx, obj, ATOM(has), 1, argv, tvr.addr()))
>+        return false;
...
>+    if (found) {
>+        *propp = (JSProperty *)id;
>+        *objp = obj;

This is another example of a violation of that class-stays-the-same invariant. js_TrapProxyHandler can run a code that turns the proxy into an ordinary object. Then the drop method from the Object class would be called on the former proxy. That would reinterpret the id as JSScopeProperty.


>+static JSBool
>+proxy_SetAttributes(JSContext *cx, JSObject *obj, jsid id, JSProperty *prop,
>+                    uintN *attrsp)
>+{
>+    /* Lookup the current property descriptor so we have setter/getter/value. */
>+    PropertyDescriptor pd;
>+    if (!GetPropertyDescriptor(cx, obj, id, &pd))
>+        return false;
>+    return proxy_DefineProperty(cx, obj, id,
>+                                pd.hasValue ? pd.value : JSVAL_VOID,
>+                                pd.hasGet ? pd.getter() : NULL,
>+                                pd.hasSet ? pd.setter() : NULL,
>+                                *attrsp);

Yet another example: GetPropertyDescriptor can turn Proxy obj into Object. Given that we should try to implement the proxy protocol without class mutation IMO.


>+static void
>+proxy_TraceObject(JSTracer *trc, JSObject *obj)
>+{
>+    JSContext *cx = trc->context;
>+
>+    if (!JS_CLIST_IS_EMPTY(&cx->runtime->watchPointList))
>+        js_TraceWatchPoints(trc, obj);
>+
>+    JSClass *clasp = obj->getClass();
>+    if (clasp->mark) {
>+        if (clasp->flags & JSCLASS_MARK_IS_TRACE)
>+            ((JSTraceOp) clasp->mark)(trc, obj);
>+        else if (IS_GC_MARKING_TRACER(trc))
>+            (void) clasp->mark(cx, obj, trc);
>+    }

This clasp->mark code should not be here as proxy overwites JSObjectOps.trace and traces all the necessary slots on its own.

>+inline bool
>+JSObject::isProxy() const
>+{
>+    return isObjectProxy() || isFunctionProxy();
>+}

Nit: put classes into 2-element array so  isProxy can just if the class pointer points into the array.
Attachment #433117 - Flags: review?(igor) → review-
(Assignee)

Comment 73

8 years ago
Thanks for the quick review! I will fix the issues and put up a new patch pronto.
FWIW, the enumerate thing is the only part I remember carrying state across slowification of arrays, which is why array enumeration is so ugly.  The other place I looked at was between lookup and other property operations, but I couldn't find a way to slowify from script in there.
(Assignee)

Comment 75

8 years ago
swap() is only used for proxy -> function and proxy -> object transitions, and the reverse via JSAPI. I will audit the iteration path.

Comment 76

8 years ago
(In reply to comment #74)
> FWIW, the enumerate thing is the only part I remember carrying state across
> slowification of arrays, which is why array enumeration is so ugly.

Another place is JSObjectOps::lookup and JSObjectOps::drop. These assumes that the object stays the same and that the drop method can be used to release the property.
Yeah, but I couldn't find a way for script to trigger a slowification from that path.  Maybe there's one that I missed, though!
(Assignee)

Comment 78

8 years ago
swap() doesn't happen at arbitrary times. Its caused by fix(), which is caused by freeze/seal/preventExtension.

Comment 79

8 years ago
(In reply to comment #78)
> swap() doesn't happen at arbitrary times. Its caused by fix(), which is caused
> by freeze/seal/preventExtension.

By "arbitrary time" I mean that a script can do that from js_TrapProxyHandler.
(Assignee)

Comment 80

8 years ago
Yeah, but only via freeze/seal/preventExtension. Proxy.fix() is a hook that will be removed before landing (its only for testing, I will make it DEBUG only).

Comment 81

8 years ago
(In reply to comment #80)
> Yeah, but only via freeze/seal/preventExtension.

But that does not change the conclusion. If the class and ops can mutate and a script can trigger that, then we cannot have the iteration protocol and lookupProperty/drop pair (and potentially other parts of code) in the current form.
(Assignee)

Comment 82

8 years ago
Created attachment 434628 [details] [diff] [review]
refreshed patch, no longer any prerequisits required (just this patch)
Attachment #433117 - Attachment is obsolete: true
(Assignee)

Comment 83

8 years ago
To fix the enumeration/fix situation, we need a rewrite of the enumeration protocol. When we transition to a native object, the proxy already gave us a list of ids to iterate over. Those are all ids (including ids found along the proto chain). Our primary enumeration protocol works per object, and walks from native enumerate state to native enumerator state as it walks up the chain. This is inherently incompatible. We should fix native iteration to use a snapshotted list, and then make proxies feed into that.
(In reply to comment #82)
> Created an attachment (id=434628) [details]
> refreshed patch, no longer any prerequisits required (just this patch)

This looks like the ES5 API patch, not the proxies patch.
(Assignee)

Comment 85

8 years ago
Created attachment 434662 [details] [diff] [review]
patch

Refreshed patch, this time for real. I think.
Attachment #434628 - Attachment is obsolete: true

Comment 86

8 years ago
(In reply to comment #83)
> To fix the enumeration/fix situation, we need a rewrite of the enumeration
> protocol.

yes and this deficiency of the enumerator protocol already hurts the array iterators. They are forced to deal with fast->slow transition during the enumeration.

Still, what about lookupProperty/dropProperty? It also has to be fixed.
(Assignee)

Comment 87

8 years ago
Yeah, I will ignore the enumeration issue while we fix the protocol and look into lookupProperty/dropProperty next.
(Assignee)

Comment 88

8 years ago
The committee didn't like the className proposal, for a number of reasons. In particular, despite responding correctly to Object.prototype.toString.call(), wrapped objects would still not pass class tests (i.e. JS_InstanceOf(DateClass)), so its a bit pointless. I have to talk to Blake to find out whether the className hack is really that crucial.
Blocks: 563010
(Assignee)

Comment 89

7 years ago
quick note: blake and I will restructure the implementation a bit. Currently we have a C++ class above the JS API layer that implements fast natives that redirect to a C++ proxy implementation. We will flip this around and make the C++ class the foundation, so its possible to implement C++ proxies that are very fast (wrappers). The cost will be a virtual method dispatch per trap. Scripted proxies will be implemented as a specialized C++ class on top of this api.
(In reply to comment #89)
> quick note: blake and I will restructure the implementation a bit. Currently we
> have a C++ class above the JS API layer that implements fast natives that
> redirect to a C++ proxy implementation. We will flip this around and make the
> C++ class the foundation, so its possible to implement C++ proxies that are
> very fast (wrappers). The cost will be a virtual method dispatch per trap.
> Scripted proxies will be implemented as a specialized C++ class on top of this
> api.

Sounds great.  I might even revisit the CPOW implementation when this framework becomes available.  Do you have a preliminary patch yet, or might you have one soon?
(Assignee)

Comment 91

7 years ago
Created attachment 445037 [details] [diff] [review]
patch

Refresh proxies patch on top of the iterator rewrite. This is against TM tip. Proxies now use a native iterator, which deletes about half the code in the proxy implementation. Also, since iterators are no longer connected to the object they iterate over, this allows fixing during iteration. This is untested. I will debug it tonight.
Attachment #434662 - Attachment is obsolete: true
(Assignee)

Comment 92

7 years ago
Created attachment 445038 [details] [diff] [review]
patch

added missing files
Attachment #445037 - Attachment is obsolete: true
(Assignee)

Comment 93

7 years ago
Next steps:
- debug the patch, make it work again
- merge it with blake's C++ base class
- lift the scripted implementation on top of the base class, making it the fast path
(Assignee)

Comment 94

7 years ago
Mark suggested that we might want to drop the stringification of property ids after all.
(Assignee)

Comment 95

7 years ago
Clarification: Mark wants enumerate to still return stringified names. Absolutely makes sense since for-in wants strings, even for numeric properties/arrays and such.
(Assignee)

Comment 96

7 years ago
Created attachment 445257 [details] [diff] [review]
patch

The attached patch subsumes mrbkap's C++ base class and implements scripted proxies on top of that. This patch also implements Mark's latest suggestion (no stringification except for iteration), which I am a big fan of (nice speedup for array-like proxies). Handlers can now be a C++ class or a JSObject. For the former, handlers can be singleton and store state directly in the proxy for object proxies (obj->setProxyPrivate/obj->getProxyPrivate). This should reduce the memory footprint of wrappers and host objects that are implemented as a proxy object. Function proxies already use all slots, so those can't use a singleton handler since the handler must store per-proxy object state. This is completely untested. Last but not least, the patch now faithfully emulates derived traps in C++, according to the proposal. This happens underneath the scripting layer, so C++ code can also fall back on this emulation. Debugging next.
Attachment #432622 - Attachment is obsolete: true
Attachment #445038 - Attachment is obsolete: true
(Assignee)

Comment 97

7 years ago
Created attachment 445259 [details] [diff] [review]
patch

Passes my internal test suite (will try Tom's suite next).
Attachment #445257 - Attachment is obsolete: true
(Assignee)

Comment 98

7 years ago
Created attachment 445260 [details] [diff] [review]
patch
Attachment #445259 - Attachment is obsolete: true
(Assignee)

Comment 99

7 years ago
Created attachment 445269 [details] [diff] [review]
patch
Attachment #432617 - Attachment is obsolete: true
Attachment #445260 - Attachment is obsolete: true
(Assignee)

Comment 100

7 years ago
Created attachment 445274 [details] [diff] [review]
add shell wrap command
Attachment #445269 - Attachment is obsolete: true
(Assignee)

Comment 101

7 years ago
Created attachment 445279 [details] [diff] [review]
restore className masquerading
Attachment #445274 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #445279 - Attachment is patch: true
Attachment #445279 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Comment 102

7 years ago
js> wrap({})
({})
js> wrap({}).toString()
"[object Object]"
js> Proxy.isTrapping(wrap({}))
true
js>
(Assignee)

Updated

7 years ago
Blocks: 565827
(Assignee)

Updated

7 years ago
Blocks: 565828
(Assignee)

Updated

7 years ago
Blocks: 565829

Comment 103

7 years ago
At the last EcmaScript meeting, didn't we decide to remove Proxy.isTrapping()? If it just appears above to aid debugging, then never mind.
(Assignee)

Comment 104

7 years ago
Yes, isTrapping will be removed for sure. It was never part of the spec.
(Assignee)

Comment 105

7 years ago
Sorry, I take that back. isTrapping was in the spec early on, but is no more. fix was never in the spec. Both will be removed or debug only.
(Assignee)

Comment 106

7 years ago
Created attachment 445284 [details] [diff] [review]
patch

only enable Proxy.isTrapping and Proxy.fix in DEBUG builds
Attachment #445279 - Attachment is obsolete: true
(Assignee)

Comment 107

7 years ago
Created attachment 445294 [details] [diff] [review]
fix reserved slot count
Attachment #445284 - Attachment is obsolete: true
(Assignee)

Comment 108

7 years ago
Created attachment 445308 [details] [diff] [review]
fix embarassing bug in JSITER_HIDDEN flag declaration
Attachment #445294 - Attachment is obsolete: true
(Assignee)

Comment 109

7 years ago
Created attachment 445311 [details] [diff] [review]
fix bug in proxy enumeration id conversion

The patch seems reasonable stable for general web use.
Attachment #445308 - Attachment is obsolete: true
(Assignee)

Comment 110

7 years ago
Gary, I could use a little help with fuzzing. This patch is supposed to land in a couple days. You can test against the shell. There are two new identifiers in the shell (wrap and Proxy). In the browser its just Proxy (notice the capital P). In both cases Proxy has two properties "create" and "createFunction". Thanks!
(Assignee)

Comment 111

7 years ago
There are probably a couple other interesting keywords actually:

+    "Proxy",                    /* ProxyAtom                    */
+
+    "getOwnPropertyDescriptor", /* getOwnPropertyDescriptorAtom */
+    "getPropertyDescriptor",    /* getPropertyDescriptorAtom    */
+    "defineProperty",           /* definePropertyAtom           */
+    "delete",                   /* deleteAtom                   */
+    "getOwnPropertyNames",      /* getOwnPropertyNames          */
+    "enumerate",                /* enumerateAtom                */
+    "fix",
+
+    "has",                      /* hasAtom                      */
+    "hasOwn",                   /* hasOwnAtom                   */
+    "enumerateOwn",             /* enumerateOwnAtom             */

And a valid example:

javascript:for (i in Proxy.create({ enumerate: function() { return ["1","2"]; } })) alert(i)
(Assignee)

Comment 112

7 years ago
Created attachment 445313 [details] [diff] [review]
removed unused construct atom
Attachment #445311 - Attachment is obsolete: true
(Assignee)

Comment 113

7 years ago
Created attachment 445315 [details] [diff] [review]
refreshed for TM tip
Attachment #445313 - Attachment is obsolete: true

Comment 114

7 years ago
(In reply to comment #113)
> Created an attachment (id=445315) [details]
> refreshed for TM tip

The changing of object's class is still not safe. There is still the lookupProeprty/dropProperty problem with the drop method assuming that the class should be the same as one for the lookup. Plus even with the new iteration implementation it is still possible (correct me if I am wrong here) that the class would change in the middle of enumeration when the code collects the iteration ids.
(Assignee)

Comment 115

7 years ago
#114: I will coordinate with jordendorff to see how quickly his stuff can land. I have no intentions of supporting multi-threaded objects here since its a dying dinosaur. Without multi-threading, both issues should go away. I might be able to land before him and fly under the radar as long none of the objects we will use this for is actually shared in our embedding.
(Assignee)

Comment 116

7 years ago
Created attachment 445441 [details] [diff] [review]
patch

Fix iteration for proxy objects. The proxy might be along the prototype, in which case we can't skip filtering for duplicate ids. Also move the proxy case out of the fast path into the non-native case. And last but not least, don't crash on "for (i in null) ...".
Attachment #445315 - Attachment is obsolete: true
(Assignee)

Comment 117

7 years ago
(in case anyone wonders how I found that, cnn.com seems to iterate _a ton_ over null, for whatever reason)

Comment 118

7 years ago
(In reply to comment #115)
> #114: I will coordinate with jordendorff to see how quickly his stuff can land.
> I have no intentions of supporting multi-threaded objects here since its a
> dying dinosaur. Without multi-threading, both issues should go away.

I am not talking about multi-threading! The issue is that the class change is unsafe as the proxy code allows for a script to trigger that between lookupProperty and dropProperty pair and even during enumeration despite the improved iteration implementation.
(Assignee)

Comment 119

7 years ago
Maybe we misunderstand each other. Can you please write down _precisely_ what your concern is? We are in the process of eliminating scopes and locks, and dropProperty can be eliminated as well. So nothing can get between lookupProperty and dropProperty, because dropProperty will be gone or at the very least a noop. Without multi-threading, the snapshot operation is atomic for native objects and can't be interrupted by a scripted proxy (not on the same thread anyway). Note that we don't turn non-native objects into proxies, only native ones (same vice versa). Again, please write down exactly what you mean. I don't want to overlook what you are concerned about, but I have to understand it first.

Comment 120

7 years ago
(In reply to comment #119)
> Maybe we misunderstand each other. Can you please write down _precisely_ what
> your concern is? 

Currently dropProperty, getAttributes and setAttributes from JSObjectOps takes JSProperty *prop that is supposed to be coming from lookupProperty. If the class changes between lookupProperty and any of these operations then the change must be such so the dropProperty, getAttributes and setAttributes coming from the new class would still work with the result of lookupProperty coming from the old class. If that cannot be guaranteed for all possible class changes, then we have a hazard if the class change can be triggered from a script.

Similarly the enumeration protocol passes the state created by JSENUMERATE_INIT to JSENUMERATE_NEXT etc. If a proxy script can trigger a class change between the init and the next calls, then the state coming from the old class must be compatible with the new class for all possible cases. If not, then we have a hazard again.
(Assignee)

Comment 121

7 years ago
> Currently dropProperty, getAttributes and setAttributes from JSObjectOps takes
> JSProperty *prop that is supposed to be coming from lookupProperty. If the
> class changes between lookupProperty and any of these operations then the
> change must be such so the dropProperty, getAttributes and setAttributes coming
> from the new class would still work with the result of lookupProperty coming
> from the old class. If that cannot be guaranteed for all possible class
> changes, then we have a hazard if the class change can be triggered from a
> script.

Yes, that's correct. Native object ops use JSScopeProperty, whereas proxy only has direct hits and uses (JSProperty *)id. We can't switch the class in between those transactions, but nothing in the patch does that right now.

> 
> Similarly the enumeration protocol passes the state created by JSENUMERATE_INIT
> to JSENUMERATE_NEXT etc. If a proxy script can trigger a class change between
> the init and the next calls, then the state coming from the old class must be
> compatible with the new class for all possible cases. If not, then we have a
> hazard again.

Look at jsiter.cpp. The protocol is now only called from within InitNativeEnumerator. No script is triggered in between, we start with INIT, poll NEXT and then destroy the enumerator. Nothing else can happen in between.

Comment 122

7 years ago
(In reply to comment #121)
> Yes, that's correct. Native object ops use JSScopeProperty, whereas proxy only
> has direct hits and uses (JSProperty *)id. We can't switch the class in between
> those transactions, but nothing in the patch does that right now.

The patch contains:

+static JSBool
+proxy_LookupProperty(JSContext *cx, JSObject *obj, jsid id, JSObject **objp,
+                     JSProperty **propp)
+{
+    bool found;
+    if (!JSProxy::has(cx, obj, id, &found))
+        return false;
+    if (found) {
+        *propp = (JSProperty *)id;
+        *objp = obj;
+    } else {
+        *objp = NULL;
+        *propp = NULL;
+    }

where JSProxy::has calls a script. That script can turn the proxy object into ordinary object leading to a crash during getAtrributes or dropProperty. 

> Look at jsiter.cpp. The protocol is now only called from within
> InitNativeEnumerator. No script is triggered in between, we start with INIT,
> poll NEXT and then destroy the enumerator. Nothing else can happen in between.

The patch contains:

JSProxyHandler::enumerateOwn(JSContext *cx, JSObject *proxy, JSIdArray **idap) {
+    if (!getOwnPropertyNames(cx, proxy, idap))
+        return false;
...
+    for (size_t n = 0; n < ida.length(); ++n) {
+        JS_ASSERT(n >= w);
+        vector[w] = vector[n];
+        if (!getOwnPropertyDescriptor(cx, proxy, vector[n], &roots[0]))

Here the script implementing getOwnPropertyNames can turn the proxy into ordinary object. Yet the patch still calls getOwnPropertyDescriptor which assumes that the proxy is still a proxy.
(Assignee)

Comment 123

7 years ago
Created attachment 445530 [details] [diff] [review]
patch
Attachment #445441 - Attachment is obsolete: true
(Assignee)

Comment 124

7 years ago
> where JSProxy::has calls a script. That script can turn the proxy object into
> ordinary object leading to a crash during getAtrributes or dropProperty. 

We can check for this. If has makes obj native, we catch that and run the native lookupProperty hook, which returns the proper state for getAttributes and dropProperty.

> Here the script implementing getOwnPropertyNames can turn the proxy into
> ordinary object. Yet the patch still calls getOwnPropertyDescriptor which
> assumes that the proxy is still a proxy.

Dito here. We check after the script hook, and re-execute the iteration for that object using the native case if the object is no longer a proxy.

Comment 125

7 years ago
(In reply to comment #124)
> We can check for this. If has makes obj native, we catch that and run the
> native lookupProperty hook, which returns the proper state for getAttributes
> and dropProperty.

That would help, but not match. It would not deal with proxy_getAttributes turning the proxy into ordinary object leading to the crash if setAttributes is called after that.

> 
> > Here the script implementing getOwnPropertyNames can turn the proxy into
> > ordinary object. Yet the patch still calls getOwnPropertyDescriptor which
> > assumes that the proxy is still a proxy.
> 
> Dito here. We check after the script hook, and re-execute the iteration for
> that object using the native case if the object is no longer a proxy.

It is also necessary to check after each getOwnPropertyDescriptor. Also it would be necessary to monitor all the code against this proxy->object mutations.
(Assignee)

Comment 126

7 years ago
Created attachment 445533 [details] [diff] [review]
patch

I like this a lot better. We disallow fixing proxies while the handler runs and a type error is thrown. This is a lot easier to audit and get right and shouldn't restrict flexibility. Touching a proxy while running the handler is super dangerous and wonky anyway (great way to recurse to death).
Attachment #445530 - Attachment is obsolete: true

Comment 127

7 years ago
(In reply to comment #125)
> (In reply to comment #124)
> > We can check for this. If has makes obj native, we catch that and run the
> > native lookupProperty hook, which returns the proper state for getAttributes
> > and dropProperty.
> 
> That would help, but not match. It would not deal with proxy_getAttributes
> turning the proxy into ordinary object leading to the crash if setAttributes is
> called after that.

I think the proper solution here would be to replace lookupProperty/getAttributes/setAttributes/dropProperty with methods like hasProperty, getAttributes() that do not take JSProperty while keeping the former only for native objects.

Comment 128

7 years ago
(In reply to comment #126)
> We disallow fixing proxies while the handler runs and a type error is thrown.

Yes, this looks much better. Still the patch should not add anything like JS_Becomes and JSObject::swap. Rather it should inline that directly into the proxy with big comments explaining the danger of class mutations and why it is safe to do it for proxies.

Updated

7 years ago
Depends on: 566141
(In reply to comment #127)
> I think the proper solution here would be to replace
> lookupProperty/getAttributes/setAttributes/dropProperty with methods like
> hasProperty, getAttributes() that do not take JSProperty while keeping the
> former only for native objects.

+∞

Please file a bug, let's do this ASAP. JSObjectOps is frozen. Any internal need for an sprop out param can be native-only and call js_LookupPropertyWithFlags.

/be
Er, JSObjectOps is *not* frozen, and not public API.

/be
(Assignee)

Comment 131

7 years ago
Yeah, I am jumping on this. We can eliminate dropProperty altogether. Lovely.
(Assignee)

Updated

7 years ago
Depends on: 566143

Comment 132

7 years ago
I am not sure about implication of the patch and trace recorder. The latter contains fragments like:

            TraceMonitor &localtm = *traceMonitor;
            int protoIndex = js_LookupPropertyWithFlags(cx, aobj, id,
                                                        cx->resolveFlags,
                                                        &obj2, &prop);
...
            /* js_LookupPropertyWithFlags can reenter the interpreter and kill |this|. */
            if (!localtm.recorder) {
                if (prop)
                    obj2->dropProperty(localcx, prop);
                return ARECORD_ABORTED;
            }

Now, with proxies and scripts doing something like:

global_object.__proto__ = proxy

the script can run arbitrary code during js_LookupPropertyWithFlags. Could that abort and restart recording again?

This also suggests that js_LookupPropertyWithFlags should not run the lookupProperty hook for non-natives on

Comment 133

7 years ago
(In reply to comment #132)
> This also suggests that js_LookupPropertyWithFlags should not run the
> lookupProperty hook for non-natives on

Sorry for the incomplete comment:

This also suggests that js_LookupPropertyWithFlags should not run the lookupProperty hook for non-natives on the prototype chain delegating that to the caller. But that is for another bug.
(Assignee)

Comment 134

7 years ago
Proxies are non-native and not dense arrays, so we don't trace them currently. When we do that we will have to do the usual deep abort dance. Once the basic tracing part is in place my intent was to properly inline the proxy code on trace, so we won't hit the lookup path at all actually (on trace anyway).

Comment 135

7 years ago
(In reply to comment #134)
> Proxies are non-native and not dense arrays, so we don't trace them currently.

My comments was about the recorder. It contains calls to js_LookupPropertyWithFlags that with proxies can run an arbitrary scrip that may trigger tracing/recording. So the question is how safe that code against such scripts?

To Luke: your patch for the bug 517973 has code and comments like:

/* js_LookupPropertyWithFlags can reenter the interpreter and kill
|this|. */
            if (!localtm.recorder) {
                if (prop)
                    obj2->dropProperty(localcx, prop);
                return ARECORD_ABORTED;
            }

Do you know if the code would be still safe if js_LookupPropertyWithFlags can run an arbitrary script?
(In reply to comment #135)
> Do you know if the code would be still safe if js_LookupPropertyWithFlags can
> run an arbitrary script?

js_Interpret kills the recorder at the top, so checks like 'if (!localtm.recorder)' inherently guard against reentrance.  The only danger is in allowing operations to reenter which are not currently checked in this manner.

Comment 137

7 years ago
(In reply to comment #136)
> js_Interpret kills the recorder at the top, so checks like 'if
> (!localtm.recorder)' inherently guard against reentrance.  The only danger is
> in allowing operations to reenter which are not currently checked in this
> manner.

And since the interpreter kills the recorder on exit from js_Interpret this is safe even if the interpreter starts the recorder recursively.

Comment 138

7 years ago
(In reply to comment #126)
> Created an attachment (id=445533) [details]
> patch

It would be better if the error is reported at the point of the fix call. This way the error would be reported immediately at the point of the call for easier debugging.
(Assignee)

Comment 139

7 years ago
Yes, that would be nicer, but I have no room to store a nesting counter right now. This is a temporary limitation until I have reworked ObjectOps. It will do for now.
(Assignee)

Comment 140

7 years ago
Created attachment 445825 [details] [diff] [review]
interdiff
(Assignee)

Comment 141

7 years ago
Created attachment 445826 [details] [diff] [review]
patch
Attachment #445533 - Attachment is obsolete: true
Attachment #445825 - Attachment is obsolete: true
(Assignee)

Comment 142

7 years ago
Created attachment 445837 [details] [diff] [review]
interdiff
(Assignee)

Comment 143

7 years ago
Created attachment 445889 [details] [diff] [review]
patch

Rework code to use JSPropertyDescriptor instead of a property descriptor object.
Attachment #445826 - Attachment is obsolete: true
Attachment #445837 - Attachment is obsolete: true
(Assignee)

Comment 144

7 years ago
Created attachment 445892 [details] [diff] [review]
mark proxy private during gc
Attachment #445889 - Attachment is obsolete: true
(Assignee)

Comment 145

7 years ago
Created attachment 445925 [details] [diff] [review]
patch
Attachment #445892 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #445925 - Flags: review?(mrbkap)
(Assignee)

Comment 146

7 years ago
I submitted a fresh build to the try server. Passes all local reftests and trace tests and the Tom's and my proxy test suite. I will land the patch as soon I have review.
Comment on attachment 445925 [details] [diff] [review]
patch

>diff --git a/js/src/jsiter.cpp b/js/src/jsiter.cpp
>@@ -250,16 +277,32 @@ InitNativeIterator(JSContext *cx, JSObje
>+                for (size_t n = 0; n < size_t(ida->length); ++n)
>+                    if (!Enumerate(cx, obj, pobj, ida->vector[n], true, flags, ht, props))
>+                        return false;

Nit: braces around the multi-line loop body.

>diff --git a/js/src/jsobj.cpp b/js/src/jsobj.cpp
> JSBool
> js_GetOwnPropertyDescriptor(JSContext *cx, JSObject *obj, jsid id, jsval *vp)
>-    jsval roots[] = { JSVAL_VOID, JSVAL_VOID };
>+    jsval roots[] = { JSVAL_VOID, JSVAL_VOID, JSVAL_VOID };

Why add the extra root? Simply extra clarity? I buy it if so, but this seems
like an unnecessary change to me.

>+DefineProperties(JSContext *cx, JSObject *obj, JSObject *props)
>+     AutoDescriptorArray descs(cx);
>+     size_t len = ida.length();
>+     for (size_t i = 0; i < len; i++) {
>+         jsid id = ida[i];
>+         PropertyDescriptor* desc = descs.append();
>+         AutoValueRooter tvr(cx);
>+         if (!desc || !JS_GetPropertyById(cx, props, id, tvr.addr()) ||
>+             !desc->initialize(cx, id, tvr.value())) {
>+             return false;
>+         }
>+     }
>+
>+     bool dummy;
>+     for (size_t i = 0; i < len; i++) {
>+         if (!DefineProperty(cx, obj, descs[i], true, &dummy))
>+             return false;
>+     }

Why two loops here? i.e., why the temporary |descs| instead of eagerly
defining each property as you get it on obj?

>+inline bool
>+JSObject::isCallable() {

Nit: { on its own line.

>+static inline bool
>+js_IsCallable(jsval v) {

Ditto.

>diff --git a/js/src/jsproxy.cpp b/js/src/jsproxy.cpp
>+JSProxyHandler::enumerateOwn(JSContext *cx, JSObject *proxy, JSIdArray **idap) {

Ditto.

>+bool
>+JSNoopProxyHandler::delete_(JSContext *cx, JSObject *proxy, jsid id, bool *bp)
>+{
>+    AutoValueRooter tvr(cx);
>+    if (!JS_DeletePropertyById2(cx, wrappedObject(proxy), id, tvr.addr()))
>+        return false;
>+    JSBool deleted;
>+    if (!JS_ValueToBoolean(cx, tvr.value(), &deleted))

js_ValueToBoolean would be better here (the other JS_ uses are ok IMO since
they do more than just forward).

>+    return js_NewPropertyDescriptorObject(cx, id, desc->attrs, jsval(desc->getter), jsval(desc->setter),

I suspect these will warn on gcc? If so, there's macros for that.

>+                                          desc->value, vp);
>+}
>+

To my eye, "ReportBadFix" reads oddly. It seems like something has already
gone wrong. I'd go with TryHandlerTrap or something like that.

>+ArrayToJSIDArray(JSContext *cx, jsval array, JSIdArray **idap)
>+{
>+    if (JSVAL_IS_PRIMITIVE(array))
>+        return (*idap = NewIdArray(cx, 0)) != NULL;
>+
>+    JSObject *obj = JSVAL_TO_OBJECT(array);
>+    jsuint length;
>+    if (!js_GetLengthProperty(cx, obj, &length)) {
>+        JS_ReportErrorNumber(cx, js_GetErrorMessage, NULL, JSMSG_BAD_ARRAY_LENGTH);
>+        return false;
>+    }
>+    AutoIdArray ida(cx, *idap = NewIdArray(cx, length));

No need to set *idap here.

This patch is sufficiently large, that I could stare at it for days and my eyes would glaze over half way through no matter where I started. Let's get this in and fuzzed as fast as possible (that being said, I did actually read through everything, and it all looks reasonable, if gal-y :)).
Attachment #445925 - Flags: review?(mrbkap) → review+
(Assignee)

Comment 148

7 years ago
> 
> Why add the extra root? Simply extra clarity? I buy it if so, but this seems
> like an unnecessary change to me.

I would have to split the control-flow further below to hand in void for setter and getter if its a data property. Its an extra root thats easier to read in favor of two ? : in the call below.
(Assignee)

Comment 149

7 years ago
> >+DefineProperties(JSContext *cx, JSObject *obj, JSObject *props)
> >+     AutoDescriptorArray descs(cx);
> >+     size_t len = ida.length();
> >+     for (size_t i = 0; i < len; i++) {
> >+         jsid id = ida[i];
> >+         PropertyDescriptor* desc = descs.append();
> >+         AutoValueRooter tvr(cx);
> >+         if (!desc || !JS_GetPropertyById(cx, props, id, tvr.addr()) ||
> >+             !desc->initialize(cx, id, tvr.value())) {
> >+             return false;
> >+         }
> >+     }
> >+
> >+     bool dummy;
> >+     for (size_t i = 0; i < len; i++) {
> >+         if (!DefineProperty(cx, obj, descs[i], true, &dummy))
> >+             return false;
> >+     }
> 
> Why two loops here? i.e., why the temporary |descs| instead of eagerly
> defining each property as you get it on obj?

The code existed before. I just moved it around. I think the idea is you make all properties or none. I can only see oom as failure in which case I don't think it matters but you have to take this up with Waldo.
(Assignee)

Comment 150

7 years ago
> No need to set *idap here.

A little further down I do idap->vector, so it better be set :)

I fixed all other nits. Testing and pushing.
I will read the patch by tomorrow.

Re: comment 149: ES5 backed off on atomic defineProperties, but maybe there is a need for two loops. Waldo would know, but can we sort it out now?

Nit on this disjunction in an if condition:

> >+         if (!desc || !JS_GetPropertyById(cx, props, id, tvr.addr()) ||
> >+             !desc->initialize(cx, id, tvr.value())) {

Break after every ||, for uniformity of starting column for each term.

/be
(In reply to comment #149)
> The code existed before. I just moved it around. I think the idea is you make
> all properties or none. I can only see oom as failure in which case I don't
> think it matters but you have to take this up with Waldo.

Not all properties or none, but rather that all descriptors to be defined have sane structures beforehand -- so there's no {get: function(), value: 17} nonsensical descriptor halfway through -- if [[Get]](props, id) or ToPropertyDescriptor failed, for example.  Once you start defining, you lose if OOM, or you lose if you attempt to redefine a non-configurable descriptor, or something like that, but you won't be calling getters or setters on any passed-in descriptors at some intermediate step in time -- you lose iff [[DefineOwnProperty]] throws.  15.2.3.7 is pretty clear about the need for two loops.
(Assignee)

Comment 153

7 years ago
Pushed to tracemonkey.

http://hg.mozilla.org/tracemonkey/rev/4dd9be00049c
Whiteboard: fixed-in-tracemonkey
(Assignee)

Comment 154

7 years ago
Forgot to add the test directory to the makefile.

http://hg.mozilla.org/tracemonkey/rev/83f38f7b4fc1

Updated

7 years ago
Depends on: 566781

Updated

7 years ago
Depends on: 566790

Updated

7 years ago
Depends on: 566806

Updated

7 years ago
Depends on: 566811

Updated

7 years ago
Blocks: 566818

Updated

7 years ago
No longer blocks: 566818
Depends on: 566818
(Assignee)

Comment 155

7 years ago
2nd attempt to make the browser test harness happy

http://hg.mozilla.org/tracemonkey/rev/67e1c38f60fd
(Assignee)

Updated

7 years ago
Blocks: 485791
Depends on: 566908
Depends on: 566914
(Assignee)

Comment 156

7 years ago
Created attachment 446271 [details] [diff] [review]
patch to disable scripted proxies
Attachment #445925 - Attachment is obsolete: true
(Assignee)

Updated

7 years ago
Attachment #446271 - Attachment is patch: true
Attachment #446271 - Attachment mime type: application/octet-stream → text/plain
(Assignee)

Updated

7 years ago
Attachment #445925 - Attachment is obsolete: false
(Assignee)

Updated

7 years ago
Blocks: 566951
(Assignee)

Updated

7 years ago
Depends on: 567055
Depends on: 567059

Updated

7 years ago
Depends on: 567068
Depends on: 567081
Depends on: 567387
Depends on: 567580
(Assignee)

Comment 157

7 years ago
Mark an Tom meant to use full value property names, not just drop the string conversion (and make integer ids visible). Since that's not likely to be implementable without major changes to the VM, we will go back to forcefully stringify property names.
(Assignee)

Comment 158

7 years ago
Stringification implemented in bug 568051.
(Assignee)

Updated

7 years ago
Depends on: 568051

Comment 159

7 years ago
http://hg.mozilla.org/mozilla-central/rev/4dd9be00049c
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → FIXED

Updated

7 years ago
Depends on: 568073
Depends on: 568281

Updated

7 years ago
Depends on: 568303
Blocks: 568398
(Assignee)

Updated

7 years ago
Depends on: 568413
(Assignee)

Comment 160

7 years ago
iterate trap implemented in bug 568413.

Updated

7 years ago
Depends on: 568465

Comment 161

7 years ago
I make the conservative stack scanner a blocker for this bug. Otherwise we will spend way too many efforts hunting for the GC hazards in the proxy-related code.
Depends on: 516832

Updated

7 years ago
Depends on: 568473
(Assignee)

Comment 162

7 years ago
I don't think #161 is a meaningful dependency. There are no intentional or hard to find GC hazards in the proxy code. There are GC hazards in the engine. The proxy code revealed some of those. Until we decide to rely on conservative stack scanning, we have to fix all engine GC hazards. I won't remove the dependency to avoid endless and pointless debate, but for the record, I think the dependency is bogus.
I agree with comment 162. We need static analysis *and* conservative stack scanning at this point. Why we don't have static analysis is not clear -- it's just work we have chosen to deprioritize.

For proxies the implementation should be paranoid and use RAII guards over all calls out of file scope, if not all call-outs. But you could argue likewise for other parts of the engine. Doing this blindly could regress perf, but we do not care about proxy perf without profile-based evidence, so it's easier to argue for applying this conservative rule blindly to jsproxy.cpp/.h.

/be

Comment 164

7 years ago
(In reply to comment #162)
> I don't think #161 is a meaningful dependency. There are no intentional or hard
> to find GC hazards in the proxy code. There are GC hazards in the engine. The
> proxy code revealed some of those.

The problem is that proxies violates a few assumptions that previously made code safe. Those can be rather hard to spot and they could be scattered through out the whole engine. For example, it took years to find GC hazards related to code that was written under assumption that getProperty can not run  arbitrary scripts and trigger a full GC which had been true prior the introduction of scripted getters. I do not want that to repeat again. So IMO we should either make this bug to depend on static GC analysis bug or on the conservative GC scanner.

Updated

7 years ago
Depends on: 568855
Depends on: 568867

Updated

7 years ago
Keywords: dev-doc-needed
Target Milestone: --- → mozilla1.9.3a5
(Assignee)

Updated

7 years ago
Depends on: 569162
(Assignee)

Updated

7 years ago
No longer depends on: 568465
Depends on: 569384
(Assignee)

Updated

7 years ago
No longer depends on: 569384
Depends on: 569774
Depends on: 571168
(Assignee)

Updated

7 years ago
Depends on: 571452

Updated

7 years ago
Depends on: 581486

Updated

7 years ago
Depends on: 584578

Updated

7 years ago
No longer blocks: 566951
Depends on: 593583

Comment 165

7 years ago
I've written some doc on proxies:
https://developer.mozilla.org/en/JavaScript/Reference/Global_Objects/Proxy

I'd love to have your feedback.
This content looks pretty good to me, and we haven't gotten any complaints, so I'm marking this as documentation complete. Feel free to add more to the docs if need be though!
Keywords: dev-doc-needed → dev-doc-complete
Depends on: 635200

Updated

6 years ago
Depends on: 643100

Updated

6 years ago
Depends on: 655112

Updated

6 years ago
Depends on: 600677

Updated

6 years ago
Depends on: 645726

Updated

6 years ago
Depends on: 582959

Updated

6 years ago
Depends on: 601379
Depends on: 637201

Updated

6 years ago
Depends on: 689684

Updated

6 years ago
Depends on: 601329
Duplicate of this bug: 694103
Blocks: 694100
Depends on: 665198
Depends on: 703537

Updated

4 years ago
Depends on: 658266
You need to log in before you can comment on or make changes to this bug.