865410 - Nicer rooting in XPComponents

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Tom S [:evilpie]]

See bug 856477 comment 13 and onwards.

Comment 1

[Tom S [:evilpie]]

Attachment: Use CallArgs for XPConnect hooks

This patch changes the argc, argv, vp to just CallArgs. It might be easily possible to do this with |obj| as well? This makes the function signature a lot nicer already, hopefully we can use CallArgs for every native soon.

Comment 2

[Tom S [:evilpie]]

Attachment: v1 with const ref CallArgs for Construct and Call

Comment 3

[Tom S [:evilpie]]

Comment on attachment 744094 [details] [diff] [review]
v1 with const ref CallArgs for Construct and Call

As discussed on IRC, we are not going to worry about the obj parameter at the moment.

Comment 4

[Tom S [:evilpie]]

Attachment: nsIJSNativeInitializer::Initialize

This changes the signature of nsIJSNativeInitializer::Initialize to take CallArgs.

Comment 5

[Tom S [:evilpie]]

Comment on attachment 744231 [details] [diff] [review]
nsIJSNativeInitializer::Initialize

Try push looking good, requesting review for this as well. https://tbpl.mozilla.org/?tree=Try&rev=0c64a86e5971

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Comment on attachment 744231 [details] [diff] [review]
nsIJSNativeInitializer::Initialize

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

::: dom/base/nsDOMClassInfo.cpp
@@ -3392,5 @@
>  BaseStubConstructor(nsIWeakReference* aWeakOwner,
>                      const nsGlobalNameStruct *name_struct, JSContext *cx,
> -                    JSObject *obj, unsigned argc, jsval *argv, jsval *rval)
> -{
> -  MOZ_ASSERT(obj);

Why?

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 744231 [details] [diff] [review]
nsIJSNativeInitializer::Initialize

>+          argv[i] = argv[i - 1];

Shouldn't the RHS there be args[i - 1]?

r=me with that fixed, I think.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Oh, and do put back the assert.

Comment 9

[Bobby Holley (:bholley)]

Comment on attachment 744094 [details] [diff] [review]
v1 with const ref CallArgs for Construct and Call

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

r=bholley with comments addressed.

::: js/xpconnect/idl/nsIXPCScriptable.idl
@@ +117,1 @@
>                      in JSFreeOpPtr fop, in JSObjectPtr obj);

You need to rev the IID on this interface.

::: js/xpconnect/src/XPCComponents.cpp
@@ +1884,1 @@
>                  return parseOptionsObject(obj);

Can't we use handleAt() to pass a handle directly to parseOptionsObject?

Comment 10

[Tom S [:evilpie]]

(In reply to Bobby Holley (:bholley) from comment #9)
> Comment on attachment 744094 [details] [diff] [review]
> v1 with const ref CallArgs for Construct and Call
> 
> Review of attachment 744094 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=bholley with comments addressed.
> 
> ::: js/xpconnect/idl/nsIXPCScriptable.idl
> @@ +117,1 @@
> >                      in JSFreeOpPtr fop, in JSObjectPtr obj);
> 
> You need to rev the IID on this interface.
> 
> ::: js/xpconnect/src/XPCComponents.cpp
> @@ +1884,1 @@
> >                  return parseOptionsObject(obj);
> 
> Can't we use handleAt() to pass a handle directly to parseOptionsObject?
We need to root the object somewhere, this seems to be a good place.

Comment 11

[Bobby Holley (:bholley)]

(In reply to Tom Schuster [:evilpie] from comment #10)
> > Can't we use handleAt() to pass a handle directly to parseOptionsObject?
> We need to root the object somewhere, this seems to be a good place.

Why? The scope of the rooted object is just the call to parseOptionsObject. Given that that function takes a handle, Why can't we just use handleAt?

Comment 12

[Tom S [:evilpie]]

Because the function take Handle<JSObject> and handleAt returns a Handle<Value>.

Comment 13

[Bobby Holley (:bholley)]

(In reply to Tom Schuster [:evilpie] from comment #12)
> Because the function take Handle<JSObject> and handleAt returns a
> Handle<Value>.

Oh, that's dumb. Can we add a handleObjectAt API?

Comment 14

[Boris Zbarsky [:bzbarsky]]

How would it work exactly?  Our value representation on 64-bit doesn't easily allow such a beastie (though on 32-bit we could do it).

Comment 15

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #14)
> How would it work exactly?  Our value representation on 64-bit doesn't
> easily allow such a beastie (though on 32-bit we could do it).

The stack is rooted, right? Can't we just do something along the lines of fromMarkedLocation? These things are handles, so they should theoretically be able to Just Deal with the backing store being either a JSObject* or a JSValue.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> The stack is rooted, right?

The C stack, or the JS stack?

> These things are handles, so they should theoretically be able to Just Deal with the
> backing store being either a JSObject* or a JSValue.

Yes, but in practice a Handle<JSObject*> can't deal with the backing store being a JSValue right now.

Comment 17

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #16)
> > The stack is rooted, right?
> 
> The C stack, or the JS stack?

The former.

> > These things are handles, so they should theoretically be able to Just Deal with the
> > backing store being either a JSObject* or a JSValue.
> 
> Yes, but in practice a Handle<JSObject*> can't deal with the backing store
> being a JSValue right now.

Sure. I'm just wondering if it's a feasible API improvement.

Comment 18

[Boris Zbarsky [:bzbarsky]]

> The former.

Then no.  Or more precisely, it's rooted right this second, but it will stop being rooted when we switch to exact GC, which is why we're going through rooting all this stuff.

> I'm just wondering if it's a feasible API improvement.

Well, so.

To make it work the |operator JSObject*()| on Handle<JSObject*> would need some sort of polymorphism (either a boolean flag check or a virtual call or whatever) to decide whether what it has internally is a JSObject** or a Value* backing store.  Then in the latter case it would need to unbox when you use the handle as a JSObject*.

That's feasible, presumably (and if a flag bit is used on the pointer the handle has it could even be done without the handle getting bigger, at the cost of an extra bitop on use), but would somewhat slow down all consumers of |operator JSObject*()|.  It's not clear whether that's the right tradeoff, though I agree it would be very nice in terms of developer ergonomics if we could do things that way....

Comment 19

[Bobby Holley (:bholley)]

(In reply to Boris Zbarsky (:bz) from comment #18)
> > The former.
> 
> Then no.  Or more precisely, it's rooted right this second, but it will stop
> being rooted when we switch to exact GC, which is why we're going through
> rooting all this stuff.

Doh! I meant "latter", sorry.

> > I'm just wondering if it's a feasible API improvement.
> 
> Well, so.
> 
> To make it work the |operator JSObject*()| on Handle<JSObject*> would need
> some sort of polymorphism (either a boolean flag check or a virtual call or
> whatever) to decide whether what it has internally is a JSObject** or a
> Value* backing store.  Then in the latter case it would need to unbox when
> you use the handle as a JSObject*.
> 
> That's feasible, presumably (and if a flag bit is used on the pointer the
> handle has it could even be done without the handle getting bigger, at the
> cost of an extra bitop on use), but would somewhat slow down all consumers
> of |operator JSObject*()|.  It's not clear whether that's the right
> tradeoff, though I agree it would be very nice in terms of developer
> ergonomics if we could do things that way....

Yeah, that's fair. I think it's reasonable not to do this unless it becomes a significant pain point, or the overhead of the pseudo-superfluous roots ends up being significant.

Comment 20

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/5d848dcb726c
https://hg.mozilla.org/mozilla-central/rev/372473b23d56