1474369 - Add basic nsTArray support to xpconnect

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P2)

Description

[Nika Layzell [:nika] (ni? for response)]

This doesn't necessarily do the most ergonomic thing, so there are a few potential changes we should make.

 1. Interfaces and DOMObjects are passed as `nsTArray<nsIFoo*>&`, rather than in a `RefPtr<nsIFoo>`. Changing this only requires changing the xpidl header generation, but I am not sure if we want to guarantee that RefPtr<T> has the same in-memory representation as a raw pointer.

 2. nsTArray<T> is used as the C++-exposed type, rather that mozilla::dom::Sequence. This would also be easy to change.

Other than that I think it should be fairly good. `Sequence<Sequence<AString> >` is all supported correctly.

Comment 1

[Nika Layzell [:nika] (ni? for response)]

Attachment: Add nsTArray support to xpidl

Comment 2

[Bobby Holley (:bholley)]

How does this relate to the solution discussed in bug 1444524?

Comment 3

[Nika Layzell [:nika] (ni? for response)]

(In reply to Bobby Holley (:bholley) from comment #2)
> How does this relate to the solution discussed in bug 1444524?

I was not aware of that bug.

What this allows us to do is use the `Sequence<T>` type directly inside of XPIDL, which is reflected into C++ code as an nsTArray<T> and in JS code as a JS Array.

It's the most similar to your suggested solution in bug 1444524 comment 1.

In my opinion, ergonomics wise, this is nicer to use than the other suggestions in that bug. From C++ code, an nsTArray<T> can be used directly, and from JS code the object you interact with is a real JS array. We may be able to use it to eliminate some of our internal nsIArray-style types in the future.

Ideally this could be used to kill [array], and other array-like internal data types for passing over XPIDL, such as nsIArray.

Comment 4

[Kris Maglione [:kmag]]

+1

Ideally, I'd love to see XPIDL API signatures move gradually closer to WebIDL to make it easier to move APIs between the two, as performance or memory usage attributes dictate.  (Also, just to decrease the cognitive overhead of writing bindings for the two systems...)

(In reply to :Nika Layzell from comment #3)
> We may be able to use it to eliminate some of our internal nsIArray-style
> types in the future.
> 
> Ideally this could be used to kill [array], and other array-like internal
> data types for passing over XPIDL, such as nsIArray.

Please yes. nsIArray is awful to use from both C++ and JS, and the overhead of all the virtual calls and QIs it requires is really not great either...

Comment 5

[Andrew McCreight [:mccr8]]

Comment on attachment 8990771 [details] [diff] [review]
Add nsTArray support to xpidl

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

A 60KB patch needs a little more of a description in the commit message, please.

Comment 6

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1474369 - Part 1: Clean up value initialization codepaths in XPConnect, r=mccr8

A goal of the Sequence<T> work is to allow using more complex types within lists
in XPConnect. For example, we ideally want to support `Sequence<AString>`,
rather than requiring people to use the unergonomic 'wstring' type.

These types require initialization before they can be read into. Currently this
initialization for parameters is directly handled by XPCWrappedNative's
CallMethodHelper object.

This patch introduces a new function to the `xpc` namespace to initialize a
specific value from an uninitialized state to a safe state.

Comment 7

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1474369 - Part 1: Clean up value initialization codepaths in XPConnect, r=mccr8

A goal of the Sequence<T> work is to allow using more complex types within lists
in XPConnect. For example, we ideally want to support `Sequence<AString>`,
rather than requiring people to use the unergonomic 'wstring' type.

These types require initialization before they can be read into. Currently this
initialization for parameters is directly handled by XPCWrappedNative's
CallMethodHelper object.

This patch introduces a new function to the `xpc` namespace to initialize a
specific value from an uninitialized state to a safe state.

Comment 8

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1474369 - Part 1: Clean up value initialization codepaths in XPConnect, r=mccr8

A goal of the Sequence<T> work is to allow using more complex types within lists
in XPConnect. For example, we ideally want to support `Sequence<AString>`,
rather than requiring people to use the unergonomic 'wstring' type.

These types require initialization before they can be read into. Currently this
initialization for parameters is directly handled by XPCWrappedNative's
CallMethodHelper object.

This patch introduces a new function to the `xpc` namespace to initialize a
specific value from an uninitialized state to a safe state.

Comment 9

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1474369 - Part 2: Make JSArray2Native and NativeArray2JS more generic, so they can be used with Sequence<T>, r=mccr8

The background logic for handling lists of XPConnect values is similar between
`[array] T` and `Sequence<T>`. The major differences are with regard to how
native length is determined, how 'null' and 'undefined' are handled, and how
native buffer allocation is handled.

This patch modifies the JSArray2Native function to make it generic over an
allocation strategy function, which can be implemented for each of `[array] T`
and `Sequence<T>`. The function takes in a `uint32_t*` pointer, pointing at the
computed length of the JS array. The callback can then allocate the correct
backing buffer, and optionally modify the length to copy.

The NativeArray2JS function is also modified to make it directly take a pointer
to the native buffer rather than determining it from a pointer to an `[array] T`
parameter.


Depends On D2105

Comment 10

[Phabricator Automation]

Comment on attachment 8991742 [details]
Bug 1474369 - Part 1: Clean up value initialization codepaths in XPConnect, r=mccr8

Andrew McCreight [:mccr8] has approved the revision.

https://phabricator.services.mozilla.com/D2105

Comment 11

[Phabricator Automation]

Comment on attachment 8991743 [details]
Bug 1474369 - Part 2: Make JSArray2Native and NativeArray2JS more generic, so they can be used with Sequence<T>, r=mccr8

Andrew McCreight [:mccr8] has approved the revision.

https://phabricator.services.mozilla.com/D2106

Comment 12

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1474369 - Part 3: Add generic type parsing support to xpidl, r=mccr8

This patch allows parsing generic types, such as Sequence<T>, in XPIDL. It does
this by introducing a new type, TypeId, which contains both the name string and
an optional list of generic parameters.

Various places which use the xpidl.py library had to be updated to construct one
of these TypeId objects, as TypeId and `str` are not compatible types.


Depends On D2106

Comment 13

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1474369 - Part 3: Add generic type parsing support to xpidl, r=mccr8

This patch allows parsing generic types, such as Sequence<T>, in XPIDL. It does
this by introducing a new type, TypeId, which contains both the name string and
an optional list of generic parameters.

Various places which use the xpidl.py library had to be updated to construct one
of these TypeId objects, as TypeId and `str` are not compatible types.


Depends On D2106

Comment 14

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1474369 - Part 4: Add support for Sequence<T> types to xpidl and XPConnect, r=mccr8

This patch adds support for the `Sequence<T>` type. This is largely a
straightforward type propagation patch, but there are a few notable things:

 1. We allow `[iid_is(x)] Sequence<nsQIResult>`, so Sequence can be Dependent.

 2. `Sequence<T>` is reflected into C++ as a `nsTArray<T>`, which is different
    than WebIDL's `mozilla::dom::Sequence<T>` type. This decision was made for
    general ergonomics reasons, as `nsTArray<T>` is more prevailent throughout
    the codebase, and lengths in this case cannot be controlled by content, as
    XPConnect is only exposed to Chrome JS.

 3. Owned pointers in `Sequence<T>` are not reflected as their owned
    counterparts. For example, `Sequence<nsISupports>` is reflected as
    `nsTArray<nsISupports*>` rather than `nsTArray<RefPtr<nsISupports>>`. This
    was done to avoid depending on `RefPtr<T>` and `T*` having the same
    in-memory representation, however if that is considered an acceptable
    dependency, it would be nice to support that.

 4. We also don't reflect singly-owned pointers as their owned counterparts. For
    example, `nsTArray<nsIIDPtr>` would be reflected as `nsTArray<nsIID*>`
    rather than `nsTArray<mozilla::UniquePtr<nsIID>>`. If we are willing to
    depend on `mozilla::UniquePtr<T>`'s in-memory representation, we could also
    do this, however.

 5. There are no restrictions on what types can appear inside of a `Sequence<T>`
    or what can appear inside an `[array] T`. We may want to add restrictions
    either at the xpidl level or in XPConnect.


Depends On D2109

Comment 15

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1474369 - Part 5: Add tests for new Sequence<T> types, r=mccr8

Depends On D2110

Comment 16

[Phabricator Automation]

Comment on attachment 8991763 [details]
Bug 1474369 - Part 3: Add generic type parsing support to xpidl, r=mccr8

Andrew McCreight [:mccr8] has approved the revision.

https://phabricator.services.mozilla.com/D2109

Comment 17

[Andrew McCreight [:mccr8]]

Boris, how does the plan in comment 0 sound to you? I'm happy to review these patches, but I don't have any real high level insight into whether the basic set up is reasonable or not.

Comment 18

[Andrew McCreight [:mccr8]]

I guess the type of the elements will be nsIFoo*, but the array actually has strong references to all of its elements. This is similar to what we have for non-array nsIFoo things, but in that case the implicit coercion from a strong pointer to a raw pointer, plus the use of getter_Addrefs, means that C++ users of this API can use strong references.

Comment 19

[Andrew McCreight [:mccr8]]

It looks like RefPtr::StartAssignment already depends on RefPtr<T> and T* having the same representation. I don't think it is good to require that people use these strong nsIFoo* pointers, but maybe making that nicer could be a followup. The setup in these patches mostly just looks possibly annoying to use, rather than dangerous.

Comment 20

[Phabricator Automation]

Comment on attachment 8991764 [details]
Bug 1474369 - Part 4: Add support for Sequence<T> types to xpidl and XPConnect, r=mccr8

Andrew McCreight [:mccr8] has approved the revision.

https://phabricator.services.mozilla.com/D2110

Comment 21

[Phabricator Automation]

Comment on attachment 8991765 [details]
Bug 1474369 - Part 5: Add tests for new Sequence<T> types, r=mccr8

Andrew McCreight [:mccr8] has approved the revision.

https://phabricator.services.mozilla.com/D2111

Comment 22

[Nika Layzell [:nika] (ni? for response)]

(In reply to Andrew McCreight [:mccr8] from comment #19)
> It looks like RefPtr::StartAssignment already depends on RefPtr<T> and T*
> having the same representation. I don't think it is good to require that
> people use these strong nsIFoo* pointers, but maybe making that nicer could
> be a followup. The setup in these patches mostly just looks possibly
> annoying to use, rather than dangerous.

If we're comfortable with depending on this for StartAssignment, I am 100% willing to quickly switch over to using RefPtr<T> for this situation :-). The other problem is the UniquePtr<T> situation for `string`, `wstring`, and `ns{I,C,}IDPtr`. The first two should probably not be used in Sequence<T> types, so I'm OK with just preventing their use. The `ns{I,C,}IDPtr` ones are more likely to be used, but we could also work around that by adding support for owned `nsID` types in arrays, which would probably be more efficient anyway...

Comment 23

[Nathan Froyd [:froydnj]]

(In reply to Andrew McCreight [:mccr8] from comment #19)
> It looks like RefPtr::StartAssignment already depends on RefPtr<T> and T*
> having the same representation. I don't think it is good to require that
> people use these strong nsIFoo* pointers, but maybe making that nicer could
> be a followup. The setup in these patches mostly just looks possibly
> annoying to use, rather than dangerous.

I am reasonably confident that Stylo's bindings depend on sizeof(RefPtr<T>) == sizeof(T*), and we might even have a static_assert for that in the tree somewhere.

Comment 24

[Andrew McCreight [:mccr8]]

I was just going to say, we should ask Nathan if that's okay to depend on, but it seems reasonable to me given that it is already baked into RefPtr<>.

Yeah, I'd just disallow any tricky cases, and people can complain and file bugs if they want to use them (you could even tell people to file a bugs in the XPIDL error message).

Comment 25

[Boris Zbarsky [:bzbarsky]]

I'm fine with nsTArray in place of Sequence.  We have Sequence because it actually has different fallibility behavior from nsTArray and for a while we weren't quite sure what the right sequence representation was, so were trying to not hardcode a specific Gecko type.

I would _much_ prefer we have nsTArray<RefPtr<>> to nsTArray<T*>.  The latter is a huge footgun when going from C++ to JS (calling into JS components or returning thing from C++), imo.  Calling into JS components we have that problem for pointer arguments too, but at least for return values we don't...

Comment 26

[Kris Maglione [:kmag]]

(In reply to Boris Zbarsky [:bz] (no decent commit message means r-) from comment #25)
> I would _much_ prefer we have nsTArray<RefPtr<>> to nsTArray<T*>.  The
> latter is a huge footgun when going from C++ to JS (calling into JS
> components or returning thing from C++), imo.  Calling into JS components we
> have that problem for pointer arguments too, but at least for return values
> we don't...

+1

It's hard enough to keep track of the ownership of raw pointers at the best of times. For out params, anything other than nsTArray<RefPtr<T>> or nsTArray<already_AddRefed<T>> is going to wind up causing leaks or early frees, and the RefPtr version seems vastly preferable.

For in params, it would be nice to be consistent, so I'd tend to favor `const nsTArray<RefPtr<T>>&`, but `const nsTArray<T&>&` or `const nsTArray<OwningNonNull<T>>&` for WebIDL consistency would probably be fine too.

But if we're expecting most of our calls to come from XPC JS, it might make more sense to just do `nsTArray<RefPtr<T>>&&` so the callee can take ownership and avoid the extra ref counting overhead if necessary.

Comment 27

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1474369 - Part 6: Use RefPtr for Array<T> of interface and WebIDL types, r=mccr8

This means that using these types involves many fewer footguns, while not
requiring any changes to the actual XPConnect implementation!


Depends on D2111

Comment 28

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1474369 - Part 7: Rename [array] to LegacyArray within xpt and xpidl, r=mccr8

This is done so we can use Array as the name for the new nsTArray-based
type, rather than having to come up with a new name.

LegacyArray was chosen as the [array] attribute is now effectively deprecated,
and we'd like to remove it ASAP.


Depends On D2334

Comment 29

[Nika Layzell [:nika] (ni? for response)]

Attachment: Bug 1474369 - Part 8: Rename from Sequence to Array in xpidl, r=mccr8

This more closely matches the C++ names, and reflects the fact that the
reflected type is not WebIDL's mozilla::dom::Sequence. The reasoning behind this
type difference is for ergonomics, due to xpidl only being exposed to internal
JS code.


Depends On D2335

Comment 30

[Phabricator Automation]

Comment on attachment 8994646 [details]
Bug 1474369 - Part 6: Use RefPtr for Array<T> of interface and WebIDL types, r=mccr8

Andrew McCreight [:mccr8] has approved the revision.

https://phabricator.services.mozilla.com/D2334

Comment 31

[Phabricator Automation]

Comment on attachment 8994647 [details]
Bug 1474369 - Part 7: Rename [array] to LegacyArray within xpt and xpidl, r=mccr8

Andrew McCreight [:mccr8] has approved the revision.

https://phabricator.services.mozilla.com/D2335

Comment 32

[Phabricator Automation]

Comment on attachment 8994650 [details]
Bug 1474369 - Part 8: Rename from Sequence to Array in xpidl, r=mccr8

Andrew McCreight [:mccr8] has approved the revision.

https://phabricator.services.mozilla.com/D2337

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/459635cdfc08
Part 1: Clean up value initialization codepaths in XPConnect, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1b6a5f74642
Part 2: Make JSArray2Native and NativeArray2JS more generic, so they can be used with Sequence<T>, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/f355d9be9912
Part 3: Add generic type parsing support to xpidl, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/db67bf0e7f91
Part 4: Add support for Sequence<T> types to xpidl and XPConnect, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/320802ab02e3
Part 5: Add tests for new Sequence<T> types, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/9d0b9a4c4651
Part 6: Use RefPtr for Array<T> of interface and WebIDL types, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/7e2e13953e19
Part 7: Rename [array] to LegacyArray within xpt and xpidl, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/79dbf5b9d8db
Part 8: Rename from Sequence to Array in xpidl, r=mccr8

Comment 34

[Cristian Brindusan [:cbrindusan]]

Backed out 15 changesets (bug 1475409, bug 1461450, bug 1474369, bug 1471726) for build bustages on xptcstubs_gcc_x86_unix.cpp:55:1.

Backout: https://hg.mozilla.org/integration/mozilla-inbound/rev/113e9ca0b5bccaf3eaee398e789f9b7b8c226009

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&revision=79dbf5b9d8db577bba582a0853eb293d80eed0ba&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&selectedJob=190109107

Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=190109107&repo=mozilla-inbound&lineNumber=12152

Comment 35

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/99c4d07d4b88
Part 1: Clean up value initialization codepaths in XPConnect, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e9f6d2544a82
Part 2: Make JSArray2Native and NativeArray2JS more generic, so they can be used with Sequence<T>, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/ccea3049fe0f
Part 3: Add generic type parsing support to xpidl, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/cbdde0474521
Part 4: Add support for Sequence<T> types to xpidl and XPConnect, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/b26c90518fca
Part 5: Add tests for new Sequence<T> types, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/13c9626970e2
Part 6: Use RefPtr for Array<T> of interface and WebIDL types, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/a8a4e2414daa
Part 7: Rename [array] to LegacyArray within xpt and xpidl, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/7ce27aa3ce68
Part 8: Rename from Sequence to Array in xpidl, r=mccr8

Comment 36

[Cosmin Sabou [:CosminS]]

Backed out for causing rooting hazards and browser chrome failures

Push with failures: https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&filter-searchStr=Linux%20x64%20pgo%20Mochitests%20with%20e10s%20test-linux64-pgo%2Fopt-mochitest-browser-chrome-e10s-1%20M-e10s(bc1)&fromchange=7ce27aa3ce6887c226baa88223c0bf95bc2c3c28&tochange=e4f654755cc5cb80fb0a5a91707e8a2eff425e3e&selectedJob=190915193

https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&fromchange=7ce27aa3ce6887c226baa88223c0bf95bc2c3c28&tochange=e4f654755cc5cb80fb0a5a91707e8a2eff425e3e&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=success&filter-classifiedState=unclassified&selectedJob=190903190&filter-searchStr=Linux%20x64%20debug%20hazard-linux64-haz%2Fdebug%20(H)

Failure log: 
browser chrome failures: https://treeherder.mozilla.org/logviewer.html#?job_id=190915193&repo=mozilla-inbound&lineNumber=3614
rooting hazards: https://treeherder.mozilla.org/logviewer.html#?job_id=190903190&repo=mozilla-inbound&lineNumber=47845

Backout link: https://hg.mozilla.org/integration/mozilla-inbound/rev/e4f654755cc5cb80fb0a5a91707e8a2eff425e3e

Comment 37

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4b7ea46a37fa
Part 1: Clean up value initialization codepaths in XPConnect, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/aaadb94f144d
Part 2: Make JSArray2Native and NativeArray2JS more generic, so they can be used with Sequence<T>, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/ff1530e9970a
Part 3: Add generic type parsing support to xpidl, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/4c78a885c77d
Part 4: Add support for Sequence<T> types to xpidl and XPConnect, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e28ef672bfd5
Part 5: Add tests for new Sequence<T> types, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/306a0fa61ea1
Part 6: Use RefPtr for Array<T> of interface and WebIDL types, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/3472197282ce
Part 7: Rename [array] to LegacyArray within xpt and xpidl, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/ce18b9d28b64
Part 8: Rename from Sequence to Array in xpidl, r=mccr8

Comment 38

[Dorel Luca [:dluca]]

https://hg.mozilla.org/mozilla-central/rev/4b7ea46a37fa
https://hg.mozilla.org/mozilla-central/rev/aaadb94f144d
https://hg.mozilla.org/mozilla-central/rev/ff1530e9970a
https://hg.mozilla.org/mozilla-central/rev/4c78a885c77d
https://hg.mozilla.org/mozilla-central/rev/e28ef672bfd5
https://hg.mozilla.org/mozilla-central/rev/306a0fa61ea1
https://hg.mozilla.org/mozilla-central/rev/3472197282ce
https://hg.mozilla.org/mozilla-central/rev/ce18b9d28b64