546590 - (harmony:proxies) Implement Harmony Proxies [ES6?]

Status: RESOLVED FIXED

(Core :: JavaScript Engine, enhancement)

Description

[Andreas Gal :gal]

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

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

Comment 1

[Andreas Gal :gal]

Attachment: very preliminary totally incomplete patch

Comment 2

[Andreas Gal :gal]

Attachment: this time with missing files, wip

Comment 3

[Reed Loden [:reed]]

Comment on attachment 427282 [details] [diff] [review]
this time with missing files, wip

>+ * The Initial Developer of the Original Code is
>+ *   Andreas Gal <[e-mail]>
>+ *
>+ * 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.

Comment 4

[Andreas Gal :gal]

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

Comment 5

[Andreas Gal :gal]

Attachment: with new and improved copyright disclaimer

Comment 6

[Reed Loden [:reed]]

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

Comment 7

[Andreas Gal :gal]

Attachment: patch with spanking new and yet better copyright header, and also switch to JSObjectOps since the JSClass interface isn't powerful enough

Comment 8

[Andreas Gal :gal]

Attachment: mostly complete proxy implementation, but only partially debugged

Comment 9

[Andreas Gal :gal]

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.

Comment 10

[Andreas Gal :gal]

Attachment: make enumeration work

Comment 11

[Andreas Gal :gal]

Attachment: implement function proxies (largly untested)

invoke trap is still missing

Comment 12

[Andreas Gal :gal]

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

Comment 13

[Brendan Eich [:brendan]]

(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

Comment 14

[Brendan Eich [:brendan]]

Er, jscloak.{h,cpp}.

/be

Comment 15

[Brendan Eich [:brendan]]

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

Comment 16

[Brendan Eich [:brendan]]

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

/be

Comment 17

[Andreas Gal :gal]

Attachment: 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

Comment 18

[Andreas Gal :gal]

We don't implement getOwnPropertyNames(aProxy), I will wire it up once that's fixed.

Comment 19

[Andreas Gal :gal]

Attachment: patch

Wire up reflective API in Object.* to call the proper proxy handler traps.

Comment 20

[Andreas Gal :gal]

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.

Comment 21

[Andreas Gal :gal]

Attachment: patch

Fixed a couple rooting bugs and cleanup.

Comment 22

[Andreas Gal :gal]

Attachment: add error reporting

Comment 23

[Andreas Gal :gal]

Attachment: patch

Implemented fixing of proxies (not wired up yet).

Comment 24

[Andreas Gal :gal]

Attachment: 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.

Comment 25

[Andreas Gal :gal]

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

Comment 26

[Andreas Gal :gal]

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?

Comment 27

[Andreas Gal :gal]

(Note: currently fix always produces an object, and never functions, until #26 is clarified).

Comment 28

[Brendan Eich [:brendan]]

(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

Comment 29

[Andreas Gal :gal]

Attachment: 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.

Comment 30

[Brendan Eich [:brendan]]

(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

Comment 31

[Andreas Gal :gal]

Attachment: 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.

Comment 32

[Andreas Gal :gal]

Attachment: patch

Rely on the proto supplied during proxy construction. This eliminates the double trap in lookupProperty, which was really nasty.

Comment 33

[Andreas Gal :gal]

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

Comment 34

[Andreas Gal :gal]

Attachment: 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.

Comment 35

[Brendan Eich [:brendan]]

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

Comment 36

[Andreas Gal :gal]

Attachment: patch

Fix a bug in the Function.prototype lookup in createFunction, and implement custom trace hook since Object/FunctionProxy are non-native objects.

Comment 37

[Andreas Gal :gal]

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.

Comment 38

[Andreas Gal :gal]

Filed as bug 547046.

Comment 39

[Andreas Gal :gal]

Attachment: patch

Follow the spec and make call and construct arguments to createFunction and store them in the proxy, not the handler.

Comment 40

[Andreas Gal :gal]

Attachment: 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.

Comment 41

[Andreas Gal :gal]

Attachment: 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.

Comment 42

[Andreas Gal :gal]

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

Comment 43

[Andreas Gal :gal]

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.

Comment 44

[Andreas Gal :gal]

Attachment: patch

Refreshed patch to tracemonkey-tip.

Comment 45

[Andreas Gal :gal]

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.

Comment 46

[Andreas Gal :gal]

Attachment: patch

Use the new typeOf hook provided by bug 547314.

Comment 47

[Andreas Gal :gal]

Attachment: patch with first round of fixes for bugs reported by Tom

Comment 48

[Andreas Gal :gal]

Attachment: patch

suppress unexpected __iterator__ property access during iteration

Comment 49

[Andreas Gal :gal]

Attachment: drop invoke() and renaming to match latest spec update

Comment 50

[Andreas Gal :gal]

Attachment: patch

Remove accidental indentation changes to reduce patch footprint.

Comment 51

[Tom Van Cutsem]

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

Comment 52

[Tom Van Cutsem]

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

Comment 53

[Andreas Gal :gal]

Thanks a lot Tom. I will import the tests and start working my way through them.

Comment 54

[Andreas Gal :gal]

Attachment: patch

Import proxy test suite. Make Proxy.fix return true/false properly.

Comment 55

[Andreas Gal :gal]

> [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

Comment 56

[Andreas Gal :gal]

Attachment: patch

Comment 57

[Andreas Gal :gal]

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.

Comment 58

[Andreas Gal :gal]

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.

Comment 59

[Brendan Eich [:brendan]]

(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

Comment 60

[Andreas Gal :gal]

Attachment: refreshed for TM tip

Comment 61

[Andreas Gal :gal]

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

Comment 62

[Andreas Gal :gal]

Attachment: 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.

Comment 63

[Tom Van Cutsem]

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.

Comment 64

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 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.

Comment 65

[Blake Kaplan (:mrbkap) (inactive)]

Attachment: 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.

Comment 66

[Andreas Gal :gal]

Attachment: 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

Comment 67

[Andreas Gal :gal]

Attachment: patch

Add support to proxy existing objects (JS API only).

Comment 68

[Andreas Gal :gal]

Attachment: patch

Expose sufficient internals to make wrappers in xpconnect possible.

Comment 69

[Andreas Gal :gal]

Attachment: always stringify property names

Comment 70

[Andreas Gal :gal]

Attachment: refreshed patch

Comment 71

[Andreas Gal :gal]

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

[Igor Bukanov]

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.

Comment 73

[Andreas Gal :gal]

Thanks for the quick review! I will fix the issues and put up a new patch pronto.

Comment 74

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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.

Comment 75

[Andreas Gal :gal]

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

Comment 76

[Igor Bukanov]

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

Comment 77

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

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!

Comment 78

[Andreas Gal :gal]

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

Comment 79

[Igor Bukanov]

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

Comment 80

[Andreas Gal :gal]

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

[Igor Bukanov]

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

Comment 82

[Andreas Gal :gal]

Attachment: refreshed patch, no longer any prerequisits required (just this patch)

Comment 83

[Andreas Gal :gal]

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.

Comment 84

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 85

[Andreas Gal :gal]

Attachment: patch

Refreshed patch, this time for real. I think.

Comment 86

[Igor Bukanov]

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

Comment 87

[Andreas Gal :gal]

Yeah, I will ignore the enumeration issue while we fix the protocol and look into lookupProperty/dropProperty next.

Comment 88

[Andreas Gal :gal]

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.

Comment 89

[Andreas Gal :gal]

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.

Comment 90

[Ben Newman (:bnewman) (:benjamn)]

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

Comment 91

[Andreas Gal :gal]

Attachment: 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.

Comment 92

[Andreas Gal :gal]

Attachment: patch

added missing files

Comment 93

[Andreas Gal :gal]

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

Comment 94

[Andreas Gal :gal]

Mark suggested that we might want to drop the stringification of property ids after all.

Comment 95

[Andreas Gal :gal]

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

Comment 96

[Andreas Gal :gal]

Attachment: 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.

Comment 97

[Andreas Gal :gal]

Attachment: patch

Passes my internal test suite (will try Tom's suite next).

Comment 98

[Andreas Gal :gal]

Attachment: patch

Comment 99

[Andreas Gal :gal]

Attachment: patch

Comment 100

[Andreas Gal :gal]

Attachment: add shell wrap command

Comment 101

[Andreas Gal :gal]

Attachment: restore className masquerading

Comment 102

[Andreas Gal :gal]

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

Comment 103

[Mark S. Miller]

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

Comment 104

[Andreas Gal :gal]

Yes, isTrapping will be removed for sure. It was never part of the spec.

Comment 105

[Andreas Gal :gal]

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.

Comment 106

[Andreas Gal :gal]

Attachment: patch

only enable Proxy.isTrapping and Proxy.fix in DEBUG builds

Comment 107

[Andreas Gal :gal]

Attachment: fix reserved slot count

Comment 108

[Andreas Gal :gal]

Attachment: fix embarassing bug in JSITER_HIDDEN flag declaration

Comment 109

[Andreas Gal :gal]

Attachment: fix bug in proxy enumeration id conversion

The patch seems reasonable stable for general web use.

Comment 110

[Andreas Gal :gal]

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!

Comment 111

[Andreas Gal :gal]

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)

Comment 112

[Andreas Gal :gal]

Attachment: removed unused construct atom

Comment 113

[Andreas Gal :gal]

Attachment: refreshed for TM tip

Comment 114

[Igor Bukanov]

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

Comment 115

[Andreas Gal :gal]

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

Comment 116

[Andreas Gal :gal]

Attachment: 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) ...".

Comment 117

[Andreas Gal :gal]

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

Comment 118

[Igor Bukanov]

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

Comment 119

[Andreas Gal :gal]

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

[Igor Bukanov]

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

Comment 121

[Andreas Gal :gal]

> 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

[Igor Bukanov]

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

Comment 123

[Andreas Gal :gal]

Attachment: patch

Comment 124

[Andreas Gal :gal]

> 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

[Igor Bukanov]

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

Comment 126

[Andreas Gal :gal]

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

Comment 127

[Igor Bukanov]

(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

[Igor Bukanov]

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

Comment 129

[Brendan Eich [:brendan]]

(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

Comment 130

[Brendan Eich [:brendan]]

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

/be

Comment 131

[Andreas Gal :gal]

Yeah, I am jumping on this. We can eliminate dropProperty altogether. Lovely.

Comment 132

[Igor Bukanov]

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

[Igor Bukanov]

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

Comment 134

[Andreas Gal :gal]

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

[Igor Bukanov]

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

Comment 136

[Luke Wagner [:luke]]

(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

[Igor Bukanov]

(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

[Igor Bukanov]

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

Comment 139

[Andreas Gal :gal]

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.

Comment 140

[Andreas Gal :gal]

Attachment: interdiff

Comment 141

[Andreas Gal :gal]

Attachment: patch

Comment 142

[Andreas Gal :gal]

Attachment: interdiff

Comment 143

[Andreas Gal :gal]

Attachment: patch

Rework code to use JSPropertyDescriptor instead of a property descriptor object.

Comment 144

[Andreas Gal :gal]

Attachment: mark proxy private during gc

Comment 145

[Andreas Gal :gal]

Attachment: patch

Comment 146

[Andreas Gal :gal]

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 147

[Blake Kaplan (:mrbkap) (inactive)]

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

Comment 148

[Andreas Gal :gal]

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

Comment 149

[Andreas Gal :gal]

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

Comment 150

[Andreas Gal :gal]

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

Comment 151

[Brendan Eich [:brendan]]

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

Comment 152

[Jeff Walden [:Waldo]]

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

Comment 153

[Andreas Gal :gal]

Pushed to tracemonkey.

http://hg.mozilla.org/tracemonkey/rev/4dd9be00049c

Comment 154

[Andreas Gal :gal]

Forgot to add the test directory to the makefile.

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

Comment 155

[Andreas Gal :gal]

2nd attempt to make the browser test harness happy

http://hg.mozilla.org/tracemonkey/rev/67e1c38f60fd

Comment 156

[Andreas Gal :gal]

Attachment: patch to disable scripted proxies

Comment 157

[Andreas Gal :gal]

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.

Comment 158

[Andreas Gal :gal]

Stringification implemented in bug 568051.

Comment 159

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/4dd9be00049c

Comment 160

[Andreas Gal :gal]

iterate trap implemented in bug 568413.

Comment 161

[Igor Bukanov]

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.

Comment 162

[Andreas Gal :gal]

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.

Comment 163

[Brendan Eich [:brendan]]

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

[Igor Bukanov]

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

Comment 165

[David Bruant]

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

I'd love to have your feedback.

Comment 166

[Eric Shepherd [:sheppy]]

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!