Closed Bug 865410 Opened 7 years ago Closed 7 years ago

Nicer rooting in XPComponents

Categories

(Core :: XPConnect, defect)

x86_64
Linux
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla23

People

(Reporter: evilpie, Assigned: evilpie)

References

Details

Attachments

(2 files, 1 obsolete file)

See bug 856477 comment 13 and onwards.
Assignee: general → nobody
Component: JavaScript Engine → XPConnect
Assignee: nobody → evilpies
Attached patch Use CallArgs for XPConnect hooks (obsolete) — Splinter Review
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.
Attachment #743923 - Attachment is obsolete: true
Attachment #744094 - Flags: review?(bobbyholley+bmo)
Attachment #744094 - Flags: review?(bobbyholley+bmo)
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.
Attachment #744094 - Flags: review?(bobbyholley+bmo)
This changes the signature of nsIJSNativeInitializer::Initialize to take CallArgs.
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
Attachment #744231 - Flags: review?(bzbarsky)
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 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.
Attachment #744231 - Flags: review?(bzbarsky) → review+
Oh, and do put back the assert.
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?
Attachment #744094 - Flags: review?(bobbyholley+bmo) → review+
(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.
(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?
Because the function take Handle<JSObject> and handleAt returns a Handle<Value>.
(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?
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).
(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.
> 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.
(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.
> 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....
(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.
https://hg.mozilla.org/mozilla-central/rev/5d848dcb726c
https://hg.mozilla.org/mozilla-central/rev/372473b23d56
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
You need to log in before you can comment on or make changes to this bug.