929467 - Only make Proxy a constructor when target is a constructor

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

test case:
new (new Proxy(Object.create, {construct: () => {print("no!!!"); return {}} }))

Expected: 
TypeError is thrown because `Object.create` is not a constructor

Actual:
"construct" trap is called on proxy


See 9.3.16 [[Construct]] for Proxy Objects, NOTE 1:
https://people.mozilla.org/~jorendorff/es6-draft.html#sec-construct-internal-method

Comment 2

[Eric Faust [:efaust]]

Attachment: Patch

This applies on top of the callability patch (part 0-0.5) of bug 966518, and should fix the problem.

Comment 3

[Jason Orendorff [:jorendorff]]

Comment on attachment 8475477 [details] [diff] [review]
Patch

Review of attachment 8475477 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit-test/tests/proxy/testDirectProxyConstruct5.js
@@ +1,3 @@
> +load(libdir + "asserts.js");
> +
> +// Make sure that a proxy only has a [[Construct]] if the target is

...is what?

@@ +4,5 @@
> +
> +var handler = {};
> +var p = new Proxy(Math.sin, handler);
> +
> +assertThrowsInstanceOf(() => new p, TypeError, "Must on construct with non-constructor target");

sorry for the syntax nits, but this error message seems weird to me too. I guess I'd write something like "can't use 'new' on a Proxy with non-constructor target"

::: js/src/jsproxy.cpp
@@ +2255,5 @@
> +
> +bool
> +ScriptedDirectProxyHandler::isConstructor(JSObject *obj) const
> +{
> +    MOZ_ASSERT(obj->as<ProxyObject>().handler() == &ScriptedDirectProxyHandler::singleton);

This is bitrotted, but the new way is so much nicer :)

Comment 4

[Jason Orendorff [:jorendorff]]

On second thought, a comment as to why we are caching the result of target->isCallable() and isConstructor() would be pretty great. After all, we could delegate those methods like we delegate everything else.

My understanding is that we cache those bits because it turns call() from O(n^2) to O(n); there's not a correctness reason, is there?

Comment 5

[Jason Orendorff [:jorendorff]]

> O(n^2) to O(n)

where n is the length of a long chain of Proxy -> target which is also a Proxy -> ... -> target

Comment 6

[Eric Faust [:efaust]]

(In reply to Jason Orendorff [:jorendorff] from comment #4)

> My understanding is that we cache those bits because it turns call() from
> O(n^2) to O(n); there's not a correctness reason, is there?

Well, we also cache it because that's what the spec says. [[Call]] and [[Construct]] are assigned to the proxy at creation-time. In the face of revocation, we might not have the target anymore to query it, so we cache them. It does have the pleasant side effect of not having to do the recursion every time, though, to be sure.

Comment 7

[Tom S [:evilpie]]

What happened to this? I just noticed this while writing a test for Reflect.construct.

Comment 8

[Eric Faust [:efaust]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7cdfef0fddb8

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7cdfef0fddb8