960591 - nsTArray's ability to use arbitrary constructors taking 1 argument is a footgun

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benoit Jacob [:bjacob] (mostly away)]

Here's a story. I was debugging some code that had a leak. Eventually I found the code responsible for the leak, it was like this:


    class nsWillChange {
    public:
      explicit nsWillChange(const nsString& aProp)
        : mProperty(aProp)
      {}
      // Yes, it is bogus to write a copy-like constructor taking a pointer,
      // But I didn't know that code when I was debugging this.
      explicit nsWillChange(const nsWillChange* aCopy)
        : mProperty(aCopy->mProperty)
      {}

      const nsString& GetProperty() const { return mProperty; }
    private:
      nsString mProperty;
    };

    ...

    nsAutoTArray<nsWillChange, 1> mWillChange;

    ...

    mWillChange.AppendElement(new nsWillChange(buffer));


The question is why does that even compile? As mWillChange is an array of nsWillChange, I expected that calling AppendElement with a nsWillChange* would fail to compile. What is going on there is apparent in GDB:


#0  nsTArrayElementTraits<nsWillChange>::Construct<nsWillChange*> (e=0x7fffcba13268, arg=@0x7fffffff9f78: 0x7fffceb4e760)
    at ../../dist/include/nsTArray.h:527
#1  0x00007ffff1befd05 in AssignRangeAlgorithm<true, false>::implementation<nsWillChange*, nsWillChange, unsigned int, unsigned int> (elements=
    0x7fffcba13268, start=0, count=1, values=0x7fffffff9f78) at ../../dist/include/nsTArray.h:558
#2  0x00007ffff1befc71 in nsTArray_Impl<nsWillChange, nsTArrayInfallibleAllocator>::AssignRange<nsWillChange*> (this=0x7fffceb4c050, start=0, count=1, 
    values=0x7fffffff9f78) at ../../dist/include/nsTArray.h:1593
#3  0x00007ffff1befc0c in nsTArray_Impl<nsWillChange, nsTArrayInfallibleAllocator>::AppendElements<nsWillChange*> (this=0x7fffceb4c050, array=
    0x7fffffff9f78, arrayLen=1) at ../../dist/include/nsTArray.h:1234
#4  0x00007ffff1bdd492 in nsTArray_Impl<nsWillChange, nsTArrayInfallibleAllocator>::AppendElement<nsWillChange*> (this=0x7fffceb4c050, item=
    @0x7fffffff9f78: 0x7fffceb4e760) at ../../dist/include/nsTArray.h:1248


Thus, nsTArray has some smart internal compile-time logic to let AppendElement accept any type that is accepted by any constructor of the ElementType. Looking back at our nsWillChange class above, this builds because nsWillChange has a bogus copy-like constructor taking a pointer instead of a reference.


I am not sure what is the right fix for this footgun. But it might simply be to rename affected methods like AppendElement to something that makes it explicit that what they take is an arbitrary constructor argument, i.e.

  1) restrict AppendElement to using only the plain copy constructor
  2) rename the fancy clever AppendElement taking arbitrary constuctor argument to "AppendElementConstructedFrom" or some such.

Thoughts?

Comment 1

[Benoit Jacob [:bjacob] (mostly away)]

Also note that the renaming proposed above could also solve another issue. The current right way to write the above faulty AppendElement call is

    mWillChange.AppendElement(buffer);

But that is not very explicit (hence hard to read), because one expects AppendElement to take a ElementType (i.e. here a nsWillChange), not a random "buffer" thing. As this code is not performance-critical and the cost of a useless temporary and copy does not matter here, I am currently writing this code as

    mWillChange.AppendElement(nsWillChange(buffer));

which is a lot more explicit. But if this AppendElement taking a buffer were renamed to something like AppendElementConstructedFrom, then I could use it and still get quite explicit code:

    mWillChange.AppendElementConstructedFrom(buffer);

Comment 2

[Trevor Saunders (:tbsaunde)]

> I am not sure what is the right fix for this footgun. But it might simply be
> to rename affected methods like AppendElement to something that makes it
> explicit that what they take is an arbitrary constructor argument, i.e.
> 
>   1) restrict AppendElement to using only the plain copy constructor
>   2) rename the fancy clever AppendElement taking arbitrary constuctor
> argument to "AppendElementConstructedFrom" or some such.
> 
> Thoughts?

We have a lot of code that does things morally equivilent to

nsTArray<nsRefPtr<Foo>> x;
Foo* y = Bar();
x.AppendElement(y);
Which seems pretty reasonable, requiring AppendElementConstructFrom() there seems kind of painful.

So maybe we want to require T* but have overloads for refptrs and the like? However having a global list of that sort of thing there seems pretty painful too, so I'm not really sure.

Comment 3

[(no longer active)]

Let's break those kinds of constructors and only those.

Comment 4

[Benoit Jacob [:bjacob] (mostly away)]

Attachment: Make Ehsan more happy

https://tbpl.mozilla.org/?tree=Try&rev=50c2df8d4351

Comment 5

[(no longer active)]

Comment on attachment 8361285 [details] [diff] [review]
Make Ehsan more happy

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

::: xpcom/glue/nsTArray.h
@@ +524,5 @@
>    // Invoke the copy-constructor in place.
>    template<class A>
>    static inline void Construct(E *e, const A &arg) {
> +    typedef typename mozilla::RemoveCV<E>::Type CleanE;
> +    typedef typename mozilla::RemoveCV<A>::Type CleanA;

Nit: please give these a better name, such as UnderlyingEType, etc.

Comment 6

[Benoit Jacob [:bjacob] (mostly away)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7eb615c086e8

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/7eb615c086e8