Add basic nsTArray support to xpconnect

RESOLVED FIXED in Firefox 63

Status

()

enhancement
P2
normal
RESOLVED FIXED
Last year
8 months ago

People

(Reporter: Nika, Assigned: Nika)

Tracking

(Blocks 3 bugs)

unspecified
mozilla63
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox63 fixed)

Details

Attachments

(8 attachments, 4 obsolete attachments)

46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review
46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review
46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review
46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review
46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review
46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review
46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review
46 bytes, text/x-phabricator-request
mccr8
: review+
Details | Review
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.
Posted patch Add nsTArray support to xpidl (obsolete) — Splinter Review
Attachment #8990771 - Flags: review?(continuation)
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.
Attachment #8990771 - Flags: review?(continuation) → review-
Priority: -- → P2
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
Attachment #8991738 - Attachment is obsolete: true
Attachment #8991491 - Attachment is obsolete: true
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
Attachment #8991742 - Flags: review+
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
Attachment #8991743 - Flags: review+
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
Attachment #8991762 - Attachment is obsolete: true
Attachment #8990771 - Attachment is obsolete: true
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
Attachment #8991763 - Flags: review+
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.
Flags: needinfo?(bzbarsky)
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
Attachment #8991764 - Flags: review+
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
Attachment #8991765 - Flags: review+
(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...
Flags: needinfo?(bzbarsky)
(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.
Blocks: 1478119
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
Attachment #8994646 - Flags: review+
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
Attachment #8994647 - Flags: review+
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
Attachment #8994650 - Flags: review+
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
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
Depends on: 1479586
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
Flags: needinfo?(nika)
Blocks: 1509978
You need to log in before you can comment on or make changes to this bug.