815671 - Make nsTArray constructors explicit

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Boris Zbarsky [:bzbarsky]]

nsTArray has two non-explicit constructors.  One of these has this signature:

  template<typename Allocator>
  nsTArray(const nsTArray<E, Allocator>& other);

And copies on construction.  This is a footgun, because nsTArray and InfallibleTArray have different allocators, so if you have a function with this signature:

  void Foo(const nsTArray<whatever>&);

and you pass an InfallibleTArray<whatever> to the function, the call will make temporary copy of the array.  We were in fact doing just that in a bunch of cases, as I discovered when I made the constructor explicit...

Patches coming up to make the two non-explicit constructors explicit and fix all the resulting build breakage.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Make RegisterRemoteChrome use InfallibleTArray like its caller does.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Make some content code use array references instead of array objects with implicit copies.

Comment 3

[Justin Lebar (not reading bugmail)]

> nsTArray and InfallibleTArray have different allocators

This is a bummer.

Do we still support fallible-malloc?  If not, can we just make nsTArray and InfallibleTArray identical types?

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Make nsXULPopupManager::GetVisiblePopups use an array out param instead of returning array object copies.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Remove some array copying in gfx code.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Various miscellaneous fixups in dom/ and hal/ code to copy arrays only when we explicitly want to.

Comment 7

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Various miscellaneous fixups in dom/ and hal/ code to copy arrays only when we explicitly want to.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 6. Fixes to widget code to not copy arrays implicitly.

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 7. Fixes to layout code to not copy arrays implicitly.

Comment 10

[Boris Zbarsky [:bzbarsky]]

Attachment: part 8. Fix charset menu code to use copy constructors for arrays.

Comment 11

[Benjamin Smedberg]

We do support fallible TArray, although it is not the default: the default nsTArray allocator is the infallible one in all the cases that matter to us: http://mxr.mozilla.org/mozilla-central/source/xpcom/glue/nsTArray.h#101

Comment 12

[Boris Zbarsky [:bzbarsky]]

Attachment: part 9. Fix hashtables to take a UserDataType reference on put, so that putting an array works.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Attachment: part 10. Make nsTArray constructors explicit.

Comment 14

[Boris Zbarsky [:bzbarsky]]

> If not, can we just make nsTArray and InfallibleTArray identical types?

I'm not the one to make this call, but note that I'd want these patches anyway because I want to prevent accidental copying from dom::Sequence (which is fallible) to nsTArray...

Comment 15

[Justin Lebar (not reading bugmail)]

> We do support fallible TArray, although it is not the default: the default nsTArray 
> allocator is the infallible one in all the cases that matter to us:

Right.  The issue is that the allocator nsTArray uses by default (nsTArrayDefaultAllocator) is not the same type as the allocator used by InfallibleTArray (nsTArrayInfallibleAllocator), even though, when MOZALLOC_HAVE_XMALLOC is defined, the two types function identically:

> #if defined(MOZALLOC_HAVE_XMALLOC)
> struct nsTArrayDefaultAllocator : public nsTArrayInfallibleAllocator { };
> #else
> struct nsTArrayDefaultAllocator : public nsTArrayFallibleAllocator { };
> #endif

I imagine we did it this way because, if !defined(MOZALLOC_HAVE_XMALLOC) is a supported platform, we don't want any code which assumes that nsTArray (with the default allocator) and InfallibleTArray are the same.

But the question is: Do we support !defined(MOZALLOC_HAVE_XMALLOC)?  If not, can we make InfallibleTArray a typedef for nsTArray with the default allocator?  Then we'd have InfallibleTArray<T> == nsTArray<T>.

This would have the advantage of improving interoperability between code which uses InfallibleTArray and code which uses nsTArray.

> I'm not the one to make this call

Is bsmedberg?

Comment 16

[Boris Zbarsky [:bzbarsky]]

> Is bsmedberg?

I think he could, yeah.

Comment 17

[Justin Lebar (not reading bugmail)]

Actually, we don't even need MOZALLOC_HAVE_XMALLOC if we can just write our own implementation inside the default allocator.

Comment 18

[Justin Lebar (not reading bugmail)]

Comment on attachment 685664 [details] [diff] [review]
part 1.  Make RegisterRemoteChrome use InfallibleTArray like its caller does.

Okay, but it would still be nice if we could make nsTArray<T> == InfallibleTArray<T>, I think...

Comment 19

[Justin Lebar (not reading bugmail)]

Comment on attachment 685674 [details] [diff] [review]
part 5.  Various miscellaneous fixups in dom/ and hal/ code to copy arrays only when we explicitly want to.

r=me, it seems clearly preferable if we were to make nsTArray<T> == InfallibleTArray<T> instead.

Comment 20

[Boris Zbarsky [:bzbarsky]]

> it seems clearly preferable if we were to make nsTArray<T> == InfallibleTArray<T> instead.

Totally agreed...

Comment 21

[Justin Lebar (not reading bugmail)]

Comment on attachment 685675 [details] [diff] [review]
part 6.  Fixes to widget code to not copy arrays implicitly.

>diff --git a/widget/windows/nsWindow.cpp b/widget/windows/nsWindow.cpp
>--- a/widget/windows/nsWindow.cpp
>+++ b/widget/windows/nsWindow.cpp
>-static const nsTArray<nsIntRect>
>-ArrayFromRegion(const nsIntRegion& aRegion)
>-{
>-  nsTArray<nsIntRect> rects;
>+static void
>+ArrayFromRegion(const nsIntRegion& aRegion, nsTArray<nsIntRect>& aRects)
>+{
>   const nsIntRect* r;
>   for (nsIntRegionRectIterator iter(aRegion); (r = iter.Next());) {
>-    rects.AppendElement(*r);
>-  }
>-  return rects;
>+    aRects.AppendElement(*r);
>+  }
> }

This one should be fine due to return-value optimization, but I guess your goal
is to disallow all implicit copies of nsTArray, so you have to change this?

Comment 22

[Justin Lebar (not reading bugmail)]

Comment on attachment 685677 [details] [diff] [review]
part 8.  Fix charset menu code to use copy constructors for arrays.

>diff --git a/toolkit/components/intl/nsCharsetMenu.cpp b/toolkit/components/intl/nsCharsetMenu.cpp
>--- a/toolkit/components/intl/nsCharsetMenu.cpp
>+++ b/toolkit/components/intl/nsCharsetMenu.cpp
>@@ -813,17 +813,17 @@ nsresult nsCharsetMenu::InitBrowserMenu(
> 
>   if (!mBrowserMenuInitialized)  {
>     nsCOMPtr<nsIRDFContainer> container;
>     res = NewRDFContainer(mInner, kNC_BrowserCharsetMenuRoot, getter_AddRefs(container));
>     if (NS_FAILED(res)) return res;
> 
> 
>     // how to clone mDecoderList??
>-    nsTArray<nsCString> browserDecoderList = mDecoderList;
>+    nsTArray<nsCString> browserDecoderList(mDecoderList);

Perhaps this comment isn't needed anymore?  :)

Comment 23

[Justin Lebar (not reading bugmail)]

Comment on attachment 685682 [details] [diff] [review]
part 10.  Make nsTArray constructors explicit.

Cool.

Comment 24

[Boris Zbarsky [:bzbarsky]]

> your goal is to disallow all implicit copies of nsTArray, so you have to change this?

Yes.  I wish I could have left the cases that return-value optimization already handled as they were, but C++ won't let me.  :(

> Perhaps this comment isn't needed anymore?  :)

Yes, indeed.  Removed it.

Comment 25

[Boris Zbarsky [:bzbarsky]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9cc4fa4f431b
https://hg.mozilla.org/integration/mozilla-inbound/rev/6469b7a5ea10
https://hg.mozilla.org/integration/mozilla-inbound/rev/0bba55aab571
https://hg.mozilla.org/integration/mozilla-inbound/rev/453b63c3f220
https://hg.mozilla.org/integration/mozilla-inbound/rev/c2e33f49c60f
https://hg.mozilla.org/integration/mozilla-inbound/rev/ef768d59bb71
https://hg.mozilla.org/integration/mozilla-inbound/rev/414f6acb1986
https://hg.mozilla.org/integration/mozilla-inbound/rev/ae518bf8df99
https://hg.mozilla.org/integration/mozilla-inbound/rev/58340dfc22cb
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e1e34df0e83

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9cc4fa4f431b
https://hg.mozilla.org/mozilla-central/rev/6469b7a5ea10
https://hg.mozilla.org/mozilla-central/rev/0bba55aab571
https://hg.mozilla.org/mozilla-central/rev/453b63c3f220
https://hg.mozilla.org/mozilla-central/rev/c2e33f49c60f
https://hg.mozilla.org/mozilla-central/rev/ef768d59bb71
https://hg.mozilla.org/mozilla-central/rev/414f6acb1986
https://hg.mozilla.org/mozilla-central/rev/ae518bf8df99
https://hg.mozilla.org/mozilla-central/rev/58340dfc22cb
https://hg.mozilla.org/mozilla-central/rev/3e1e34df0e83