Closed Bug 929467 Opened 6 years ago Closed 5 years ago

Only make Proxy a constructor when target is a constructor

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla37

People

(Reporter: anba, Assigned: efaust)

References

Details

Attachments

(1 file)

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
Blocks: 978228
Duplicate of this bug: 1041756
Attached patch PatchSplinter Review
This applies on top of the callability patch (part 0-0.5) of bug 966518, and should fix the problem.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #8475477 - Flags: review?(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 :)
Attachment #8475477 - Flags: review?(jorendorff) → review+
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?
> O(n^2) to O(n)

where n is the length of a long chain of Proxy -> target which is also a Proxy -> ... -> target
(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.
What happened to this? I just noticed this while writing a test for Reflect.construct.
Flags: needinfo?(efaustbmo)
https://hg.mozilla.org/mozilla-central/rev/7cdfef0fddb8
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla37
Duplicate of this bug: 853987
You need to log in before you can comment on or make changes to this bug.