1457972 - Unify xpconnect cleanup code & fix some other special cases and discrepancies.

Status: RESOLVED FIXED

(Core :: XPConnect, enhancement, P2)

Description

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

These patches do a few things which help clean up and unify code paths in xpconnect:

1. Merge the various cleanup code paths & ensure that they all follow the same cleanup logic, so that only one place needs to be modified when adding new types.

2. Simplify XPCConvert to have a smaller number of entry points, which handle more different types. Namely, the special entry points for arrays and sized strings were removed.

3. Unify code paths in XPCConvert to avoid code duplication where it was unnecessary.

4. Eliminate "dipper" types and avoid allocating nsString/nsCString objects on the heap when calling xpconnect methods.

5. Remove a user of a legacy internal JS rooting API in favour of more modern rooting for temporary JSValue parameters. (NOTE: There now remains only 1 consumer of this legacy API in ErrorResult, if that consumer is eliminated we can get away with removing the API alltogether!)

I started doing these changes in the work up to supporting full Sequence<T> types in XPConnect and XPIDL, however I have put work on that temporarily on hold to focus on other problems. I'll probably come back to it later.

Comment 1

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

Attachment: Part 1: Unify xpconnect cleanup codepaths

It used to be that in XPConnect there were many different pieces of code for
each place where we may need to clean up some untyped values based on their
nsXPTType information. This was a mess, and meant that every time you needed to
add a new data type you'd have to find every one of these places and add support
for your new type to them.

In fact, this was bad enough that it appears that I missed some places when
adding my webidl support! Which means that in some edge cases we may clean up
one of these values incorrectly D:!

This patch adds a new unified method which performs the cleanup by looking at a
nsXPTType object. The idea is that this function takes a void* which is actually
a T* where T is a value of the nsXPTType parmaeter. It clears the value behind
the pointer to a valid state such that free-ing the memory would not cause any
leaks. e.g. it free(...)s owned pointers and sets the pointer to `nullptr`, and
truncates nsA[C]String values such that they reference the static empty string.

I also modify every one of these custom cleanup codepaths to instead call into
this unified cleanup method.

This also involved some simplification of helper methods in order to make the
implementation cleaner.

Comment 2

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

Attachment: Part 2: Remove unused code paths in xpconnect

Thanks to the changes in the previous patch, we had some unused code which we
can get rid of. This patch just cleans stuff up a bit.

Comment 3

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

Attachment: Part 3: Remove unnecessary #includes of xptinfo headers

We are going to want to include some "gecko internal" types in more places in
the codebase, and we have unused includes of some of these headers in non-libxul
files.

This patch just cleans up these unnecssary includes.

Comment 4

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

Attachment: Part 4: Remove dipper types

PT accrued some weird types and flags over the years, and one of the worst of
these is the "dipper" type flag. This flag was added for ns[C]String values, as
they needed to be passed indirectly as in and out.

There was another tool which was added for the same purpose, which was the
"Indirect" behaviour. This flag is set for outparameters by default, and
designates that a value will be passed indirectly, but is also used by jsvals
unconditionally, as jsvals are always passed behind a pointer.

The effective way that indirect parameters works is as follows:

1. When calling from C++ into JS code, the parameter data pointer is
   dereferenced an extra time before being passed to conversion methods.

2. When calling from JS into C++ code, a flag is set on the nsXPTCVariant
   object. This flag is read by the platform-specific call code to cause them to
   pass the pointer value stored in nsXPTCVariant::ptr as the parameter (which
   points to the nsXPTMiniVariant member) rather than the value stored in the
   variant, thus causing the value to be passed indirectly.

For reference dipper parmaeters worked in a different manner:

1. When calling from C++ into JS code, an extra level of indirection is added to
   the passed-in pointer before passing it to conversion methods, causing the
   pointer passed in to have a "real" type of `nsA[C]String**`

2. When calling from JS into C++ code, a nsA[C]String object is allocated using
   a custom allocator (which tries to avoid allocating for the first 2 strings
   of each type, and after that heap allocates), and the allocation's pointer is
   stored in the variant. The value is not considered as being passed
   "indirectly" for both in and out parameters.

As you can see, these two mechanisms take similar but slightly different
approaches. The most notable difference is that in the Indirect case, the "real"
value is assumed to be stored directly in the nsXPTCVariant object in the JS ->
C++ case. This was probably not done in the past for ns[C]String as the
nsXPTCVariant object did not have enough space to allocate a ns[C]String object,
as it could only hold 8 bytes of information.

Fortunately for us, we actually have _two_ variants of nsXPTCVariant, the
nsXPTCMiniVariant is what is used most of the time, such as when calling from
C++ into JS, while the nsXPTCVariant is what is used when we need to actually
allocate space to store whatever value we're passing ourselves (namely it is
only used in the JS -> C++ case).

nsXPTCVariant is (almost) always allocated on the stack (It is allocated in a
stack-allocated AutoTArray with a inline capacity of 8. For reference, the
largest parameter count of a JS-exposed xpt method right now is 14 - I
considered bumping the inline capacity up to 16 to make it so we never need to
heap allocate parmaeters, but it seemed like it should be done in a seperate
bug).

This object is also already pretty big. It has in it:
 1. a nsXPTCMiniVariant (8 bytes)
 2. a nsXPTType (3 bytes)
 3. a void* for indirect calls (8/4 bytes)
 4. a flag byte (1 byte)

We only need to add enough space to store a ns[C]String in the nsXPTCVariant,
and not in nsXPTCMiniVariant. My approach to this problem was to make
nsXPTCVariant actually hold a union of a nsXPTCMiniVariant, and some storage
space for the other, potentially larger information, which we never need to
store in a MiniVariant.

This allows us to stack allocate the ns[C]Strings created by XPConnect and avoid
the use of dipper types entirely, in favour of just using indirect values. It
also allows us to delete some of the now-unnecessary custom allocator code for
ns[C]String objects.

Comment 5

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

Attachment: Part 5: Use modern JS APIs to root jsval temporaries in XPConnect

When a jsval passed from JS code it needs to be stored in a nsXPTCVariant
object. This object is not rooted by default, as it is stored in some
C++-allocated memory. Currently, we root the values by adding a custom root
using the `js::AddRawValueRoot` API, which is deprecated, and only used by this
code and ErrorResult.

This also has the unfortunate effect that we cannot support XPCOM arrays of
jsvals, as we cannot root all of the values in the array using this API.

Fortunately, the JS engine has a better rooting API which we can use here
instead. I make the call context a custom rooter, like the SequenceRooter type
from WebIDL, and make sure to note every jsval when tracing, both in arrays and
as direct values.

This should allow us to avoid some hashtable operations with roots when
performing XPConnect calls, and remove a consumer of this gross legacy API.

In addition it allows us to support arrays. This will be even more useful in the
future when I add support for sequence<T> (which is a nsTArray<T>) to xpidl and
xpconnect.

Comment 6

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

Attachment: Part 6: Ensure the extended types list has some basic types with known indexes

Currently XPCVariant has some code for working with arrays of a series of basic
types. I want to unify and simplify code which works with nsXPTTypes to always
take the topmost level type (rather than passing in an array element type when
working with an array).

This is pretty easy for most of XPConnect, but XPCVariant occasionally needs to
perform calls on made-up array types, which isn't compatible with the current
implementation. Fortunately, it only needs a very small set of array types. This
patch adds a set of simple types (mostly the arithmetic types and
TD_INTERFACE_IS_TYPE for interfaces) to the extra types array unconditionally
with a known index, for XPCVariant.

An other option I was considering was to consider the value `0xff` in the data
byte on nsXPTType to be a flag which indicates that the array element type is
actually the type immediately following the current nsXPTType object in memory,
but that was incompatible with many of the existing nsXPTType consumers which
copy the nsXPTType objects around (e.g. onto the stack), rather than always
using them by reference, so I decided it was not a good approach to take.

Comment 7

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

Attachment: Part 7: Eliminate XPCConvert::NativeStringWithSize2JS/JSStringWithSize2Native

PIDL supports explicitly sized string types. These types currently have to be
handled by a separate entry point into XPCConvert, and don't share any logic
with the implicitly sized string types.

If we just add an array length parameter to the basic JSData2Native and
NativeData2JS methods we can handle them in the same place as every other type.

This also allows us to share a lot of code with non-sized string types, which is
nice :-).

Comment 8

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

Attachment: Part 8: Remove external consumers of XPCConvert::NativeArray2JS/JSArray2Native

Current XPIDL native arrays currently also require a custom entry point. With
the new arraylen parameter we can handle them in JSData2Native/NativeData2JS. As
these methods are more complex and don't share logic with an existing codepath,
I keep them in external helper methods.

Comment 9

[Andrew McCreight [:mccr8]]

As a warning, this will take me a while to get through because I haven't looked at this code before.

Comment 10

[Andrew McCreight [:mccr8]]

Comment on attachment 8972169 [details] [diff] [review]
Part 3: Remove unnecessary #includes of xptinfo headers

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

nit: "unnecssary" -> "unnecessary" in the commit message.

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 8972171 [details] [diff] [review]
Part 5: Use modern JS APIs to root jsval temporaries in XPConnect

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

Jon, could you please look at the use of JS::UnsafeTraceRoot() and CustomAutoRooter? I'm not familiar with those APIs. Thanks.

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +1799,5 @@
> +    if (aType.Tag() == nsXPTType::T_JSVAL) {
> +        JS::UnsafeTraceRoot(aTrc, (JS::Value*)aVal,
> +                            "XPCWrappedNative::CallMethod param");
> +    }
> +    else if (aType.Tag() == nsXPTType::T_ARRAY && *(void**)aVal) {

nit: "} else if " ...

@@ +1806,5 @@
> +            return;
> +        }
> +
> +        for (uint32_t i = 0; i < aArrayLen; ++i) {
> +            TraceParam(aTrc, elty.ElementPtr(aVal, i), elty);

Are arrays-of-arrays something we need to support? It looks like this won't work in that case.

@@ +1816,5 @@
> +CallMethodHelper::trace(JSTracer* aTrc)
> +{
> +    // We need to note each of our initialized parameters which contain jsvals.
> +    for (nsXPTCVariant& param : mDispatchParams) {
> +        // JSVals are marked as needing cleanup (even though they don't).

That seems a little fragile. Maybe assert that the tag isn't JSVAL if we |continue|?

Comment 12

[Andrew McCreight [:mccr8]]

Comment on attachment 8972172 [details] [diff] [review]
Part 6: Ensure the extended types list has some basic types with known indexes

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

::: xpcom/reflect/xptinfo/xptinfo.h
@@ +267,5 @@
> +  // Indexes into the extra types array of a small set of known types.
> +  enum class Idx : uint8_t
> +  {
> +    INT8 = 0,
> +    UINT8,

Could you define these directly in terms of TD_INT8 etc. and thus not need the static_asserts? Or are they not available in the header or something?

Comment 13

[Andrew McCreight [:mccr8]]

Comment on attachment 8972173 [details] [diff] [review]
Part 7: Eliminate XPCConvert::NativeStringWithSize2JS/JSStringWithSize2Native

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

The way this patch shares code is really nice. However, I think you should leave the existing methods around, and implement them as a thin wrapper calling into your new method with the array length argument (with the string things asserting that the type is really a string type or something). Having all of the bare zeroes floating around in the call sites is a little confusing. This would also make your patch a lot smaller. :) Does that sound reasonable?

::: js/xpconnect/src/XPCConvert.cpp
@@ +99,5 @@
>  // static
>  bool
>  XPCConvert::NativeData2JS(MutableHandleValue d, const void* s,
> +                          const nsXPTType& type, const nsID* iid,
> +                          uint32_t arrlen, nsresult* pErr)

I think this should be arrLen everywhere, not arrlen.

Please assert that it is 0 unless the type is one of the weird string type things, here and in the other method.

@@ +440,5 @@
>      AutoJSContext cx;
>      if (pErr)
>          *pErr = NS_ERROR_XPC_BAD_CONVERT_JS;
>  
> +    bool sizeis = type.Tag() == TD_PSTRING_SIZE_IS ||

maybe "sizeIs"?

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 8972174 [details] [diff] [review]
Part 8: Remove external consumers of XPCConvert::NativeArray2JS/JSArray2Native

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

Do you mean, external to XPCConvert? I'm not sure what you mean there. Also, if people aren't supposed to call some methods, can you make them static in XPCConvert? Or at least add a comment that you should use method X instead.

::: js/xpconnect/src/xpcprivate.h
@@ +1987,5 @@
>       * @param count the number of items in the array
>       * @param scope the default scope to put on the new JSObjects' parent chain
>       * @param pErr [out] relevant error code, if any.
>       */
> +    static bool NativeArray2JS(JS::MutableHandleValue d, const void* const* s,

Why does this type have to change?

Comment 15

[Andrew McCreight [:mccr8]]

Ok, most of these patches were not too bad for me to review. Can the dipper types patch be split out into a separate bug? Removing an entire feature of XPCOM seems weird to shove into the middle of a series of XPConnect cleanup patches...

Comment 16

[Jon Coppeard (:jonco)]

Comment on attachment 8972171 [details] [diff] [review]
Part 5: Use modern JS APIs to root jsval temporaries in XPConnect

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

This is fine but I'd prefer use of Rooted<> over CustomAutoRooter to root this.

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +1131,5 @@
>  }
>  
>  /***************************************************************************/
>  
> +class MOZ_STACK_CLASS CallMethodHelper final : private JS::CustomAutoRooter

CustomAutoRooter is also something we're trying to kill off.

If you give this class a non-virtual trace method taking a JSTracer* only then you can use it inside a Rooted, like:

  Rooted<CallMethodHelper> helper(cx);

This will ensure the trace method gets called if necessary.

@@ +1797,5 @@
> +           uint32_t aArrayLen = 0)
> +{
> +    if (aType.Tag() == nsXPTType::T_JSVAL) {
> +        JS::UnsafeTraceRoot(aTrc, (JS::Value*)aVal,
> +                            "XPCWrappedNative::CallMethod param");

This is fine and UnsafeTraceRoot is the correct API here.  "Unsafe" in this case means "manually barriered", but barriers are not necessary on roots so perhaps we should call this something else.

Comment 17

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

(In reply to Andrew McCreight [:mccr8] from comment #11)
> @@ +1806,5 @@
> > +            return;
> > +        }
> > +
> > +        for (uint32_t i = 0; i < aArrayLen; ++i) {
> > +            TraceParam(aTrc, elty.ElementPtr(aVal, i), elty);
> 
> Are arrays-of-arrays something we need to support? It looks like this won't
> work in that case.

There's no way to specify nested arrays in xpidl, so no.



(In reply to Andrew McCreight [:mccr8] from comment #12)
> ::: xpcom/reflect/xptinfo/xptinfo.h
> @@ +267,5 @@
> > +  // Indexes into the extra types array of a small set of known types.
> > +  enum class Idx : uint8_t
> > +  {
> > +    INT8 = 0,
> > +    UINT8,
> 
> Could you define these directly in terms of TD_INT8 etc. and thus not need
> the static_asserts? Or are they not available in the header or something?

These aren't types, they are indexes into the extra types array, so they are actually unrelated values. They are needed specifically for array types in XPCVariant.



(In reply to Andrew McCreight [:mccr8] from comment #14)
> Do you mean, external to XPCConvert? I'm not sure what you mean there. Also,
> if people aren't supposed to call some methods, can you make them static in
> XPCConvert? Or at least add a comment that you should use method X instead.

Ok. I was trying to keep the changes smaller, but I can also just make these methods static.

> ::: js/xpconnect/src/xpcprivate.h
> @@ +1987,5 @@
> >       * @param count the number of items in the array
> >       * @param scope the default scope to put on the new JSObjects' parent chain
> >       * @param pErr [out] relevant error code, if any.
> >       */
> > +    static bool NativeArray2JS(JS::MutableHandleValue d, const void* const* s,
> 
> Why does this type have to change?

It makes the const-preserving cast work nicer. The types here don't really matter.



(In reply to Andrew McCreight [:mccr8] from comment #13)
> The way this patch shares code is really nice. However, I think you should
> leave the existing methods around, and implement them as a thin wrapper
> calling into your new method with the array length argument (with the string
> things asserting that the type is really a string type or something). Having
> all of the bare zeroes floating around in the call sites is a little
> confusing. This would also make your patch a lot smaller. :) Does that sound
> reasonable?

I don't think that makes as much sense with the context of the other changes I want to make. Simplifying the entry points has been my goal here.



(In reply to Andrew McCreight [:mccr8] from comment #15)
> Ok, most of these patches were not too bad for me to review. Can the dipper
> types patch be split out into a separate bug? Removing an entire feature of
> XPCOM seems weird to shove into the middle of a series of XPConnect cleanup
> patches...

These patches were originally leadup cleanups to Sequence<T> support, and that was made much cleaner by eliminating Dipper types. I see the elimination as a cleanup (as it's just removing unnecessary code entirely within XPConnect) rather than removing a feature. 

I can move it into a different patch if you want.

Comment 18

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

(In reply to Jon Coppeard (:jonco) from comment #16)
> This is fine but I'd prefer use of Rooted<> over CustomAutoRooter to root
> this.
> 
> ::: js/xpconnect/src/XPCWrappedNative.cpp
> @@ +1131,5 @@
> >  }
> >  
> >  /***************************************************************************/
> >  
> > +class MOZ_STACK_CLASS CallMethodHelper final : private JS::CustomAutoRooter
> 
> CustomAutoRooter is also something we're trying to kill off.
> 
> If you give this class a non-virtual trace method taking a JSTracer* only
> then you can use it inside a Rooted, like:
> 
>   Rooted<CallMethodHelper> helper(cx);
> 
> This will ensure the trace method gets called if necessary.

Oh! I saw that it was being used by SequenceRooter, and figured it was the more modern API. I can definitely change it to use the Rooted<T> system.

Comment 19

[Andrew McCreight [:mccr8]]

(In reply to :Nika Layzell from comment #17)
> > Are arrays-of-arrays something we need to support? It looks like this won't
> > work in that case.
> 
> There's no way to specify nested arrays in xpidl, so no.

Wait, so all of that code that does the ArrayElementType iteration like in nsXPTInterfaceInfo::GetSizeIsArgNumberForParam() just dead code? That would answer one of my questions about part 1.

> Ok. I was trying to keep the changes smaller, but I can also just make these
> methods static.

Well, like I said, if you just want to put a comment in saying to not use that method, that would be fine with me.

> I don't think that makes as much sense with the context of the other changes
> I want to make. Simplifying the entry points has been my goal here.

I guess I don't understand the context, or what you mean by entry points, so whatever you think is best is fine.

Comment 20

[Andrew McCreight [:mccr8]]

(In reply to :Nika Layzell from comment #17)
> These patches were originally leadup cleanups to Sequence<T> support, and
> that was made much cleaner by eliminating Dipper types. I see the
> elimination as a cleanup (as it's just removing unnecessary code entirely
> within XPConnect) rather than removing a feature. 

Alright. Maybe it will make more sense to me here once I have looked at the patch and learned something about dipper types. I don't know what they are.

Comment 21

[Andrew McCreight [:mccr8]]

(In reply to Andrew McCreight [:mccr8] from comment #19)
> Wait, so all of that code that does the ArrayElementType iteration like in
> nsXPTInterfaceInfo::GetSizeIsArgNumberForParam() just dead code? That would
> answer one of my questions about part 1.

Oh yikes, I just looked up how arrays are defined in XPIDL. That's not what I was expecting. Anyways. Also, I didn't realize that GetSizeIsArgNumberForParam() is always passed in 0 for dimension... yikes.

Comment 22

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

(In reply to Andrew McCreight [:mccr8] from comment #19)
> (In reply to :Nika Layzell from comment #17)
> > > Are arrays-of-arrays something we need to support? It looks like this won't
> > > work in that case.
> > 
> > There's no way to specify nested arrays in xpidl, so no.
> 
> Wait, so all of that code that does the ArrayElementType iteration like in
> nsXPTInterfaceInfo::GetSizeIsArgNumberForParam() just dead code? That would
> answer one of my questions about part 1.

Yes. It existed in the original code so I kept it (especially because it will be useful to have it around for the Sequence<T> types which can be nested). 

> > Ok. I was trying to keep the changes smaller, but I can also just make these
> > methods static.
> 
> Well, like I said, if you just want to put a comment in saying to not use
> that method, that would be fine with me.

OK. I think I will do that.

(In reply to Andrew McCreight [:mccr8] from comment #20)
> (In reply to :Nika Layzell from comment #17)
> > These patches were originally leadup cleanups to Sequence<T> support, and
> > that was made much cleaner by eliminating Dipper types. I see the
> > elimination as a cleanup (as it's just removing unnecessary code entirely
> > within XPConnect) rather than removing a feature. 
> 
> Alright. Maybe it will make more sense to me here once I have looked at the
> patch and learned something about dipper types. I don't know what they are.

Here's my attempt at an explanation:

In XPConnect, native values are passed around within nsXPTCMiniVariant objects. an nsXPTCMiniVariant contains 64-bits of data in a union type. 

nsXPTCMiniVariant values are created by the platform-specific glue code and passed into XPConnect proper when calling from C++ into JS.

When calling from JS into C++, we instead create nsXPTCVariant objects and pass them into the glue code. These objects have extra information in addition to the nsXPTCMiniVariant: namely they also have a `type` field with the type stored in the variant, space for flags, and a `ptr` field which is passed over IPC instead of the inner nsXPTCMiniVariant when a particular flag is set.

The JSValue type in XPConnect is always passed as a pointer to a JSValue object, both for in parameters and out parameters. This is handled by making the JSValue type be unconditionally flagged as `IsIndirect()` (as you can see here: https://searchfox.org/mozilla-central/rev/c0d81882c7941c4ff13a50603e37095cdab0d1ea/xpcom/reflect/xptinfo/xptinfo.h#371). This flag is also used for all types of outparameters. 

When the `IsIndirect()` flag is set, it means that the actual data is stored in the nsXPTCVariant's val field, and it sets the flag to tell the glue code to instead pass the `ptr` field (which is always pointing to the `val` field) into the C++ code.

In contrast "dipper" is a different and super weird flag. A "dipper" type is always passed as an "in" type (and thus always passed "directly"), even when it's actually an out parameter. XPConnect treats these types as though they are pointer types (nsAString*). This means that there is no space in the nsXPTCVariant to store the actual nsAString types when passing from JS into C++, so these values have to be allocated by a different mechanism (in the current code, there is a size 2 buffer for each string type in the context and once that buffer is exceeded, we heap allocate the nsString values).

In effect, the current state looks something like this:
+------------+---------------------+---------------------+
| type       | out (xpt/native)    | in (xpt/native)     |
+------------+---------------------+---------------------+
| TD_INT32   | indirect (int32_t*) | direct (int32_t)    |
+------------+---------------------+---------------------+
| TD_JSVAL   | indirect (JSValue*) | indirect (JSValue*) |
+------------+---------------------+---------------------+
| TD_ASTRING | direct (nsAString*) | direct (nsAString*) |
+------------+---------------------+---------------------+

All I do in this patch is ensure there is enough space in the nsXPTCVariant to fit the nsString value, and switch the type to being unconditionally indirect instead of using the dipper system. This allows me to delete a ton of dipper-specific code, and unify the indirect and string class codepaths.

It's OK to make nsXPTCVariant a bit bigger because they are (almost) always allocated on the stack.

(In reply to Andrew McCreight [:mccr8] from comment #21)
> Oh yikes, I just looked up how arrays are defined in XPIDL. That's not what
> I was expecting. Anyways. Also, I didn't realize that
> GetSizeIsArgNumberForParam() is always passed in 0 for dimension... yikes.

Yeah, it's... not great. There's a reason why I want to switch us to a Sequence<T> based system ^_^. Hopefully all of this sketchy stuff will go away soon.

Comment 23

[Andrew McCreight [:mccr8]]

(In reply to :Nika Layzell from comment #22)
> Yes. It existed in the original code so I kept it (especially because it
> will be useful to have it around for the Sequence<T> types which can be
> nested).
Ah, ok. That answers another one of my questions about the first patch.

Thanks for the explanation of dippers. I'll read it over when I get to that patch.

Comment 24

[Andrew McCreight [:mccr8]]

Comment on attachment 8972167 [details] [diff] [review]
Part 1: Unify xpconnect cleanup codepaths

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

Do you think we should have a targeted backport of the fixes in the code you landed in 61? Sorry my review took so long.

The commit message should say the name of the new unified method. The patch is large and changes a lot of things, so having the name of that would help guide readers.

There's really a lot of redundant code here that you are cleaning up!

nit: in the commit message, "parmaeter" should be "parameter".

::: js/xpconnect/src/XPCConvert.cpp
@@ -1330,5 @@
> -    case nsXPTType::T_DOUBLE        : POPULATE(double);         break;
> -    case nsXPTType::T_BOOL          : POPULATE(bool);           break;
> -    case nsXPTType::T_CHAR          : POPULATE(char);           break;
> -    case nsXPTType::T_WCHAR         : POPULATE(char16_t);       break;
> -    case nsXPTType::T_VOID          : NS_ERROR("bad type");     return false;

There are a number of cases here (like T_VOID) that return false in the original code, but that AFAICT will work with your patch. Is that okay? Or do they fail somewhere else?

@@ +1491,3 @@
>  
> +    // Allocate the destination array, and begin converting elements.
> +    *d = moz_xmalloc(count * type.Stride());

You dropped the OOM null check on moz_xmalloc.

@@ +1668,5 @@
> +
> +// Internal implementation details for xpc::CleanupValue.
> +
> +void
> +xpc::InnerCleanupValue(const nsXPTType& aType, void* aValue, uint32_t aArrayLen)

I think this should have an assert about aArrayLen being zero if the tag isn't for a string or array.

::: js/xpconnect/src/XPCWrappedJSClass.cpp
@@ +661,4 @@
>                                             nsXPTCMiniVariant* nativeParams,
>                                             uint32_t* result) const
>  {
> +    if (type.Tag() != nsXPTType::T_ARRAY &&

Alright, so before every call site to this method was guarded with a check if it is an array, but now you just accept every type and return 0 if it isn't an array or string. That makes sense, though the name is a little odd now.

@@ +673,4 @@
>  
>      // This should be enforced by the xpidl compiler, but it's not.
>      // See bug 695235.
> +    if (param.Type().Tag() != nsXPTType::T_U32) {

Why is this not an assert any more?

@@ +703,5 @@
>          }
> +
> +        *result = inner.GetInterface()->IID();
> +    }
> +    else if (inner.Tag() == nsXPTType::T_INTERFACE_IS) {

nit: } should be on the same line as the else

@@ +739,5 @@
>          if (!param.IsOut())
>              continue;
>  
> +        // Extract the array length so we can use it in CleanupValue.
> +        uint32_t arraylen = 0;

nit: this should be "arrayLen".

@@ -776,5 @@
>          if (!param.IsOut())
>              continue;
>  
> -        const nsXPTType& type = param.GetType();
> -        if (!type.deprecated_IsPointer())

What is this check and why is it okay to get rid of it?

@@ +747,5 @@
> +        MOZ_ASSERT(param.IsIndirect(), "Outparams are always indirect");
> +
> +        // The inOutOnly flag was introduced when consolidating two very similar
> +        // code paths in CallMethod in bug 1175513. I don't know if and why the
> +        // difference is necessary.

You should delete this part of the comment now that you have figured out why it is needed and documented it. :)

::: js/xpconnect/src/XPCWrappedNative.cpp
@@ +1278,5 @@
>  
>  CallMethodHelper::~CallMethodHelper()
>  {
> +    for (nsXPTCVariant& param : mDispatchParams) {
> +        // Only clean up values which need cleanup.

Maybe remove this comment? I think it isn't useful.

@@ -1281,5 @@
>  CallMethodHelper::~CallMethodHelper()
>  {
> -    uint8_t paramCount = mMethodInfo->GetParamCount();
> -    if (mDispatchParams.Length()) {
> -        for (uint8_t i = 0; i < paramCount; i++) {

Huh. As near as I can see, we weren't cleaning up everything in this array before.

@@ +1283,5 @@
> +        if (!param.DoesValNeedCleanup())
> +            continue;
> +
> +        // Only get the array length for arrays.
> +        uint32_t arraylen = 0;

nit: arrayLen.

@@ +1284,5 @@
> +            continue;
> +
> +        // Only get the array length for arrays.
> +        uint32_t arraylen = 0;
> +        if (param.type.Tag() == nsXPTType::T_ARRAY &&

Why do you special case arrays here, while most other places you get the length of non-array things and just return 0?

@@ +1288,5 @@
> +        if (param.type.Tag() == nsXPTType::T_ARRAY &&
> +            !GetArraySizeFromParam(param.type, UndefinedHandleValue, &arraylen))
> +            continue;
> +
> +        CleanupParam(param, param.type, arraylen);

This call to CleanupParam takes an array length as an argument, but the only CleanupParam I see here only takes two arguments. What am I missing?

@@ +1346,4 @@
>  
> +        *result = inner.GetInterface()->IID();
> +    }
> +    else if (inner.Tag() == nsXPTType::T_INTERFACE_IS) {

nit: } should be on the same line as the rest of it

@@ +1675,5 @@
>          else
>              src.setNull();
>      }
>  
> +    nsID param_iid = { 0 };

Does this actually initialize the entire nsID to 0? Should this instead have a call to param_iid->Clear() like you did elsewhere?

@@ +1828,4 @@
>  {
> +    uint32_t arraylen = 0;
> +    switch (type.Tag()) {
> +        // If we have a string type at the toplevel, it is allocated using the

nit: "toplevel" should be "top level", here and below.

Comment 25

[Andrew McCreight [:mccr8]]

Comment on attachment 8972170 [details] [diff] [review]
Part 4: Remove dipper types

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

It would be nice if you included your explanation of what dipper types were in the commit message.

Nathan, could you look at the extbuf in xptcall.h stuff and determine if that is okay or if there's a better way? The basic idea is that ExtendedVal is trying to essentially extend the union Union with some new fields.

::: xpcom/reflect/xptcall/xptcall.h
@@ +73,5 @@
> +
> +        // Storage for any extended variants, if present. The storage is
> +        // performed using a char buffer, as ExtendedVal is not a
> +        // standard-layout type, which we want nsXPTCVariant to be.
> +        char extbuf[sizeof(ExtendedVal)];

I don't know if this is okay to do or not. Nathan should take a look.

Comment 26

[Nathan Froyd [:froydnj]]

Comment on attachment 8972170 [details] [diff] [review]
Part 4: Remove dipper types

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

I am so glad mccr8 is reviewing the patches in this bug and not me.

::: xpcom/reflect/xptcall/xptcall.h
@@ +73,5 @@
> +
> +        // Storage for any extended variants, if present. The storage is
> +        // performed using a char buffer, as ExtendedVal is not a
> +        // standard-layout type, which we want nsXPTCVariant to be.
> +        char extbuf[sizeof(ExtendedVal)];

I think you want to throw MOZ_ALIGNAS_IN_STRUCT(ExtendedVal) on here; I'd also suggest making this `unsigned char`, just to make everybody agree on what kind of chars this contains.  

Without the alignment bit, there's no guarantee this array (and by extension, the struct) is aligned the same way as ExtendedVal itself.  (I am not entirely sure this makes a difference, since nsXPTCMiniVariant::Union will have the same-ish alignment as ExtendedVal, but better safe than sorry?)

@@ +83,5 @@
>  
> +    // Getters for data stored in the extended value buffer.
> +    ExtendedVal& Ext() {
> +        MOZ_ASSERT(IsIndirect(), "Ext() only supports indirect nsXPTCVariants!");
> +        return *(ExtendedVal*) &extbuf;

Maybe.h says GCC -Werror=strict-aliasing has/will complain about direct casts; you may want to consider using indirection through separate functions here.

Comment 27

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

Attachment: Part 9: Allow passing invalid values to xpidl arrays for compat reasons

This one sucks. I found out when running tests that we exploit some edge
case behaviour in our tests where passing an invalid value to an xpidl
array parameter will be passed as an empty array...

I figured that just respecting this behaviour for now is the easiest
approach, but we will probably want to fix it in the future.

Comment 28

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

(In reply to Andrew McCreight [:mccr8] from comment #24)
> Comment on attachment 8972167 [details] [diff] [review]
> Part 1: Unify xpconnect cleanup codepaths
> 
> Review of attachment 8972167 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Do you think we should have a targeted backport of the fixes in the code you
> landed in 61? Sorry my review took so long.

Perhaps. There are some edge cases which should probably be fixed. I think we should make the backportable patch in a different bug probably.


> The commit message should say the name of the new unified method. The patch
> is large and changes a lot of things, so having the name of that would help
> guide readers.

OK.

> ::: js/xpconnect/src/XPCConvert.cpp
> @@ -1330,5 @@
> > -    case nsXPTType::T_DOUBLE        : POPULATE(double);         break;
> > -    case nsXPTType::T_BOOL          : POPULATE(bool);           break;
> > -    case nsXPTType::T_CHAR          : POPULATE(char);           break;
> > -    case nsXPTType::T_WCHAR         : POPULATE(char16_t);       break;
> > -    case nsXPTType::T_VOID          : NS_ERROR("bad type");     return false;
> 
> There are a number of cases here (like T_VOID) that return false in the
> original code, but that AFAICT will work with your patch. Is that okay? Or
> do they fail somewhere else?

I don't _think_ xpidl will let you do hit those codepaths, but I can do some 
extra checks here now.

> ::: js/xpconnect/src/XPCWrappedJSClass.cpp
> @@ +661,4 @@
> >                                             nsXPTCMiniVariant* nativeParams,
> >                                             uint32_t* result) const
> >  {
> > +    if (type.Tag() != nsXPTType::T_ARRAY &&
> 
> Alright, so before every call site to this method was guarded with a check
> if it is an array, but now you just accept every type and return 0 if it
> isn't an array or string. That makes sense, though the name is a little odd
> now.

Sure. I hope to delete this entire function in the near-ish future, so I don't know how much I care about that... :-/

> @@ +673,4 @@
> >  
> >      // This should be enforced by the xpidl compiler, but it's not.
> >      // See bug 695235.
> > +    if (param.Type().Tag() != nsXPTType::T_U32) {
> 
> Why is this not an assert any more?

There was another code path which was similar which didn't assert, and I figured it would be fine. I can make it an assert again if you care.

> @@ -776,5 @@
> >          if (!param.IsOut())
> >              continue;
> >  
> > -        const nsXPTType& type = param.GetType();
> > -        if (!type.deprecated_IsPointer())
> 
> What is this check and why is it okay to get rid of it?

This check was checking an old flag which was set if the value had a pointer representation to short circuit. It isn't useful anymore with the new changes to cleanup code. HasPointerRepr() replaces it.

> @@ -1281,5 @@
> >  CallMethodHelper::~CallMethodHelper()
> >  {
> > -    uint8_t paramCount = mMethodInfo->GetParamCount();
> > -    if (mDispatchParams.Length()) {
> > -        for (uint8_t i = 0; i < paramCount; i++) {
> 
> Huh. As near as I can see, we weren't cleaning up everything in this array
> before.

The other values we were skipping before will never have meaningful cleanup, and the new code feels cleaner to me.

> @@ +1284,5 @@
> > +            continue;
> > +
> > +        // Only get the array length for arrays.
> > +        uint32_t arraylen = 0;
> > +        if (param.type.Tag() == nsXPTType::T_ARRAY &&
> 
> Why do you special case arrays here, while most other places you get the
> length of non-array things and just return 0?

That's a good question. Fixed.

> 
> @@ +1288,5 @@
> > +        if (param.type.Tag() == nsXPTType::T_ARRAY &&
> > +            !GetArraySizeFromParam(param.type, UndefinedHandleValue, &arraylen))
> > +            continue;
> > +
> > +        CleanupParam(param, param.type, arraylen);
> 
> This call to CleanupParam takes an array length as an argument, but the only
> CleanupParam I see here only takes two arguments. What am I missing?

Uhh, looks like I messed up a rebase, oops.

> @@ +1675,5 @@
> >          else
> >              src.setNull();
> >      }
> >  
> > +    nsID param_iid = { 0 };
> 
> Does this actually initialize the entire nsID to 0? Should this instead have
> a call to param_iid->Clear() like you did elsewhere?

Yeah, if you don't list all of the fields the rest are set to 0. I find this more clear than ->Clear(), but *shrug*.

(In reply to Andrew McCreight [:mccr8] from comment #25)
> Comment on attachment 8972170 [details] [diff] [review]
> Part 4: Remove dipper types
> 
> Review of attachment 8972170 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> It would be nice if you included your explanation of what dipper types were
> in the commit message.

Ok. I'll add that.

(In reply to Nathan Froyd [:froydnj] from comment #26)
> Comment on attachment 8972170 [details] [diff] [review]
> Part 4: Remove dipper types
> 
> Review of attachment 8972170 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> I am so glad mccr8 is reviewing the patches in this bug and not me.

<3 you're welcome

> I think you want to throw MOZ_ALIGNAS_IN_STRUCT(ExtendedVal) on here; I'd
> also suggest making this `unsigned char`, just to make everybody agree on
> what kind of chars this contains.  
> 
> Without the alignment bit, there's no guarantee this array (and by
> extension, the struct) is aligned the same way as ExtendedVal itself.  (I am
> not entirely sure this makes a difference, since nsXPTCMiniVariant::Union
> will have the same-ish alignment as ExtendedVal, but better safe than sorry?)

Yeah, sure. I can also just use the support for non-pod enums and some constexpr constructors to get a pretty good result with the types inline perhaps? I can give 'er a prod & see how it looks.

> 
> @@ +83,5 @@
> >  
> > +    // Getters for data stored in the extended value buffer.
> > +    ExtendedVal& Ext() {
> > +        MOZ_ASSERT(IsIndirect(), "Ext() only supports indirect nsXPTCVariants!");
> > +        return *(ExtendedVal*) &extbuf;
> 
> Maybe.h says GCC -Werror=strict-aliasing has/will complain about direct
> casts; you may want to consider using indirection through separate functions
> here.

Interesting. I don't think we turn strict aliasing on in gecko but perhaps we should dodge that anyway. IIRC there was some thing about char* being castable to other types because it is allowed to alias other pointers even in strict or something like that? Not sure.

Comment 29

[Andrew McCreight [:mccr8]]

(In reply to :Nika Layzell from comment #28)
> Perhaps. There are some edge cases which should probably be fixed. I think
> we should make the backportable patch in a different bug probably.
Sounds good.

Whatever you think is best for the rest of it is fine with me.

Comment 30

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/37c92a1202ba
Part 1: Unify xpconnect cleanup codepaths, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/481c3b583e1f
Part 2: Remove unused code paths in xpconnect, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/5b3c21508aa4
Part 3: Remove unnecessary #includes of xptinfo headers, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/560dc697645d
Part 4: Remove dipper types, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/898c8bc882b1
Part 5: Use modern JS APIs to root jsval temporaries in XPConnect, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e5200c78b1ad
Part 6: Ensure the extended types list has some basic types with known indexes, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/5271eef135c4
Part 7: Eliminate XPCConvert::NativeStringWithSize2JS/JSStringWithSize2Native, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/1ce6d9f85a56
Part 8: Remove external consumers of XPCConvert::NativeArray2JS/JSArray2Native, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/18baccb9f4c6
Part 9: Allow passing invalid values to xpidl arrays for compat reasons, r=mccr8
https://hg.mozilla.org/integration/mozilla-inbound/rev/74ed1eb2a77d
Part 10: Make sure to allocate ExtendedVal inline, r=froydnj

Comment 31

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/37c92a1202ba
https://hg.mozilla.org/mozilla-central/rev/481c3b583e1f
https://hg.mozilla.org/mozilla-central/rev/5b3c21508aa4
https://hg.mozilla.org/mozilla-central/rev/560dc697645d
https://hg.mozilla.org/mozilla-central/rev/898c8bc882b1
https://hg.mozilla.org/mozilla-central/rev/e5200c78b1ad
https://hg.mozilla.org/mozilla-central/rev/5271eef135c4
https://hg.mozilla.org/mozilla-central/rev/1ce6d9f85a56
https://hg.mozilla.org/mozilla-central/rev/18baccb9f4c6
https://hg.mozilla.org/mozilla-central/rev/74ed1eb2a77d