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.

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

(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.

+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 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.

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.

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 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 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

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

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

Depends On D2110

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

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.

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.

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 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 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

(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...

(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.

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).

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...

(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.

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

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

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 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 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 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

Pushed by nika@thelayzells.com: 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

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

Pushed by nika@thelayzells.com: 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

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

Pushed by nika@thelayzells.com: 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

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