Allow multiple Proxy JSClasses

RESOLVED FIXED in mozilla29

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: efaust, Assigned: efaust)

Tracking

unspecified
mozilla29
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(9 attachments, 1 obsolete attachment)

2.73 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
7.47 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
3.02 KB, patch
bholley
: review+
Details | Diff | Splinter Review
3.13 KB, patch
billm
: review+
Details | Diff | Splinter Review
30.84 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
9.55 KB, patch
Waldo
: review+
Details | Diff | Splinter Review
8.87 KB, patch
bholley
: review+
Details | Diff | Splinter Review
2.19 KB, patch
bholley
: review+
Details | Diff | Splinter Review
3.88 KB, patch
bholley
: review+
Details | Diff | Splinter Review
Currently in the engine, there is a single JSClass for all ObjectProxies. This actively inhibits the DOM from storing relevant Class-scoped information about objects that happen to be implemented as proxies.

Boris has proposed we move to a model in which we rely on a flag check to see if a JSClass represents a proxy, and implement some macros which allow easy class implementation of non-overridden features.

Preliminary discussions at the Toronto Summit gave at least verbal approval of such a notion from both Jason and Jan.
Sounds to me like a good incremental step toward proxies as the only way to have custom objects!  \o/
According to my research (but mostly Waldo), these flags are juts unused in the engine. Since we are gonna need one to do this anyway, start by freeing up what we can.
Assignee: nobody → efaustbmo
Status: NEW → ASSIGNED
Attachment #824956 - Flags: review?(jwalden+bmo)
Comment on attachment 824956 [details] [diff] [review]
Part 0: Free up a JSClass Flag

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

::: js/public/Class.h
@@ +536,5 @@
>  #define JSCLASS_IS_GLOBAL               (1<<(JSCLASS_HIGH_FLAGS_SHIFT+1))
>  #define JSCLASS_INTERNAL_FLAG2          (1<<(JSCLASS_HIGH_FLAGS_SHIFT+2))
>  #define JSCLASS_INTERNAL_FLAG3          (1<<(JSCLASS_HIGH_FLAGS_SHIFT+3))
>  
> +// Bits 21 and 22 unused.

21/22 don't signal (1<<(JSCLASS_HIGH_FLAGS_SHIFT+(4 or 5))) to me.  I'd just rename the #defines to JSCLASS_RESERVEDFLAG[12] with a comment /* not used at present */ above.
Attachment #824956 - Flags: review?(jwalden+bmo) → review+
https://tbpl.mozilla.org/?tree=Try&rev=74284c8a7087

All the changes needed to write some macros for proxy class generation and call it a win. Green.
Oh, don't forget to update

https://developer.mozilla.org/en-US/docs/SpiderMonkey/31

for this.  For better or worse,

https://developer.mozilla.org/en-US/docs/SpiderMonkey/JSAPI_Reference/JSClass.flags

never mentioned this flags.  Oh well.
Attachment #8343414 - Flags: review?(bobbyholley+bmo)
Attachment #8343418 - Flags: review?(bobbyholley+bmo)
Comment on attachment 8343408 [details] [diff] [review]
Part 2: Standardize OuterWindowProxy to use the proxy Convert and HasInstance stubs

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

r=bholley with the defaultValue override on nsOuterWindowProxy removed.

::: dom/base/nsGlobalWindow.cpp
@@ +915,5 @@
> +nsOuterWindowProxy::defaultValue(JSContext *cx, JS::Handle<JSObject*> proxy,
> +                                 JSType hint, JS::MutableHandle<JS::Value> vp)
> +{
> +  return js::BaseProxyHandler::defaultValue(cx, proxy, hint, vp);
> +}

We don't need this override. nsOuterWindowProxy is not a security wrapper, so the inherited behavior from Wrapper is fine.
Attachment #8343408 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 8343414 [details] [diff] [review]
Part 6: Allow callers of Wrapper::New to specify a proxy class

Eric and I discussed a way to shrink this patch on IRC. He's going to do that.
Attachment #8343414 - Flags: review?(bobbyholley+bmo) → review-
Attachment #8343417 - Flags: review?(bobbyholley+bmo) → review+
Attachment #8343418 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 8343409 [details] [diff] [review]
Part 3: Allow future Proxy JSClasses to safely have more than 4 reserved slots.

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

::: js/src/jsproxy.cpp
@@ +3060,5 @@
> +    /*
> +     * Allow for people to add extra slots to "proxy" classes, without allowing
> +     * them to set their own trace hook. Trace the extras.
> +     */
> +    unsigned numSlots = JSCLASS_RESERVED_SLOTS(obj->getClass());

It's a bit clearer to use |proxy| instead of |obj| here.

::: js/src/jsproxy.h
@@ +349,5 @@
>   */
>  const uint32_t PROXY_PRIVATE_SLOT = 0;
>  const uint32_t PROXY_HANDLER_SLOT = 1;
>  const uint32_t PROXY_EXTRA_SLOT   = 2;
> +const uint32_t PROXY_NUM_SLOTS    = 4;

I haven't seen the other patches in this bug, but currently we have two places where we hard-code 4 as the number of reserved slots in jsproxy.cpp. Can you replace those with this const if that's still relevant?

::: js/src/vm/ProxyObject.cpp
@@ +95,5 @@
>      NukeSlot(this, EXTRA_SLOT + 1);
> +
> +    /* Allow people to add their own number of reserved slots beyond the expected 4 */
> +    unsigned numSlots = JSCLASS_RESERVED_SLOTS(getClass());
> +    for (unsigned i = PROXY_NUM_SLOTS; i < numSlots; i++)

It should be safe to start at 0 and eliminate the NukeSlot calls above.

::: js/src/vm/ProxyObject.h
@@ +70,5 @@
>          JS_ASSERT(n == 0 || n == 1);
>          return &getReservedSlotRef(EXTRA_SLOT + n);
>      }
>  
> +    HeapSlot *slotOfClassSpecific(size_t n) {

This name is not so good. How about classSpecificSlot?
Attachment #8343409 - Flags: review?(wmccloskey) → review+
Attachment #8343414 - Attachment is obsolete: true
Attachment #8343491 - Flags: review?(bobbyholley+bmo)
Comment on attachment 8343491 [details] [diff] [review]
Part 6: Allow callers of Wrapper::New to specify a proxy class v2

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

This is much nicer.

::: js/src/jsproxy.h
@@ +411,5 @@
>  
>  class MOZ_STACK_CLASS ProxyOptions {
> +  protected:
> +    /* protected constructor for subclass */
> +    ProxyOptions(bool argSingleton, const Class *argClasp)

Isn't the convention |singletonArg| and |claspArg|?
Attachment #8343491 - Flags: review?(bobbyholley+bmo) → review+
Comment on attachment 8343413 [details] [diff] [review]
Part 5: Allow callers of NewProxyObject() to specify a proxy class

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

::: js/src/jsproxy.h
@@ +397,5 @@
>  
>  class MOZ_STACK_CLASS ProxyOptions {
>    public:
> +    ProxyOptions() : singleton_(false),
> +                   clasp_(UncallableProxyClassPtr)

Fix indentation, please
Comment on attachment 8343417 [details] [diff] [review]
Part 7: Rename OuterWindowProxy::class_ nsOuterWindowProxyClass and move it to dom/base/nsGlobalWindow.cpp

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

::: dom/base/nsGlobalWindow.cpp
@@ +667,5 @@
>    bool AppendIndexedPropertyNames(JSContext *cx, JSObject *proxy,
>                                    JS::AutoIdVector &props);
>  };
>  
> +const js::Class nsOuterWindowProxyClass = {

Don't use ns prefixes in new code.
Comment on attachment 8343405 [details] [diff] [review]
Part 1: Use newly freed flags to implement IsProxy()

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

::: js/src/jit/IonMacroAssembler.h
@@ +925,4 @@
>  
> +        branchTest32(Assembler::NonZero, flags, Imm32(JSCLASS_IS_PROXY), slowCheck);
> +
> +        test32(flags, Imm32(JSCLASS_EMULATES_UNDEFINED));

We could combine these two into a single less-than check, or similar, if the two bits were the largest bits in flags (or similar if these were the lowest bits).  Would it win anything?  Might be worth a followup to at least investigate.  As far as I know, aside from the fields of bits used for JSProtoKey, there are no constraints on the ordering of the flags.

::: js/src/jsproxy.h
@@ +319,5 @@
>  extern JS_FRIEND_DATA(const js::Class* const) OuterWindowProxyClassPtr;
>  
>  inline bool IsProxyClass(const Class *clasp)
>  {
> +    return clasp->isProxy();

Please inline this into all callers and get rid of the method.  Followup patch, rs=me on it.
Attachment #8343405 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8343412 [details] [diff] [review]
Part 4: Create handy new friendapi macros for proxy class creation

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

::: js/src/jsfriendapi.h
@@ +228,5 @@
>  /*
> + * Helper Macros for creating JSClasses that function as proxies.
> + *
> + * NB: The macro invocation must be surrounded by braces, so as to
> + *     allow for potention JSClass extensions.

Rather than say "must be", give an example in the comment of exactly how to use the macro.  Right now if I wanted to use this I'd have to go find a user and copy it, or do some very careful reading of what this expands to.

Also, if this comment sticks around past that (which it probably shouldn't -- it should be clear from the example how to use it), "potential".

@@ +244,5 @@
> +#define PROXY_CLASS_WITH_EXT(name, extraSlots, flags, callOp, constructOp, ext)         \
> +    {                                                                                   \
> +        name,                                                                           \
> +        js::Class::NON_NATIVE |                                                         \
> +            JSCLASS_IS_PROXY |                                                          \

Weird alignment.
Attachment #8343412 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8343413 [details] [diff] [review]
Part 5: Allow callers of NewProxyObject() to specify a proxy class

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

::: js/src/jsproxy.h
@@ +413,5 @@
> +    ProxyOptions &setClass(const Class *argClasp) {
> +        clasp_ = argClasp;
> +        return *this;
> +    }
> +    ProxyOptions &setStandardClass(bool callable) {

This name, going with this argument, is weird.  Maybe "selectDefaultClass" instead?

::: js/src/vm/ProxyObject.h
@@ +84,5 @@
> +
> +        // proxy_Trace is just a trivial wrapper around ProxyObject::trace for
> +        // friend api exposure.
> +        return clasp->isProxy() &&
> +               clasp->flags & JSCLASS_IMPLEMENTS_BARRIERS &&

I think we parenthesize & expressions because the bitwise precedence is screwy.
Attachment #8343413 - Flags: review?(jwalden+bmo) → review+
PROXY_MINIMUM_SLOTS seems like a clearer name than PROXY_NUM_SLOTS, to me.
Once this all lands, can we move the proxy handler pointer into the JSClass instead of sticking it in a slot on the object?

Becuase then we could also get rid of the proxy private/extra slots and just have proxies have the same reserved slot setup as normal objects...

If that seems reasonable, I'll file a followup to do it.
I guess the main drawbacks would be the extra entry in JSClass for all other objects and the fact that you have to pointer-chase a bit more to get to the handler.  But you need less bitshifting as you do it, so who knows whether it's any slower in practice.
(In reply to Boris Zbarsky [:bz] from comment #25)
> Once this all lands, can we move the proxy handler pointer into the JSClass
> instead of sticking it in a slot on the object?

Yes please. This will surely not be any slower in practice.
Whiteboard: [qa-]
Duplicate of this bug: 664250
You need to log in before you can comment on or make changes to this bug.