958641 - de-holder nsXPConnect::WrapNative

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

This will be more involved than nsXPConnect::WrapNativeToJSVal, because nsIXPConnectJSObjectHolder is being used as a return value, and there's no equivalent rooted JS object being passed in, so that conversion will have to be done at the same time.  Bug 958629 might be useful, but I suppose it isn't essential.

We might as well de-holder NativeInterface2JSObject in nsXPConnect.cpp at the same time, as it will be trivial at that point.

Comment 1

[Andrew McCreight [:mccr8]]

Attachment: part 1 - Remove some unused holder variables.

A few unused local vars that slipped through the cracks.  Not related to WrapNative,
but I didn't feel like filing a separate bug for it.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: part 2 - de-holder nsXPConnect::WrapNative

This is the actual patch.  I'm not sure how to break it down further.  I created
a new version, WrapNativeHolder, that behaves like the current function.  Most places
can be converted over easily, but there are a few places that use holders on the heap
that are not too easy to convert.  Specifically, there are three storage-related things
that seem to be objects usable on multiple threads that use the holders to keep the
JS objects alive.  Because they are threadsafe, they can't just be made into CCed objects.

I tried using nsXPConnect::HoldObject to keep the objects alive, but it turns out
they expect the holder to actually be an XPCWN, and it tries to QI to that later.
Maybe I could change the return type, and fail if it fails to produce a real XPCWN
holder, but maybe that's okay under some circumstances.

Anyways, even without that, I think this patch still cleans up a lot of cruft.  I
still need to run it through try.

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: de-Holder WrapNative.

Comment 4

[Andrew McCreight [:mccr8]]

Attachment: De-Holder nsIXPConnect::WrapNative.

No hurry on the review here, I just wanted to make sure I don't forget about this.

try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2952db5b7e8

Comment 5

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8624954 [details] [diff] [review]
De-Holder nsIXPConnect::WrapNative.

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

Sorry for this taking so long, but the workweek was really busy for me (and you stated that it's not super urgent :) ). The rest of the patch looks great, let me know what you think about this one:

::: js/xpconnect/idl/nsIXPConnect.idl
@@ +332,1 @@
>      wrapNative(in JSContextPtr aJSContext,

The only part I didn't get is why not returning a HandleObject here instead of void (and the weird |in JSMutableHandleObject| which should be an |out| actually but I guess you chose |in| because with |out| you would get a pointer to a mutable handle...). For the HandleObject return value you have to use [notxpcom] but I guess that's fine since this is noscript already.

Comment 6

[Andrew McCreight [:mccr8]]

> Sorry for this taking so long
Totally fine. I wasn't even really expecting a review today. ;)

You can't really return a Handle<> (because the function doesn't know who is holding it) but talking to people in #jsapi it sounds like the more usual thing would be to return a raw pointer, so I'll try change it to do that.

Comment 7

[Andrew McCreight [:mccr8]]

Attachment: De-holder nsIXPConnect::WrapNative.

Static analysis try build looks okay.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6b11ac68ad31

Comment 8

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8628002 [details] [diff] [review]
De-holder nsIXPConnect::WrapNative.

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

This makes sense. And does clean up things a lot indeed :)

::: js/xpconnect/idl/nsIXPConnect.idl
@@ +262,5 @@
>  // For use with the service manager
>  // {CB6593E0-F9B2-11d2-BDD6-000064657374}
>  #define NS_XPCONNECT_CID \
> +{ 0x8bc1230e, 0x036b, 0x4749, \
> +  { 0x87, 0x63, 0x76, 0x8e, 0xd6, 0x5d, 0xf1, 0x43 } }

Same here, CID should stay the same.

Comment 9

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/44dd8a5ed939

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/44dd8a5ed939