Closed Bug 921448 Opened 11 years ago Closed 11 years ago

Unify Function and Object Proxies

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: bholley, Assigned: bholley)

References

Details

Attachments

(6 files)

The current distinction is totally wacky, and fixing it has been on my list for a while. Some recent refactoring seems to have made it even more unwieldy, so I think it's time. 

For the most part, the dichotomy is an artifact of the indirect proxy days, which gave us separate Proxy.create() and Proxy.createFunction() APIs, the latter of which took 2 extra caller-supplied objects that served as the call and construct traps. Because these things weren't baked into proxy handler (as they are with DirectProxies), we needed extra slots to store them. Yuck.

Currently, we only ever create a FunctionProxyObject for indirect proxies. But we use FunctionObjectProxy::class_ for any ObjectProxyObject that happens to be callable. Total mess.

I've got patches that seem to do the trick. I'm going to push 'em to try and the upload for review.
Blocks: 921454
Green. Uploading patches and flagging for review.
Function proxies are going away with these patches. First, let's stop pretending
like they're equal citizens with regular proxies.
Attachment #811570 - Flags: review?(ejpbruel)
There's no reason to store the target's [[Call]] in the reserved slot. If
there's no scripted call trap on the handler, DirectProxyHandler::call will
forward to the target, and we'll get that for free.
Attachment #811571 - Flags: review?(ejpbruel)
\o/
Attachment #811573 - Flags: review?(ejpbruel)
Comment on attachment 811573 [details] [diff] [review]
Part 6 - Remove FunctionProxyObjects. v1

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

::: js/src/jsfriendapi.cpp
@@ -527,5 @@
> -    const js::Class *clasp = GetObjectClass(obj);
> -    if (clasp == js::ObjectProxyClassPtr ||
> -        clasp == js::OuterWindowProxyClassPtr ||
> -        clasp == js::FunctionProxyClassPtr)
> -    {

I'm particularly happy to see this condition simplified :)
Apologies in advance for this stupid question:

How does the new scheme implement the following behavior?

  js> typeof new Proxy(Math, {})
  "object"
  js> typeof new Proxy(parseInt, {})
  "function"

I flipped through these patches quickly but didn't see where JSObject::isCallable is modified. It seems like it would have to be changed in order to get this right (JSOP_TYPEOF -> TypeOfOperation -> TypeOfValue -> TypeOfObject -> JSObject::isCallable).
(In reply to Jason Orendorff [:jorendorff] from comment #11)
> Apologies in advance for this stupid question:
> 
> How does the new scheme implement the following behavior?
> 
>   js> typeof new Proxy(Math, {})
>   "object"
>   js> typeof new Proxy(parseInt, {})
>   "function"
> 
> I flipped through these patches quickly but didn't see where
> JSObject::isCallable is modified. It seems like it would have to be changed
> in order to get this right (JSOP_TYPEOF -> TypeOfOperation -> TypeOfValue ->
> TypeOfObject -> JSObject::isCallable).

We still use two different JSClasses: ProxyObject::callableClass_ and ProxyObject::uncallableClass_. I think we should eventually fix things up so that proxies can decide dynamically whether they have a call hook or not, but that's fodder for another bug.
Comment on attachment 811568 [details] [diff] [review]
Part 1 - Implement sane default behavior for fun_toString for all proxies. v1

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

::: js/src/jsfun.cpp
@@ +781,5 @@
>  JSString *
>  fun_toStringHelper(JSContext *cx, HandleObject obj, unsigned indent)
>  {
>      if (!obj->is<JSFunction>()) {
> +        if (obj->is<ProxyObject>())

This is now possible due to the change you made in BaseProxyHandler::fun_toString, right? And that should also allow us to use isCallable to determine how to toSource(). Very nice!

::: js/src/jsproxy.cpp
@@ +310,5 @@
>  {
> +    if (proxy->isCallable())
> +        return JS_NewStringCopyZ(cx, "function () {\n    [native code]\n}");
> +    ReportIsNotFunction(cx, ObjectValue(*proxy));
> +    return NULL;

Right. This allows us to call fun_toString on any proxy.

@@ +2692,5 @@
>      AutoEnterPolicy policy(cx, handler, proxy, JSID_VOIDHANDLE,
>                             BaseProxyHandler::GET, /* mayThrow = */ false);
>      // Do the safe thing if the policy rejects.
> +    if (!policy.allowed())
> +        return handler->BaseProxyHandler::fun_toString(cx, proxy, indent);

So if the policy does not allow fun_toString we fall back to the base implementation, which doesn't expose any information about the function. Makes sense.
Attachment #811568 - Flags: review?(ejpbruel) → review+
Comment on attachment 811569 [details] [diff] [review]
Part 2 - Use callability rather than object classes when determining how to toSource(). v1

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

::: js/src/jsfun.cpp
@@ -829,5 @@
>      if (!obj)
>          return false;
>  
>      RootedString str(cx);
> -    if (obj->is<JSFunction>() || obj->is<FunctionProxyObject>())

This makes so much more sense. Nice!
Attachment #811569 - Flags: review?(ejpbruel) → review+
Comment on attachment 811570 [details] [diff] [review]
Part 3 - Get rid of weird demultiplexing NewProxyObject overload. v1

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

::: js/src/jsproxy.cpp
@@ -3247,5 @@
> -{
> -    if (!call && !construct)
> -        return ProxyObject::New(cx, handler, priv, TaggedProto(proto), parent, ProxyNotCallable);
> -
> -    return FunctionProxyObject::New(cx, handler, priv, proto, parent, call, construct);

I honestly have no idea what I was thinking here. Thanks for getting rid of it.

@@ +3308,5 @@
> +    ProxyObject *proxy = target->isCallable()
> +      ? FunctionProxyObject::New(cx, &ScriptedDirectProxyHandler::singleton,
> +                                 priv, proto, cx->global(), target, target)
> +      : ProxyObject::New(cx, &ScriptedDirectProxyHandler::singleton,
> +                         priv, TaggedProto(proto), cx->global(), ProxyNotCallable);

Yeah, this makes much more sense. The decision whether we should create a FunctionProxyObject is based on the callability of the target, so should be made here, not in NewProxyObject.

@@ +3378,5 @@
>  
>      RootedValue priv(cx, ObjectValue(*handler));
> +    JSObject *proxy =
> +      FunctionProxyObject::New(cx, &ScriptedIndirectProxyHandler::singleton,
> +                               priv, proto, parent, call, construct);

Great!
Attachment #811570 - Flags: review?(ejpbruel) → review+
Attachment #811571 - Flags: review?(ejpbruel) → review+
Comment on attachment 811572 [details] [diff] [review]
Part 5 - Stop using FunctionObjectProxies for ScriptedIndirectProxies. v1

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

Right, so if I understand this correctly, the CallConstructHolder allows us to store both the call and construct field in a single slot on ProxyObject, rather than use a separate FunctionProxyObject. ScriptedIndirectProxyHandlers *know* that slot is there, so we don't even need a check for it.
Attachment #811572 - Flags: review?(ejpbruel) → review+
Comment on attachment 811573 [details] [diff] [review]
Part 6 - Remove FunctionProxyObjects. v1

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

This is all straightforward mechanical stuff.

I noticed that we define ProxyObject::New in jsproxy.cpp, but all its other members in vm/ProxyObject.cpp. As a final change, can I suggest we move ProxyObject::New to vm/ProxyObject.cpp as well? r+ with that.

This is all great cleanup Bobby. Awesome work!

::: js/src/jsfriendapi.cpp
@@ -527,5 @@
> -    const js::Class *clasp = GetObjectClass(obj);
> -    if (clasp == js::ObjectProxyClassPtr ||
> -        clasp == js::OuterWindowProxyClassPtr ||
> -        clasp == js::FunctionProxyClassPtr)
> -    {

Second that!
Attachment #811573 - Flags: review?(ejpbruel) → review+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: