1331137 - nsTArray::AppendElement bypasses |explicit| keyword somehow

Status: NEW

(Core :: XPCOM, defect, P2)

Description

[Bobby Holley (:bholley)]

I just noticed this while doing some refactoring. I might just be missing something obvious, but I wanted to get this on file. Note that I'm currently refactoring the code demonstrating the issue, so it may change soon. I'm permalinking the source, but please coordinate with me before any drafting any rototiller fixup patches.

nsIAnonymousContentCreator::ContentInfo has an explicit single-arg constructor [1]. However, there are lots of places in the code that seem to use the single-arg constructor implicitly [2] (basically most CreateAnonymousContent overrides). I tried deleting the constructor, and got lots of compilation failures trying to invoke nsTArrayElementTraits::Construct [3] [4].

I think there's something weird going on with Forward<>, but don't have time to sort out exactly what.

[1] http://searchfox.org/mozilla-central/rev/3f614bdf91a2379a3e2c822da84e524f5e742121/layout/generic/nsIAnonymousContentCreator.h#32
[2] http://searchfox.org/mozilla-central/rev/3f614bdf91a2379a3e2c822da84e524f5e742121/layout/generic/DetailsFrame.cpp#121
[3] http://searchfox.org/mozilla-central/rev/0aed9484bd3e97206fd1949ee4a4992ef300a81f/xpcom/glue/nsTArray.h#2221
[4] http://searchfox.org/mozilla-central/rev/0aed9484bd3e97206fd1949ee4a4992ef300a81f/xpcom/glue/nsTArray.h#667

Comment 1

[Bobby Holley (:bholley)]

Gerald, any idea what's going on here?

Comment 2

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

(In reply to Bobby Holley (:bholley) (busy with Stylo) from comment #1)
> Gerald, any idea what's going on here?

AppendElement is intentionally implemented as an in-place construct method. The method is templated over its argument type, and uses elem_traits::Construct() to in-place construct the type. This allows for inplace construction of `explicit` constructed types, as the constructor is invoked explicitly.

http://searchfox.org/mozilla-central/rev/0aed9484bd3e97206fd1949ee4a4992ef300a81f/xpcom/glue/nsTArray.h#2221

This change was made in bug 1053420 AFAICT in order to attempt to intelligently move construct into the array. I imagine it would be possible to add the constraint you're looking for through some clever use of TypeTraits.h, but I'd have to put some thought into exactly what it would look like.

Comment 3

[Bobby Holley (:bholley)]

To be clear, I don't have a burning need for this constraint, but the current setup does seem to bypass the hard work we've done to make things |explicit|.

Comment 4

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

What Nika said in comment 2... Except I think the naughty 'template <class Item>' that accepts anything comes from bug 325331 -- 11 years ago! (Nice find, Bobby) :-)

Looking at std::vector<T>, they have distinct 'push_back' that only takes a T and therefore prevents conversions with explicit constructors of T, and 'emplace_back' that takes construction arguments (like our nsTArray::AppendElement), and implicitly allows conversions.

Without investigating further, separate methods would probably be the way to go if we wanted some way to respect explicit-ness in most cases, while still allowing in-place construction where needed. But I'm guessing it would be a time-consuming undertaking!

Comment 5

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

As a Sunday exercise, I renamed nsTArray's AppendElement to AppendEmplace, and also copied it to equivalent AppendElement functions that only accept 'const elem_type&' or 'elem_type&&'. And same with the plural versions: AppendElements and AppendEmplaces.
After that, I mindlessly converted 234 AppendElement[s] to AppendEmplace[s] in 146 files to make them build again (on Mac).

It's not too bad I think, as there were 4800+ AppendElement[s] calls. Lots of these fixes were needed due to the different string types.
Example try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=0ff9a43444a605b709aa13df814a5c591e09a70c
(Obviously the non-Mac platforms would need more work.)

So I think it could be reasonable to continue with this work, and take care of other nsTArray methods as well, and maybe other containers too?
(Not me though, no more time to play with this now.)


As an interesting anecdote, while studying bug 1330869 (where I wanted to add a MakeMaybe() to construct a Maybe, to complement the existing Some() that only takes an object of the target type), I discovered that both were in fact optimized to exactly the same low number of instructions!

So assuming the same optimization works for AppendElement vs AppendEmplace, one cleaner solution could be to just get rid of AppendEmplace, and instead only keep the newly-restricted AppendElement, and where necessary use explicit construction of objects with explicit constructors, e.g.: `array.AppendElement(ContentInfo(mDefaultSummary))`.

Comment 6

[Bobby Holley (:bholley)]

(In reply to Gerald Squelart [:gerald] from comment #5)
> So I think it could be reasonable to continue with this work, and take care
> of other nsTArray methods as well, and maybe other containers too?
> (Not me though, no more time to play with this now.)

Please post your patch here for posterity?

Comment 7

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

Attachment: 1331137-partial-solution.patch

I thought the 'try' would be enough, but these things expire!

So here's my attempt at an imperfect fix, currently only building on Mac.

Comment 8

[JW Wang [:jwwang] [:jw_wang]]

It is good to separate AppendEmplace from AppendElement because I am surprised the following code doesn't work:

  struct Foo {
    Foo(int, int) {}
    Foo(int) {}
  };
  nsTArray<Foo> arr;
  Foo foo1 = 94; //OK
  Foo foo2 = {87}; //OK
  arr.AppendElement(94); //OK
  Foo foo3 = {94, 87}; //OK
  arr.AppendElement({94, 87}); //KO!
  arr.AppendElement(94, 87); //KO!

Comment 9

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to JW Wang [:jwwang] [:jw_wang] from comment #8)
>   arr.AppendElement({94, 87}); //KO!
I think this one is because {94, 87} becomes an initializer list when forwarded, and there's no Foo constructor for that.

>   arr.AppendElement(94, 87); //KO!
This one is because AppendElement only accepts *one* argument (of any type). A variadic AppendEmplace would fix that.

> It is good to separate AppendEmplace from AppendElement
I agree that we should at least have a distinct nsTArray<E>::AppendElement taking only an E.

My follow-up question was whether we really needed a separate AppendEmplace(Args&&...), when the same effect could be achieved (I think) by just explicitly building a temporary E and giving it to AppendElement: arr.AppendElement(E(...)).

Put another way: Are there use cases that emplace-style functions allow but that a copy-or-move functions do not?
(If yes, I'd like to know, so I can reopen bug 1330869!) :-)

Comment 10

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

(In reply to Gerald Squelart [:gerald] from comment #9)
> Put another way: Are there use cases that emplace-style functions allow but
> that a copy-or-move functions do not?
> (If yes, I'd like to know, so I can reopen bug 1330869!) :-)

Yes, the following class could be added to the array potentially with emplace but not push:

class Foo {
public:
  Foo(int, int);
  Foo(const Foo&) = delete;
  Foo(Foo&&) = delete;
};

As it cannot be move or copy constructed, and instead must be constructed in place. Your proposed AppendElement on that type could not compile, while an AppendEmplace could. 

That being said, putting a value of that type inside of an nsTArray would probably not work out well just because we would need to be able to relocate the values for the purposes of array resizing.

Comment 11

[Simon Giesecke [:sg] [he/him]]

FWIW, this is also possible through nsTArrayBackInserter/MakeBackInserter after Bug 1184468. We could change that specifically again by replacing the templated operator= by two separate operator=(const E&) and operator=(E&&), but actually the templated operator as such is fine. But since changing that in AppendElement seems to be problematic, we might prohibit that specifically for nsTArrayBackInserter/MakeBackInserter for the time being.

Comment 12

[Simon Giesecke [:sg] [he/him]]

Attachment: Bug 1331137 - Disallow explicit conversions with nsTArray::AppendElement.

Comment 13

[BugBot [:suhaib / :marco]]

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Comment 14

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

It'd be nice to land this, but it's probably bitrotted a bit.