1289137 - Make XPCNativeSet::NewInstance() take an nsTArray argument

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Andrew McCreight [:mccr8]]

Right now, it takes an XPCNativeInterface** argument which requires manual cleanup in the caller, and is ugly to iterate over.

Comment 1

[Andrew McCreight [:mccr8]]

This code is mostly untouched since the initial XPCDOM landing in 2001.

Comment 2

[Andrew McCreight [:mccr8]]

ASan try run: https://treeherder.mozilla.org/#/jobs?repo=try&revision=9008017e28f2

Comment 3

[Andrew McCreight [:mccr8]]

Attachment: Make XPCNativeSet::NewInstance() take an nsTArray argument.

I could clean up ArrayAutoMarkingPtr more, but it is going to be removed entirely in bug 1288870.

Comment 4

[Andrew McCreight [:mccr8]]

Bobby, are you okay with billm reviewing this? I think the only tricky non-generic part is understanding ArrayAutoMarkingPtr and Bill is probably most familiar with it.

Comment 5

[Andrew McCreight [:mccr8]]

I suppose this request is redundant with my one in bug 1288817, so I'll just clear it and ask Bill for review. Feel free to register an objection. :)

Comment 6

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

Comment on attachment 8774380 [details] [diff] [review]
Make XPCNativeSet::NewInstance() take an nsTArray argument.

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

Setting r?

Comment 7

[Andrew McCreight [:mccr8]]

Oops, thanks.

Comment 8

[Andrew McCreight [:mccr8]]

Attachment: Make XPCNativeSet::NewInstance() take an nsTArray argument.

Minor rebasing.

Comment 9

[Andrew McCreight [:mccr8]]

Attachment: Bug 1289137 - Make XPCNativeSet::NewInstance() take an nsTArray argument.

I could clean up ArrayAutoMarkingPtr more, but it is going to be
removed entirely in bug 1288870.

Review commit: https://reviewboard.mozilla.org/r/69788/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/69788/

Comment 10

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

Comment on attachment 8778485 [details]
Bug 1289137 - Make XPCNativeSet::NewInstance() take an nsTArray argument.

https://reviewboard.mozilla.org/r/69788/#review67442

::: js/xpconnect/src/XPCWrappedNativeInfo.cpp:490
(Diff revision 1)
> -    // hacky way to get a XPCNativeInterface** using the AutoPtr
> -    XPCNativeInterface* temp[] = {iface};
> -    set = NewInstance(temp, 1);
> +    AutoTArray<XPCNativeInterface*, 1> temp;
> +    temp.AppendElement(iface);
> +    set = NewInstance(temp);

I would recommed passing the nsTArray to NewInstance as an nsTArray&&. Then you could call:
  set = NewInstance(AutoTArray<XPCNI*, 1>(iface));
I think this should work and be efficient.

::: js/xpconnect/src/XPCWrappedNativeInfo.cpp:567
(Diff revision 1)
> -            *(currentInterface++) = iface;
> +            interfaceArray.AppendElement(iface);
> -            interfaceCount++;
>          }
>  
> -        if (interfaceCount) {
> -            set = NewInstance(interfaceArray, interfaceCount);
> +        if (interfaceArray.Length() > 0) {
> +            set = NewInstance(interfaceArray);

You would need to use Move here if you take my suggestion.

::: js/xpconnect/src/XPCWrappedNativeInfo.cpp:641
(Diff revision 1)
> -        set = NewInstance(&newInterface, 1);
> +        AutoTArray<XPCNativeInterface*, 1> temp;
> +        temp.AppendElement(newInterface);
> +        set = NewInstance(temp);

Can use the same pattern here as above.

Comment 11

[Andrew McCreight [:mccr8]]

(In reply to Bill McCloskey (:billm) from comment #10)
> I would recommed passing the nsTArray to NewInstance as an nsTArray&&.

That's pretty nice. Unfortunately, that doesn't work, because you can't pass in a single argument to the ctor. I'd noticed that before, which is why there was the AppendElement() on a second line. It looks like AutoTArray is just missing a ctor with initializer_list like the other array types, so I'll file a bug for that and then make the changes you suggest, assuming there are no issues there.

Comment 12

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

Oops. I read over that code and thought I saw it. The initializer_list thing sounds good.

Comment 13

[Andrew McCreight [:mccr8]]

Thinking about this some more, I might as well just use nsTArray in the caller: moving from an AutoTArray to an nsTArray is surely going to cause a heap allocation, so I might as well not bother with the auto array. This is technically less efficient, but I'm sure it doesn't matter.

And I can just do this, which is slick:
  set = NewInstance({newInterface});

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 8778485 [details]
Bug 1289137 - Make XPCNativeSet::NewInstance() take an nsTArray argument.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/69788/diff/1-2/

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a7a5fd1b36e5
Make XPCNativeSet::NewInstance() take an nsTArray argument. r=billm

Comment 16

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

(In reply to Andrew McCreight [:mccr8] from comment #13)
> Thinking about this some more, I might as well just use nsTArray in the
> caller: moving from an AutoTArray to an nsTArray is surely going to cause a
> heap allocation, so I might as well not bother with the auto array. This is
> technically less efficient, but I'm sure it doesn't matter.
> 
> And I can just do this, which is slick:
>   set = NewInstance({newInterface});

We're not actually moving (as in invoking a move constructor), we're just passing a reference. So I think it would have stayed an AutoTArray the whole time. But yeah, it doesn't really matter.

Comment 17

[Carsten Book [:Tomcat]]

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