Closed
Bug 921448
Opened 11 years ago
Closed 11 years ago
Unify Function and Object Proxies
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla27
People
(Reporter: bholley, Assigned: bholley)
References
Details
Attachments
(6 files)
2.81 KB,
patch
|
ejpbruel
:
review+
|
Details | Diff | Splinter Review |
885 bytes,
patch
|
ejpbruel
:
review+
|
Details | Diff | Splinter Review |
3.51 KB,
patch
|
ejpbruel
:
review+
|
Details | Diff | Splinter Review |
1.62 KB,
patch
|
ejpbruel
:
review+
|
Details | Diff | Splinter Review |
5.83 KB,
patch
|
ejpbruel
:
review+
|
Details | Diff | Splinter Review |
29.00 KB,
patch
|
ejpbruel
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=ba1ed567c7af
Assignee | ||
Comment 2•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=e29d9567d5cd
Assignee | ||
Comment 3•11 years ago
|
||
Green. Uploading patches and flagging for review.
Assignee | ||
Comment 4•11 years ago
|
||
Attachment #811568 -
Flags: review?(ejpbruel)
Assignee | ||
Comment 5•11 years ago
|
||
Attachment #811569 -
Flags: review?(ejpbruel)
Assignee | ||
Comment 6•11 years ago
|
||
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)
Assignee | ||
Comment 7•11 years ago
|
||
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)
Assignee | ||
Comment 8•11 years ago
|
||
Attachment #811572 -
Flags: review?(ejpbruel)
Comment 10•11 years ago
|
||
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 :)
Comment 11•11 years ago
|
||
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).
Assignee | ||
Comment 12•11 years ago
|
||
(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 13•11 years ago
|
||
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 14•11 years ago
|
||
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 15•11 years ago
|
||
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+
Updated•11 years ago
|
Attachment #811571 -
Flags: review?(ejpbruel) → review+
Comment 16•11 years ago
|
||
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 17•11 years ago
|
||
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+
Assignee | ||
Comment 18•11 years ago
|
||
https://tbpl.mozilla.org/?tree=Try&rev=749a8b2ba4f6
Assignee | ||
Comment 19•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?changeset=c55b7450097b
Comment 20•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/553f2ce03457 https://hg.mozilla.org/mozilla-central/rev/ed544130b96d https://hg.mozilla.org/mozilla-central/rev/875d92327646 https://hg.mozilla.org/mozilla-central/rev/fbb25bc1d0e4 https://hg.mozilla.org/mozilla-central/rev/62829945c612 https://hg.mozilla.org/mozilla-central/rev/df569a38d1b0 https://hg.mozilla.org/mozilla-central/rev/05abf18b5635
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
You need to log in
before you can comment on or make changes to this bug.
Description
•