Nicholas Nethercote [inactive] Assignee
	Description • 11 years ago

Bug 884124 introduced some subclasses of JSObject:  DateObject, ArrayObject, etc.  This bug is for introducing more relating to proxies and wrappers.

	Nicholas Nethercote [inactive] Assignee
	Comment 2 • 11 years ago

This patch introduces a minimal ProxyObject class and some sub-classes.  The
next patch will move a bunch of stuff into them.

This patch also renames the existing "proxy object" as
|ProxyConstructorObject|, as recently discussed on the JS internals list
(https://groups.google.com/forum/#!topic/mozilla.dev.tech.js-engine.internals/cVX6EN4avFY)

Also, the introduction of the |*ClassPtr| values (as synonyms of
|*Object::class_|) has precedent -- we already have |FunctionClassPtr| -- and
is necessary for GetObjectProto() to be inlined.

	Nicholas Nethercote [inactive] Assignee
	Comment 4 • 11 years ago

I'm ambivalent about this patch... it makes wrapper objects and
cross-compartment wrapper objects follow the same pattern as all other JSObject
sub-classes, which is good.

But they are degenerate cases -- WrapperObject and
CrossCompartmentWrapperObject are both empty, and exist only so that
is<WrapperObject>() and is<CrossCompartmentWrapper>() can be defined -- so I
wonder if it represents an attempt at foolish consistency.  

A couple of notes:

- IsCrossCompartmentWrapper() stays because it's in the friend API and used by
  Gecko.

- getWrapperFamily() is dead.

	Nicholas Nethercote [inactive] Assignee
	Comment 5 • 11 years ago

Rebased after bug 788172.  Patch applies on top of the full patch stack in bug
889146.

	Nicholas Nethercote [inactive] Assignee
	Comment 6 • 11 years ago

rebased

	Nicholas Nethercote [inactive] Assignee
	Comment 7 • 11 years ago

rebased

	Nicholas Nethercote [inactive] Assignee
	Comment 8 • 11 years ago

Comment on attachment 770638 [details] [diff] [review]
(part 2) - Introduce ProxyObject and some sub-classes.

Ah crap, I just discovered that jsproxy.h is exported and that functions like GetProxyHandler() and IsProxy() are used outside the engine.  This totally borks my moving of various things from jsproxy.h to ProxyObject.h.  I'll need to rethink this.

	Nicholas Nethercote [inactive] Assignee
	Comment 9 • 11 years ago

Comment on attachment 768055 [details] [diff] [review]
(part 1) - Remove dead functions RenewProxyObject() and Wrapper::Renew().

Oh, this code isn't dead.  It's used by xpconnect.

	Nicholas Nethercote [inactive] Assignee
	Comment 11 • 11 years ago

This new version renames the exported JSSLOT_* constants to match the naming
scheme used by both SpiderMonkey and Gecko (i.e. "SLOT" at the end).  And I
removed the FunctionProxy-specific SLOT constants from the friend API because
they weren't needed.

	Nicholas Nethercote [inactive] Assignee
	Comment 12 • 11 years ago

As I said before, having empty classes is weird, but getting rid of the last
JSObject::isFoo() method (well, except for isObject()) is pretty sweet...

	Nicholas Nethercote [inactive] Assignee
	Comment 13 • 11 years ago

This should have been part of bug 880041 (it's just like the JSFunction patch
from that bug), but I didn't think of that at the time.  This bug will do.

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

Comment on attachment 771179 [details] [diff] [review]
(part 1) - Introduce ProxyObject and some sub-classes.

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

::: js/src/jscompartment.cpp
@@ +311,5 @@
>      RootedObject existing(cx, existingArg);
>      if (existing) {
>          /* Is it possible to reuse |existing|? */
>          if (!existing->getTaggedProto().isLazy() ||
> +            existing->getClass() != &ObjectProxyObject::class_ ||

I can just see someone wandering through this code and changing this line to read '->is<ObjectProxyObject>()'.  :-\  Since this is the only place we do this, add a one-line comment?

::: js/src/vm/ObjectImpl.cpp
@@ +567,5 @@
>      JS_CHECK_RECURSION(cx, return false);
>  
>      Rooted<PropertyId> pid(cx, pid_);
>  
> +    if (static_cast<JSObject *>(obj.get())->is<ProxyObject>())

Use Downcast here too?

::: js/src/vm/ScopeObject.cpp
@@ +1589,5 @@
>  
>  bool
>  js_IsDebugScopeSlow(JSObject *obj)
>  {
> +    return obj->hasClass(&ObjectProxyObject::class_) &&

...OK, here's one more place we're doing this. One-liner comment here too, if you feel like it.

::: js/src/vm/ScopeObject.h
@@ +651,5 @@
>  JSObject::is<js::DebugScopeObject>() const
>  {
>      extern bool js_IsDebugScopeSlow(JSObject *obj);
> +    return hasClass(&js::ObjectProxyObject::class_) &&
> +           js_IsDebugScopeSlow(const_cast<JSObject*>(this));

js_IsDebugScopeSlow already contains the check for ObjectProxyObject::class_. Remove the redundant check here?

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

Comment on attachment 771180 [details] [diff] [review]
(part 2) - Move various functions into ProxyObject and FunctionProxyObject.

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

::: js/src/jsproxy.cpp
@@ -1038,5 @@
>  ScriptedIndirectProxyHandler::fun_toString(JSContext *cx, HandleObject proxy, unsigned indent)
>  {
>      assertEnteredPolicy(cx, proxy, JSID_VOID);
> -    Value fval = GetCall(proxy);
> -    if (proxy->is<FunctionProxyObject>() &&

Oh, wow, the existing code here was weird. Good change.

@@ +3028,5 @@
>       * The GC can use the second reserved slot to link the cross compartment
>       * wrappers into a linked list, in which case we don't want to trace it.
>       */
> +    if (!IsCrossCompartmentWrapper(proxy))
> +        MarkSlot(trc, proxy->slotOfExtra(1), "extra1");

If slotOfExtra isn't needed otherwise, this could be named
    static void ProxyObject::trace(JSTracer *, JSObject *);
and slotOfExtra could be private.

@@ +3035,5 @@
>  static void
>  proxy_TraceFunction(JSTracer *trc, JSObject *obj)
>  {
>      // NB: If you add new slots here, make sure to change
>      // js::NukeChromeCrossCompartmentWrappers to cope.

Update the comment. (You can say "Note:" instead of "NB:", as an optional style thing.)

::: js/src/jsproxy.h
@@ +290,4 @@
>  /*
> + * These are part of the API.
> + *
> + * NOTE: PROXY_PRIVATE_SLOT is 0, because that way slot 0 is usable by API

'jorendorff review -v -pedantic' grammar nit: remove the comma?

::: js/src/jswrapper.cpp
@@ +850,5 @@
>  
> +    if (wrapper->is<FunctionProxyObject>())
> +        wrapper->as<FunctionProxyObject>().nuke(&DeadObjectProxy::singleton);
> +    else
> +        wrapper->as<ProxyObject>().nuke(&DeadObjectProxy::singleton);

The flavor of non-virtual method overloading going on with ProxyObject and FunctionProxyObject always feels a bit brittle to me. It's not real overloading. The if statement is still here. And you can't assert in ProxyObject::nuke() that the caller has called the right method.

So instead, accept the if statement, put it in a single ProxyObject::nuke() method, remove FunctionProxyObject::nuke(), and then the caller can't make that mistake. Less code too, I think.

"Use the mighty if statement" --brendan

::: js/src/vm/ProxyObject.h
@@ +25,5 @@
> +  public:
> +    static ProxyObject *New(JSContext *cx, BaseProxyHandler *handler, HandleValue priv,
> +                            TaggedProto proto_, JSObject *parent_, ProxyCallable callable);
> +
> +    const Value &Private() {

I think it has to be getPrivate() or private_(), unless it's a static method. cf. new_, delete_, class_.

@@ +35,5 @@
> +    HeapSlot *slotOfPrivate() {
> +        return &getReservedSlotRef(PRIVATE_SLOT);
> +    }
> +
> +    JSObject *targetObject() const {

How about calling this target()?

@@ +53,5 @@
> +    static size_t offsetOfHandler() {
> +        return getFixedSlotOffset(HANDLER_SLOT);
> +    }
> +
> +    const Value &extra(size_t n) {

const method?

::: js/src/vm/ScopeObject.cpp
@@ +1558,5 @@
>  
>  ScopeObject &
>  DebugScopeObject::scope() const
>  {
> +    return const_cast<DebugScopeObject*>(this)->targetObject()->as<ScopeObject>();

I don't think the const_cast is necessary anymore. Same thing in the other methods below.

	Nicholas Nethercote [inactive] Assignee
	Comment 16 • 11 years ago

> js_IsDebugScopeSlow already contains the check for
> ObjectProxyObject::class_. Remove the redundant check here?

It's redundant if you ignore performance, but it's possible that the hasClass check fails most of the time, and you get that without having to call a non-inlined function.

So I redid it to remove the check from js_IsDebugScopeSlow() instead:

template<>
inline bool
JSObject::is<js::DebugScopeObject>() const
{
    extern bool js_IsDebugScopeSlow(JSObject *obj);
    // Note: don't use is<ObjectProxyObject>() here -- it also matches subclasses!
    return hasClass(&js::ObjectProxyObject::class_) &&
           js_IsDebugScopeSlow(&const_cast<JSObject*>(this)->as<js::ObjectProxyObject>());
}

bool
js_IsDebugScopeSlow(ObjectProxyObject *proxy)
{   
    return GetProxyHandler(proxy) == &DebugScopeProxy::singleton;
}

	Nicholas Nethercote [inactive] Assignee
	Comment 17 • 11 years ago

> > +    if (wrapper->is<FunctionProxyObject>())
> > +        wrapper->as<FunctionProxyObject>().nuke(&DeadObjectProxy::singleton);
> > +    else
> > +        wrapper->as<ProxyObject>().nuke(&DeadObjectProxy::singleton);
> 
> The flavor of non-virtual method overloading going on with ProxyObject and
> FunctionProxyObject always feels a bit brittle to me. It's not real
> overloading. The if statement is still here. And you can't assert in
> ProxyObject::nuke() that the caller has called the right method.
> 
> So instead, accept the if statement, put it in a single ProxyObject::nuke()
> method, remove FunctionProxyObject::nuke(), and then the caller can't make
> that mistake. Less code too, I think.

That doesn't work because CALL_SLOT and CONSTRUCT_SLOT are private to FunctionProxyObject, and so can't be accessed from ProxyObject::nuke().  But something similar can be made to work:

  void
  ProxyObject::nuke(BaseProxyHandler *handler)
  {
      NukeSlot(this, PRIVATE_SLOT);
      setHandler(handler);

      NukeSlot(this, EXTRA_SLOT + 0);
      NukeSlot(this, EXTRA_SLOT + 1);
      
      if (is<FunctionProxyObject>())
          as<FunctionProxyObject>().nukeExtra();
  }   

  void
  FunctionProxyObject::nukeExtra()
  {
      NukeSlot(this, CALL_SLOT);
      NukeSlot(this, CONSTRUCT_SLOT);
  }

	Nicholas Nethercote [inactive] Assignee
	Comment 18 • 11 years ago

I landed parts 1, 2 and 4 (but labelled them as 1, 2 and 3 in the commit messages):
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b78a7180120
https://hg.mozilla.org/integration/mozilla-inbound/rev/96b8f28d35e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/08cb6548110d

Still waiting on review for the WrapperObject patch.

	Ryan VanderMeulen [:RyanVM]
	Comment 19 • 11 years ago

https://hg.mozilla.org/mozilla-central/rev/9b78a7180120
https://hg.mozilla.org/mozilla-central/rev/96b8f28d35e8
https://hg.mozilla.org/mozilla-central/rev/08cb6548110d

	Nicholas Nethercote [inactive] Assignee
	Comment 20 • 11 years ago

Part 3 (labelled as part 4 in the commit message):
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb2316d853d6

	Ed Morley [:emorley]
	Comment 21 • 11 years ago

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