1257102 - Scripted proxy code should use GetMethod, not GetProperty, to get proxy traps

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Driveby observed during other work.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Invoking a trap on a proxy, where the handler has null as the value of that trap, should fall back to operating on the target just like undefined would. NOT REVIEWED YET

Comment 3

[Tom S. (please needinfo tschuster)]

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

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

::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +147,5 @@
> +
> +    if (IsCallable(func))
> +        return true;
> +
> +    JS_ReportErrorNumber(cx, GetErrorMessage, nullptr, JSMSG_BAD_TRAP);

Don't you need to actually pass a trap name here?

@@ +247,4 @@
>  bool
>  ScriptedDirectProxyHandler::isExtensible(JSContext* cx, HandleObject proxy, bool* extensible) const
>  {
> +    // steps 1-3

I would prefer if we used // Steps 1-3. // Step 4. etc.
And I don't understand why you decided to group those three together.

@@ +258,3 @@
>      RootedObject target(cx, proxy->as<ProxyObject>().target());
>  
> +    // steps 5-6

This doesn't look right. GetMethod is just Step 5 with the new "?" syntax. This also applies to some other places, especially were we had implemented a draft.

::: js/src/tests/ecma_6/Proxy/trap-null.js
@@ +31,5 @@
> +  construct: null,
> +};
> +
> +var complainingHandler = new Proxy(allTraps, {
> +  get(target, p, receiver) {

\o/

Comment 4

[Tom S. (please needinfo tschuster)]

Comment on attachment 8731102 [details] [diff] [review]
Invoking a trap on a proxy, where the handler has null as the value of that trap, should fall back to operating on the target just like undefined would.  NOT REVIEWED YET

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

Please do fix the comments. I think on IRC we discussed using the ES7 draft as the reference.

Comment 5

[Jeff Walden [:Waldo]]

Attachment: Patch (only changing behavior, massive step-recommenting in a future patch if this approach passes muster)

Comment 6

[Tom S. (please needinfo tschuster)]

Comment on attachment 8731975 [details] [diff] [review]
Patch (only changing behavior, massive step-recommenting in a future patch if this approach passes muster)

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

::: js/src/proxy/ScriptedDirectProxyHandler.cpp
@@ +952,2 @@
>  bool
>  ScriptedDirectProxyHandler::call(JSContext* cx, HandleObject proxy, const CallArgs& args) const

Did you forget to split those out?

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/16e429ad5696

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/16e429ad5696