Eric Faust [:efaust] Assignee
	Description • 11 years ago

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.

	Jeff Walden [:Waldo]
	Comment 1 • 11 years ago

Sounds to me like a good incremental step toward proxies as the only way to have custom objects!  \o/

	Eric Faust [:efaust] Assignee
	Comment 2 • 11 years ago

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.

	Jeff Walden [:Waldo]
	Comment 3 • 11 years ago

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.

	Eric Faust [:efaust] Assignee
	Comment 4 • 11 years ago

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.

	Jeff Walden [:Waldo]
	Comment 5 • 11 years ago

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.

	Bobby Holley (:bholley)
	Comment 14 • 11 years ago

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.

	Bobby Holley (:bholley)
	Comment 15 • 11 years ago

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.

	Bill McCloskey [inactive unless it's an emergency] (:billm)
	Comment 16 • 11 years ago

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?

	Bobby Holley (:bholley)
	Comment 18 • 11 years ago

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|?

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 19 • 11 years ago

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

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 20 • 11 years ago

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.

	Jeff Walden [:Waldo]
	Comment 21 • 11 years ago

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.

	Jeff Walden [:Waldo]
	Comment 22 • 11 years ago

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.

	Jeff Walden [:Waldo]
	Comment 23 • 11 years ago

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.

	Jeff Walden [:Waldo]
	Comment 24 • 11 years ago

PROXY_MINIMUM_SLOTS seems like a clearer name than PROXY_NUM_SLOTS, to me.

	Boris Zbarsky [:bzbarsky]
	Comment 25 • 11 years ago

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.

	Boris Zbarsky [:bzbarsky]
	Comment 26 • 11 years ago

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.

	Jason Orendorff [:jorendorff]
	Comment 27 • 11 years ago

(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.

	Eric Faust [:efaust] Assignee
	Comment 28 • 11 years ago

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/32aeb054f757
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/488ab195f1ae
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d038c9b4c7dc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/1a0f8903f3ce
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/ae3ceb166be1
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/aac3d84c27c0
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/d3af8463dddc
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/c1bd9e2e41b2
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/beb52f820ac5

	Carsten Book [:Tomcat]
	Comment 29 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/32aeb054f757
https://hg.mozilla.org/mozilla-central/rev/488ab195f1ae
https://hg.mozilla.org/mozilla-central/rev/d038c9b4c7dc
https://hg.mozilla.org/mozilla-central/rev/1a0f8903f3ce
https://hg.mozilla.org/mozilla-central/rev/ae3ceb166be1
https://hg.mozilla.org/mozilla-central/rev/aac3d84c27c0
https://hg.mozilla.org/mozilla-central/rev/d3af8463dddc
https://hg.mozilla.org/mozilla-central/rev/c1bd9e2e41b2
https://hg.mozilla.org/mozilla-central/rev/beb52f820ac5

	Carsten Book [:Tomcat]
	Comment 30 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/5ff73743d1d3

	Phil Ringnalda (:philor)
	Comment 31 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/9c264aed5f9e