911553 - Proxy cannot trap ToPrimitive toString/valueOf calls

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[André Bargull [:anba]]

Test case:
""+new Proxy({toString:() => "inner toString"}, {get:(t, pk) => (pk === "toString" ? () => "proxy toString" : t[pk])})

Expected:
"proxy toString" is returned

Actual:
"inner toString" is returned


The same problem can be reproduced with "valueOf":
""+new Proxy({valueOf:() => "inner valueOf"}, {get:(t, pk) => (pk === "valueOf" ? () => "proxy valueOf" : t[pk])})

Comment 1

[Yaron Tausky (please use my Mozilla profile)]

Attachment: 911553_fix_proxy_defaultvalue.patch v1

I think I found the problem, but I'm not sure who could review this. Any suggestions?

Comment 2

[Till Schneidereit [:till]]

Comment on attachment 798540 [details] [diff] [review]
911553_fix_proxy_defaultvalue.patch v1

I'm volunteering efaust.

Comment 3

[Jeff Walden [:Waldo]]

A |using BaseProxyHandler::defaultValue;| would be simpler, if possibly less expressive/breakpointable.  I dunno which I'd prefer.

Comment 4

[Eric Faust [:efaust]]

Comment on attachment 798540 [details] [diff] [review]
911553_fix_proxy_defaultvalue.patch v1

I volunteer bholley. ;)

Comment 5

[Bobby Holley (:bholley)]

Comment on attachment 798540 [details] [diff] [review]
911553_fix_proxy_defaultvalue.patch v1

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

This works, but it leaves us with a a slightly unintuitive inheritance hierarchy:

       BaseProxyHandler      (Runs the [[DefaultValue]] algorithm on the proxy, invoking other traps)
              ||
              ||       
       DirectProxyHandler    (Runs the [[DefaultValue]] algorithm on the target object, ignoring handler traps)
        //    ||       \\
       //     ||        \\
     SPH    Wrapper     SIPH

Where both ScriptedProxyHandler and ScriptedIndirectProxyHandler explicitly need to bounce back to the BaseProxyHandler version, and Wrapper uses the DPH version, which actually exists for Wrapper-related and non-ES6-related reasons.

So I think the more correct solution is to hoist the DirectProxyHandler::defaultValue override into Wrapper, and remove the overrides in SPH and SIPH.

Comment 6

[Yaron Tausky (please use my Mozilla profile)]

Attachment: 911553_fix_proxy_defaultvalue.patch v2

Right, it does make more sense. Here's the revised patch.

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 799351 [details] [diff] [review]
911553_fix_proxy_defaultvalue.patch v2

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

This looks great. r=bholley

Thanks for the patch!

::: js/src/jswrapper.cpp
@@ +22,5 @@
>  using namespace js::gc;
>  
>  int js::sWrapperFamily;
>  
> +bool

Please add a comment here indicating that wrappers forward directly to the target for efficiency, and transparency in a few edge cases.

Comment 8

[Yaron Tausky (please use my Mozilla profile)]

Attachment: 911553_fix_proxy_defaultvalue.patch v3

Okay, added the comment. Carrying over the r+.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/12e4307ff333

Comment 10

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/12e4307ff333