966518 - Move Proxy Handlers to Proxy JSClasses

Status: NEW

(Core :: JavaScript Engine, defect)

Description

[Eric Faust [:efaust]]

Now that we can have custom JSClasses for Proxies, we can remove the ProxyHandler from a reserved slot and hang it off the JSClass.

This is interesting for the DOM, because they want to unify the slot layouts between native objects and proxies that implement various DOM objects in the new WebIDL bindings.

This is a good first step for that.

As a pleasant side efftect, ee can then free the JSClass flag I just used to indicate that the class is a proxy, and just check that it has a handler at all to perform that check instead.

Comment 1

[Eric Faust [:efaust]]

Attachment: stop the jits referencing the Handler directly from the object v-1

Imagine that the proxy handler did live in a field on js::Class. Does this look reasonable?

Comment 2

[Kannan Vijayan [:djvj]]

Comment on attachment 8370367 [details] [diff] [review]
stop the jits referencing the Handler directly from the object v-1

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

Looks good overall.  Caught a minor register shuffling bug.

::: js/src/jit/BaselineIC.cpp
@@ +3215,4 @@
>  
>      // At this point, if not checking for an expando object, just return.
>      if (!checkExpandoShapeAddr)
>          return;

The pushed |tempVal| above is not popped when control flow reaches |classSuccess|.

Subsequently, if |checkExpandoShapeAddr| is true at compile time, the codegen method will return, and the generated code will leave a pushed tempVal on the stack.

Comment 3

[Eric Faust [:efaust]]

Attachment: Part 0: Conxtexprify DOMProxyHandler's constructor to allow for a static singleton

Comment 4

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8370544 [details] [diff] [review]
Part 0: Conxtexprify DOMProxyHandler's constructor to allow for a static singleton

r=me

Comment 5

[Eric Faust [:efaust]]

Attachment: Just get the browser building

Nothing will work yet, because rewrapping hasn't been addressed, but it's some 30k just to get the browser working. I welcome input about style; I've tried to minimize the suck, but it's fairly pervasive.

Comment 6

[Eric Faust [:efaust]]

https://tbpl.mozilla.org/?tree=Try&rev=71c8bde91ebe

It looks like everything is up and running. I can start the browser and scroll around your favorite website. Time to see what's really going on ;)

Comment 7

[Bobby Holley (:bholley)]

Comment on attachment 8370891 [details] [diff] [review]
Just get the browser building

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

efaust is going to get rid of this callable/uncallable stuff by adding BaseProxyHandler::isCallable. Everything else looks reasonable at first glance.

Comment 8

[Eric Faust [:efaust]]

Attachment: Get the shell up with BaseProxyHandler::isCallable()

This takes care of the comments that Bobby posted, so he should take a look if he thinks it works out cleaner (I certainly think it does).

This required some jit changes, so I would appreciate it if Jan could take a look at those. In particular, should we restructure visitIsCallable() in some way?

Comment 9

[Jan de Mooij [:jandem]]

Comment on attachment 8377297 [details] [diff] [review]
Get the shell up with BaseProxyHandler::isCallable()

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

Looks good.

::: js/src/jit/CodeGenerator.cpp
@@ +7940,5 @@
> +    Label notProxy, notFunction, done;
> +    // Just skim proxies off. Their notion of isCallable() is more complicated.
> +    masm.branchTestObjectIsProxy(false, object, output, &notProxy);
> +
> +    saveVolatile(output);

It'd be nice to move this (uncommon) case to an OOL path (saveVolatile/restoreVolatile emit a lot of code to save/restore xmm registers etc.)

@@ +7953,3 @@
>      masm.loadObjClass(object, output);
>  
> +    // An object is callable iff (is<JSFunction>() || getClass()->call.

Nit: missing )

::: js/src/jit/MCallOptimize.cpp
@@ +1539,5 @@
>          if (clasp) {
> +            if (clasp->isProxy() && clasp->proxyMaybeCallable()) {
> +                // We only know the answer for a proxy if we can say for sure that it's true
> +                isCallableKnown = true;
> +                isCallableConstant = true;

Is this correct, considering the name "proxyMaybeCallable"? The name seems to imply there are cases where proxyMaybeCallable() returns true but the proxy is not callable.

::: js/src/jsinfer.cpp
@@ +1747,1 @@
>              return true;

Would it make sense to do something like this:

    if (clasp && (clasp->isProxy() ? clasp->proxyMaybeCallable() : clasp->nonProxyCallable()))
        return true;

Comment 10

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #9)
> Is this correct, considering the name "proxyMaybeCallable"? The name seems
> to imply there are cases where proxyMaybeCallable() returns true but the
> proxy is not callable.

> Would it make sense to do something like this:
> 
>     if (clasp && (clasp->isProxy() ? clasp->proxyMaybeCallable() :
> clasp->nonProxyCallable()))

Er, ignore these two comments, I probably misunderstood proxyMaybeCallable. I thought if it returns false, you know for sure it isn't callable and if it returns true it may be. But instead, if it returns true you know it's callable and if it returns false you don't know.

I think definitelyCallable() or something would convey this better.

Comment 11

[Bobby Holley (:bholley)]

Comment on attachment 8377297 [details] [diff] [review]
Get the shell up with BaseProxyHandler::isCallable()

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

If this is an interdiff from the last patch, I'd expect to see some changes to js/xpconnect in here. Where are those? Ideally, I think I'd look at the non-interdiff changes there.

r=bholley on these parts with comments addressed. Please be very careful on the construct vs call stuff.

Thanks for doing this!

::: js/public/Class.h
@@ +615,5 @@
>          return flags & JSCLASS_EMULATES_UNDEFINED;
>      }
>  
> +    bool nonProxyCallable() const {
> +        JS_ASSERT(!isProxy()); 

Whitespace error.

@@ +625,1 @@
>      }

Hm. I think we should just have:

bool isDefinitelyCallable() const {
  return this == js::FunctionClassPtr || call_;
}

And then tweak JSObject::isCallable as noted below.

::: js/src/jsobjinlines.h
@@ +96,5 @@
> +        return true;
> +
> +    if (getClass()->call_)
> +        return true;
> +

This can then be replaced with getClass()->isDefinitelyCallable().

@@ +137,5 @@
> +        return clasp->construct_;
> +
> +    if (is<js::ProxyObject>()) {
> +        js::ProxyObject &p = as<js::ProxyObject>();
> +        if (p.handler()->isCallable(p.private_()))

Er, what? This should be isConstructable() I'd think.

::: js/src/jsproxy.cpp
@@ +652,5 @@
> +
> +bool
> +DirectProxyHandler::isConstructable(const Value &priv)
> +{
> +    return isCallable(priv);

This seems wrong. Don't we need to consult the constructability of the target?

::: js/src/jsproxy.h
@@ +191,5 @@
>  
> +    // Allow proxies, wrappers in particular, to specify callability at runtime based on the priv
> +    // value
> +    virtual bool isCallable(const Value &priv);
> +    virtual bool isConstructable(const Value &priv);

I think these traps should take JSObject *proxy, rather than assuming that |priv| is the only interesting value.

@@ +348,3 @@
>                      constructOp)
>  
> +/* 

Whitespace.

@@ +348,5 @@
>                      constructOp)
>  
> +/* 
> + * This provides a shorthand allowing various wrapper handlers to skip
> + * providing call or construct hooks, as they will be discovered dynamically

Needs a period, and we should mention that specifically, proxy_Call and proxy_Construct will be supplied as the call/construct hooks if isCallable/isConstructable return true.

Comment 12

[Eric Faust [:efaust]]

Attachment: Get the browser building with the new isCallable() semantics.

Comment 13

[Eric Faust [:efaust]]

Attachment: First (rough) pass at getting wrapper renewal fixed in the new world

Comment 14

[Eric Faust [:efaust]]

Brian, I am in particular curious about whether the compartment initialization in Wrapper::New needs a lock. How many different people can be running JS doing proxy creating in a given compartment at once?

Comment 15

[Bobby Holley (:bholley)]

Comment on attachment 8382723 [details] [diff] [review]
Get the browser building with the new isCallable() semantics.

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

Looks reasonable at a 3-minute pass.

::: js/xpconnect/wrappers/FilteringWrapper.cpp
@@ -193,5 @@
> -#define SCSOW FilteringWrapper<SameCompartmentSecurityWrapper, Opaque>
> -#define XOW FilteringWrapper<SecurityXrayXPCWN, CrossOriginAccessiblePropertiesOnly>
> -#define DXOW   FilteringWrapper<SecurityXrayDOM, CrossOriginAccessiblePropertiesOnly>
> -#define NNXOW FilteringWrapper<CrossCompartmentSecurityWrapper, Opaque>
> -#define GO FilteringWrapper<CrossCompartmentSecurityWrapper, GentlyOpaque>

Why do you need to get rid of the #defines here?

Comment 16

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8382724 [details] [diff] [review]
First (rough) pass at getting wrapper renewal fixed in the new world

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

Generally this looks good. I don't think you need to worry about locking. If you have an ExclusiveContext or a JSContext, you can assume that only one thread is allowed to touch any accessible compartment at a time.

Comment 17

[Eric Faust [:efaust]]

https://tbpl.mozilla.org/?tree=Try&rev=bda7956ca87f

I have fixed the problem causing assertion failures in shell tests, and it looks like there's still some namespacing issues to work out for some builds, but that's a lot of green, so that's promising.

Comment 18

[Brian Hackett [Laid off!]]

(In reply to Eric Faust [:efaust] from comment #14)
> Brian, I am in particular curious about whether the compartment
> initialization in Wrapper::New needs a lock. How many different people can
> be running JS doing proxy creating in a given compartment at once?

Sorry for the delay.  Except for PJS, only the main thread can run JS code.  None of the JS helper threads will create any proxies.

Comment 19

[Jason Orendorff [:jorendorff]]

This stack would likely perturb bug 1008791; might be worth rebasing and landing for that reason alone.

Comment 20

[Eric Faust [:efaust]]

Attachment: Part 0: Simplify later class additions by allowing proxy handlers to determine their callability dynamically

Bobby, this is mostly for you. In particular, I'm curious what you think of the isConstructor() hook for DirectProxyHandler. Is this something we should move to BaseProxyHandler, or something we should not rely on falling back on?

Maybe we should move the notion of "constructable if callable" to Wrapper instead, and not have an isConstructor trap in DirectProxyHandler? That might be best.

Kannan, can you take a look at the changes to visitIsCallable in ion? I am mostly concerned with that and32 instruction. Should we maybe make ObjectIsCallable return int instead, and upcast from the C++, instead?

Comment 21

[Eric Faust [:efaust]]

Attachment: Part 1: Make the VM ignore the contents of the handler slot on proxies

Waldo is real busy, so guess who wins ;)

Comment 22

[Eric Faust [:efaust]]

Attachment: Part 2: Make the jits ignore the contents of the proxy handler slot

Comment 23

[Eric Faust [:efaust]]

Attachment: Part 3: Change DOMProxyHandler::get_instance() to DOMProxyHandler::singleton to allow for Proxy Class creation

This can't land until bug 1027425 does, for b2g startup cost reasons, but we can at least get the ball rolling on reviews now.

Comment 24

[Eric Faust [:efaust]]

Attachment: Part 4: Get the browser building in the new world

This is split out for ease of reviewer selection. we will have to figure out how to roll this up at the end.

Comment 25

[Eric Faust [:efaust]]

Attachment: Part 5: Handle the "little business" of wrapper remapping

Huge props to billm for revamping the dead compartment logic, which cleared up the remaining leaks from this scheme.

Comment 26

[Eric Faust [:efaust]]

Attachment: Part 6: Final cleanups. Stop allocating proxy handler slot, and remove JSCLASS_IS_PROXY

This patch could go to anyone. Give it to Kanna to sanity check my jit isProxy changes.

Comment 27

[Bobby Holley (:bholley)]

Comment on attachment 8472535 [details] [diff] [review]
Part 0: Simplify later class additions by allowing proxy handlers to determine their callability dynamically

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

r=me with a follow filed and resourced for forcing null call/construct hooks and making everything go through the proxy handler.

::: js/public/Class.h
@@ +475,3 @@
>          return this == js::FunctionClassPtr || call;
>      }
> +    bool proxyDefinitelyCallable() const {

This is going to be meaningless soon, so let's not add it.

::: js/src/jsobj.cpp
@@ +3618,5 @@
>  bool
> +JSObject::isCallable() const
> +{
> +    if (is<JSFunction>())
> +        return true;

Please add a comment explaining the special-case.

@@ +3628,5 @@
>  {
>      if (is<JSFunction>()) {
>          const JSFunction &fun = as<JSFunction>();
>          return fun.isNativeConstructor() || fun.isInterpretedConstructor();
>      }

Same here.

::: js/src/jsproxy.cpp
@@ +669,5 @@
> +bool
> +DirectProxyHandler::isConstructor(JSObject *obj) const
> +{
> +    // The old ProxyOptions approach had all constructors if the target was
> +    // callable. Mimic that behavior here.

I think we should just implement this everywhere rather than doing the fallback thing. Long-term, we're going to want separate behavior for these two anyway.

@@ +1137,5 @@
>  
>      // The "proxy extra" slot index in which the handler is stored. Revocable proxies need to set
>      // this at revocation time.
>      static const int HANDLER_EXTRA = 0;
> +    static const int CALLABLE_EXTRA = 1;

Call this IS_CALLABLE_EXTRA

@@ +3028,5 @@
>      if (!proxy)
>          return false;
> +    proxy->as<ProxyObject>().setExtra(ScriptedDirectProxyHandler::HANDLER_EXTRA, ObjectValue(*handler));
> +    proxy->as<ProxyObject>().setExtra(ScriptedDirectProxyHandler::CALLABLE_EXTRA,
> +                                      BooleanValue(target->isCallable()));

Why do we need a slot for this at all? Can't it be dynamic?

@@ +3124,5 @@
> +    PROXY_CLASS_DEF("Proxy",
> +                    0,
> +                    0,
> +                    proxy_Call,
> +                    proxy_Construct);

Per IRC we want to move away from non-null hooks. We should just use the same JSClass here, and handle this issue in the proxy handler, right?

Comment 28

[Bobby Holley (:bholley)]

Comment on attachment 8472546 [details] [diff] [review]
Part 4: Get the browser building in the new world

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

r=me with comments addressed.

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +425,5 @@
>  
>      MOZ_ASSERT(AccessCheck::isChrome(JS::CurrentGlobalOrNull(cx)));
>      MOZ_ASSERT(targetAddon);
>  
> +    const Wrapper *wrapper = static_cast<const Wrapper *>(wrapperClass->proxyHandler);

Hm, any reason not remove this part and just compare class with class below?

@@ +552,3 @@
>  
>      if (existing)
> +        return Wrapper::Renew(cx, existing, obj, nullptr);

Per IRC, this is wrong.

::: js/xpconnect/wrappers/WrapperFactory.h
@@ +35,5 @@
>          return !js::CheckedUnwrap(obj);
>      }
>  
>      static bool IsCOW(JSObject *wrapper);
> +    static bool IsCOW(const js::Class *clasp);

What is this? I don't see it implemented anywhere. Presumably it should go?

Comment 29

[Peter Van der Beken [:peterv]]

Comment on attachment 8472543 [details] [diff] [review]
Part 3: Change DOMProxyHandler::get_instance() to DOMProxyHandler::singleton to allow for Proxy Class creation

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

::: dom/bindings/Codegen.py
@@ +9047,5 @@
>          else:
>              body = ""
> +        const = 'const ' if self.const else ''
> +        return '%s%s %s::%s%s;\n' % (const, self.type, cgClass.getNameString(),
> +                                      self.name, body)

Line this line up with the opening brace.

Comment 30

[Kannan Vijayan [:djvj]]

Comment on attachment 8472535 [details] [diff] [review]
Part 0: Simplify later class additions by allowing proxy handlers to determine their callability dynamically

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

No correctness issues, just style and some refactoring comments, and a minor codegen perf comment.  r=me after addressing comments (or indicating why they will not be addressed).

::: js/src/jit/CodeGenerator.cpp
@@ +8518,5 @@
> +        return false;
> +
> +    Label notFunction, done;
> +    // Just skim proxies off. Their notion of isCallable() is more complicated.
> +    masm.branchTestObjectIsProxy(true, object, output, ool->entry());

I noticed that branchTestObjectIsProxy does a loadObjClass(object, output) before checking some flags on the class.. but we do a redundant loadObjClass(obj, output) immediately after the call (below the call to |branchTestObjectIsProxy|).

It seems like it should be possible to refactor |branchTestObjectIsProxy| to to use an underlying |branchTestClassIsProxy| method, do the |loadObjClass(object, output)| before the call, and then call |branchTestClassIsProxy| with |output| as the register containing the classptr.

That should eliminate a redundant loadObjClass from this code.

@@ +8529,5 @@
>      masm.jump(&done);
>  
>      masm.bind(&notFunction);
> +    masm.cmpPtr(Address(output, offsetof(js::Class, call)), ImmPtr(nullptr));
> +    masm.emitSet(Assembler::NonZero, output);

I'm not sure why this is being split up into two calls... that seem identical in semantics to the single call to |cmpPtrSet| they replace.

@@ +8543,5 @@
> +{
> +    LIsCallable *ins = ool->ins();
> +    Register object = ToRegister(ins->object());
> +    Register output = ToRegister(ins->output());
> + 

Nit: remove whitespace on blank line.

::: js/src/jit/IonMacroAssembler.h
@@ +981,5 @@
>          Condition cond = truthy ? Assembler::Zero : Assembler::NonZero;
>          branchTest32(cond, flags, Imm32(JSCLASS_EMULATES_UNDEFINED), checked);
>      }
>  
> +    void branchTestObjectIsProxy(bool proxy, Register object, Register scratch, Label *label)

As noted in CodeGenerator.cpp, this might work better if it's refactored to use an underlying |branchTestClassIsProxy| method, which accepts a single read-only register, and then the body here can become:

  loadObjClass(object, scratch);
  branchTestClassIsProxy(proxy, scratch, label);

::: js/src/jit/MCallOptimize.cpp
@@ +1954,5 @@
>      } else {
>          types::TemporaryTypeSet *types = callInfo.getArg(0)->resultTypeSet();
>          const Class *clasp = types ? types->getKnownClass() : nullptr;
>          if (clasp) {
> +            if (clasp->isProxy() && clasp->proxyDefinitelyCallable()) {

From your IRC comments, I'm assuming this is going to become:

  if (clasp && !clasp->isProxy()) {
    isCallableKnown = true;
    isCallableConstant = clasp->nonProxyCallable();
  }

?

Comment 31

[Eric Faust [:efaust]]

(In reply to Kannan Vijayan [:djvj] from comment #30)
> 
> @@ +8529,5 @@
> >      masm.jump(&done);
> >  
> >      masm.bind(&notFunction);
> > +    masm.cmpPtr(Address(output, offsetof(js::Class, call)), ImmPtr(nullptr));
> > +    masm.emitSet(Assembler::NonZero, output);
> 
> I'm not sure why this is being split up into two calls... that seem
> identical in semantics to the single call to |cmpPtrSet| they replace.
> 

It's rebase gunk. The original patch was quite old. It's not intentional. It should clearly read cmpPtrSet.

> ::: js/src/jit/IonMacroAssembler.h
> @@ +981,5 @@
> >          Condition cond = truthy ? Assembler::Zero : Assembler::NonZero;
> >          branchTest32(cond, flags, Imm32(JSCLASS_EMULATES_UNDEFINED), checked);
> >      }
> >  
> > +    void branchTestObjectIsProxy(bool proxy, Register object, Register scratch, Label *label)
> 
> As noted in CodeGenerator.cpp, this might work better if it's refactored to
> use an underlying |branchTestClassIsProxy| method, which accepts a single
> read-only register, and then the body here can become:
> 
>   loadObjClass(object, scratch);
>   branchTestClassIsProxy(proxy, scratch, label);
> 

It's even better than that. We also get to clean up brachTestEmulatesUndefined, which currently does a by-hand check of branchTestIsProxyClass. I would like to have a single point where this check is done in the MacroAssembler, so this works out nicely.

> ::: js/src/jit/MCallOptimize.cpp
> @@ +1954,5 @@
> >      } else {
> >          types::TemporaryTypeSet *types = callInfo.getArg(0)->resultTypeSet();
> >          const Class *clasp = types ? types->getKnownClass() : nullptr;
> >          if (clasp) {
> > +            if (clasp->isProxy() && clasp->proxyDefinitelyCallable()) {
> 
> From your IRC comments, I'm assuming this is going to become:
> 
>   if (clasp && !clasp->isProxy()) {
>     isCallableKnown = true;
>     isCallableConstant = clasp->nonProxyCallable();
>   }
> 
> ?

That's correct.

Comment 32

[Kannan Vijayan [:djvj]]

Comment on attachment 8472542 [details] [diff] [review]
Part 2: Make the jits ignore the contents of the proxy handler slot

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

Canceling review until I understand what the MOZ_CRASH() stuff is about.  Otherwise it looks good.  Can you tell me why that code (where you put the MOZ_CRASH calls) should never get called anymore?

::: js/src/jit/BaselineIC.cpp
@@ +3250,5 @@
>  
> +    // Check that object is a DOMProxy.
> +    Label classSuccess;
> +    masm.loadPtr(checkProxyClassAddr, scratch);
> +    masm.branchTestObjClass(Assembler::Equal, object, tempVal.scratchReg(), scratch, &classSuccess);

It'd be clearer if these args were labeled:
  /* scratchReg = */ tempVal.scratchReg(),
  /* classReg = */ scratch

I had to go searching to ensure that they were being passed in the right order.

::: js/src/jsproxy.cpp
@@ +3002,5 @@
>      JS_ASSERT(!getClass()->ext.innerObject);
>      JS_ASSERT(getTaggedProto().isLazy());
>  
> +    MOZ_CRASH();
> +    //setHandler(handler);

Wut?  I'm guessing this is leftover debugging cruft you forgot to clean up.

::: js/src/jsproxy.h
@@ +422,5 @@
>  inline void
>  SetProxyHandler(JSObject *obj, BaseProxyHandler *handler)
>  {
>      JS_ASSERT(IsProxy(obj));
> +    MOZ_CRASH();

Ok the MOZ_CRASH stuff seems more deliberate to me now.  Can you explain the reasoning here?

Comment 33

[Kannan Vijayan [:djvj]]

Comment on attachment 8472591 [details] [diff] [review]
Part 6: Final cleanups. Stop allocating proxy handler slot, and remove JSCLASS_IS_PROXY

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

r=me with question addressed.

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

I don't get this change.  Why do we go from checking for the existence of proxyHandler before to checking !isNative after?

Is one equivalent to the other?

Comment 34

[Kannan Vijayan [:djvj]]

Comment on attachment 8472542 [details] [diff] [review]
Part 2: Make the jits ignore the contents of the proxy handler slot

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

Canceling review until I understand what the MOZ_CRASH() stuff is about.  Otherwise it looks good.  Can you tell me why that code (where you put the MOZ_CRASH calls) should never get called anymore?

::: js/src/jit/BaselineIC.cpp
@@ +3250,5 @@
>  
> +    // Check that object is a DOMProxy.
> +    Label classSuccess;
> +    masm.loadPtr(checkProxyClassAddr, scratch);
> +    masm.branchTestObjClass(Assembler::Equal, object, tempVal.scratchReg(), scratch, &classSuccess);

It'd be clearer if these args were labeled:
  /* scratchReg = */ tempVal.scratchReg(),
  /* classReg = */ scratch

I had to go searching to ensure that they were being passed in the right order.

::: js/src/jsproxy.cpp
@@ +3002,5 @@
>      JS_ASSERT(!getClass()->ext.innerObject);
>      JS_ASSERT(getTaggedProto().isLazy());
>  
> +    MOZ_CRASH();
> +    //setHandler(handler);

Wut?  I'm guessing this is leftover debugging cruft you forgot to clean up.

::: js/src/jsproxy.h
@@ +422,5 @@
>  inline void
>  SetProxyHandler(JSObject *obj, BaseProxyHandler *handler)
>  {
>      JS_ASSERT(IsProxy(obj));
> +    MOZ_CRASH();

Ok the MOZ_CRASH stuff seems more deliberate to me now.  Can you explain the reasoning here?

Comment 35

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 8472552 [details] [diff] [review]
Part 5: Handle the "little business" of wrapper remapping

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

This looks fine (although I feel like I'm missing some context from prior patches). However, please file a followup to remove all the proxy renewal code. We don't need that anymore. Some for the |existing| arguments all over the place in wrapping.

::: js/src/jscompartment.cpp
@@ +565,5 @@
>       * JSContext::global() remains valid.
>       */
>      if (enterCompartmentDepth && global_)
>          MarkObjectRoot(trc, global_.unsafeGet(), "on-stack compartment global");
> +

Please remove.

::: js/src/vm/ProxyObject.cpp
@@ +79,5 @@
>      proxy->setReservedSlot(slot, NullValue());
>  }
>  
> +/* static */ void
> +ProxyObject::nuke(JSContext *cx, ProxyObject *proxy, const Class *handlerClass)

Can you make |proxy| be a handle?

::: js/src/vm/ProxyObject.h
@@ +84,5 @@
>      }
>  
> +    static bool setWrapperShapeClass(JSContext *cx, ProxyObject *wrapper, const Class *clasp);
> +    static void setHandler(JSContext *cx, HandleObject proxy, const Class *handlerClass);
> +    static void NukeSlot(ProxyObject *proxy, uint32_t slot);

Please try to capitalize the names of static methods. We don't always follow that convention, but we usually do.

@@ +94,4 @@
>  
>      static void trace(JSTracer *trc, JSObject *obj);
>  
> +    static void nuke(JSContext *cx, ProxyObject *obj, const Class *handlerClass);

Same here.

Comment 36

[Jason Orendorff [:jorendorff]]

Comment on attachment 8472541 [details] [diff] [review]
Part 1: Make the VM ignore the contents of the handler slot on proxies

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

I have two big-ish questions about this (the last two comments below). r=me with those answered.

::: js/src/jsproxy.h
@@ +443,5 @@
>  
>  class MOZ_STACK_CLASS ProxyOptions {
>    protected:
>      /* protected constructor for subclass */
>      ProxyOptions(bool singletonArg)

While you're here, please make this constructor explicit.  (Even though it's not public.)

::: js/src/jswrapper.h
@@ +157,2 @@
>      static const CrossCompartmentWrapper singleton;
>      static const CrossCompartmentWrapper singletonWithPrototype;

Can these Handler singletons be made private, since the Class is public? (And similarly in other places.)

::: js/src/shell/js.cpp
@@ +4018,5 @@
>      args.rval().setString(str);
>      return true;
>  }
>  
> +/* These are only uses here for testing purposes */

s/uses/used/

::: js/src/vm/ProxyObject.cpp
@@ +100,5 @@
> +const Class ProxyObject::class_ = {
> +    "Proxy",
> +    JSCLASS_HAS_CACHED_PROTO(JSProto_Proxy),
> +    JS_PropertyStub,         /* addProperty */
> +    JS_DeletePropertyStub,   /* delProperty */

I'm a bit confused by this. Is this still used, with the patch? ISTM it should be deleted by this patch.

::: js/src/vm/ProxyObject.h
@@ +50,5 @@
>  
>      void initHandler(const BaseProxyHandler *handler);
>  
>      void setHandler(const BaseProxyHandler *handler) {
> +        MOZ_CRASH();

What's going on here? Seems like this method should just be removed, if we feel that strongly about it.

Even if the method is deleted in a later patch in the queue, this is really strange.

Comment 37

[Eric Faust [:efaust]]

Attachment: Part 0.5: Remove the notion of definitely callable proxies

Address the nits from bobby's initial review of Part 0.

Peter, since this patch contains some Codegen.py changes, I thought it would be good to have a peer look at them. the rest of it is probably not interesting for you.

Comment 38

[Bobby Holley (:bholley)]

Comment on attachment 8475420 [details] [diff] [review]
Part 0.5: Remove the notion of definitely callable proxies

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

r=bholley with comments addressed. I didn't look at the Codegen changes, since presumably peter will.

::: dom/base/nsGlobalWindow.cpp
@@ +647,5 @@
>      return false;
>    }
> +  virtual bool isConstructor(JSObject *obj) const MOZ_OVERRIDE {
> +    return false;
> +  }

Shouldn't we instead just get rid of both isCallable and isConstructor here, and just forward to the target (which is not callable and not constructible)?

I can't quite tell from the interdiff, but if there are other similar places (where we added an isCallable trap, but forwarding to the target would be just fine), then we should remove them too.

::: js/src/jsfriendapi.cpp
@@ +1233,5 @@
>  }
>  #endif /* JSGC_GENERATIONAL */
> +
> +JS_FRIEND_API(bool)
> +js::ForwardToNative(JSContext *cx, JSNative native, const CallArgs &args)

Why is this necessary? Isn't JS::CallArgs accessible in js/public?

::: js/src/jsproxy.h
@@ -297,5 @@
>      virtual bool regexp_toShared(JSContext *cx, HandleObject proxy,
>                                   RegExpGuard *g) const MOZ_OVERRIDE;
>      virtual JSObject *weakmapKeyDelegate(JSObject *proxy) const MOZ_OVERRIDE;
>      virtual bool isCallable(JSObject *obj) const MOZ_OVERRIDE;
> -    virtual bool isConstructor(JSObject *obj) const MOZ_OVERRIDE;

I think this should forward to the target instead of going away?

::: js/src/vm/ProxyObject.h
@@ +87,5 @@
>          // proxy_Trace is just a trivial wrapper around ProxyObject::trace for
>          // friend api exposure.
> +
> +        // Proxy classes are not allowed to have call or construct hooks directly. Their
> +        // callability is instead decided by a trap call

Nit - add a period.

::: js/xpconnect/wrappers/XrayWrapper.h
@@ +179,5 @@
>      virtual bool iterate(JSContext *cx, JS::Handle<JSObject*> proxy, unsigned flags,
>                           JS::MutableHandle<JS::Value> vp) const MOZ_OVERRIDE;
>  
>      virtual bool isCallable(JSObject *obj) const MOZ_OVERRIDE {
>          return false;

This override was added in the base patch, right? Shouldn't it go away now?

@@ +199,5 @@
>                        const JS::CallArgs &args) const MOZ_OVERRIDE;
>  
>      virtual bool isCallable(JSObject *obj) const MOZ_OVERRIDE {
>          return true;
>      }

This one too.

Comment 39

[Eric Faust [:efaust]]

Pushed part 0,0.5

https://hg.mozilla.org/integration/mozilla-inbound/rev/ed2fb19942d0

Comment 40

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed2fb19942d0

Comment 41

[Eric Faust [:efaust]]

(In reply to Jason Orendorff [:jorendorff] from comment #36)
> ::: js/src/jswrapper.h
> @@ +157,2 @@
> >      static const CrossCompartmentWrapper singleton;
> >      static const CrossCompartmentWrapper singletonWithPrototype;
> 
> Can these Handler singletons be made private, since the Class is public?
> (And similarly in other places.)

Almost. In general, I think it's the right idea to make all the singletons private. In practice, this is vaguely annoying because of the way XrayWrappers use templates. One way to get around these is to make all wrappers use the same class name (importantly not class_, which would make JSObject::is<CrossCompartmentWrapper>() a footgun) and then we can do that. As long as Bobby is OK with such a restriction, we can shuffle this stuff around. It's a little silly in this specific instance, but I think overall it's a good step.

In practice what this looks like is adding another wrapperHandler(const js::Class *clasp) to Wrapper, and making the XrayTraits functions that currently take Wrapper& either take a class and use that, or the callers call that. It doesn't sound all that painful.

Bobby, thoughts?

Comment 42

[Bobby Holley (:bholley)]

(In reply to Eric Faust [:efaust] from comment #41)
> In practice what this looks like is adding another wrapperHandler(const
> js::Class *clasp) to Wrapper, and making the XrayTraits functions that
> currently take Wrapper& either take a class and use that, or the callers
> call that. It doesn't sound all that painful.
> 
> Bobby, thoughts?

Didn't I already a review a patch that did that? Anyway, I'm mostly ok with that, but just make sure that the ergonomics don't get any worse. I don't think it's actually much of a problem for this to be public in practice.

Comment 43

[Boris Zbarsky [:bzbarsky]]

Eric, any idea when/whether this might happen?  It'd be nice to get rid of LoadDOMPrivate over in bug 1113014....

Comment 44

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 46

[Jan de Mooij [:jandem]]

(In reply to Release mgmt bot [:sylvestre / :calixte] from comment #44)
> The leave-open keyword is there and there is no activity for 6 months.
> :sdetar, maybe it's time to close this bug?

I think this would still be nice to fix at some point. However I fixed bug 1113014 and bug 1237504 without it and I don't think this blocks anything urgent.

Bug 1016130 comment 5 describes a complicated GC issue with wrapper renewing. The compartment/wrapper work may get rid of that so it's probably worth holding off on this bug for now.

Comment 47

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 48

[Steven DeTar [:sdetar]]

I am assuming we should still this needs to be left open based on Jan's comment in comment 46.

Comment 49

[BugBot [:suhaib / :marco/ :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 50

[Jan de Mooij [:jandem]]

(In reply to Release mgmt bot [:sylvestre / :calixte / :marco for bugbug] from comment #49)

The leave-open keyword is there and there is no activity for 6 months.
:sdetar, maybe it's time to close this bug?

Comment 46 still applies.