Closed Bug 815671 Opened 7 years ago Closed 7 years ago

Make nsTArray constructors explicit

Categories

(Core :: XPCOM, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: bzbarsky, Assigned: bzbarsky)

Details

(Whiteboard: [MemShrink:P3])

Attachments

(10 files, 1 obsolete file)

2.40 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
4.63 KB, patch
smaug
: review+
Details | Diff | Splinter Review
5.28 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.08 KB, patch
roc
: review+
Details | Diff | Splinter Review
6.50 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
7.80 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
5.01 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.53 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
1.13 KB, patch
benjamin
: review+
Details | Diff | Splinter Review
1.75 KB, patch
justin.lebar+bug
: review+
Details | Diff | Splinter Review
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.
> 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?
Attachment #685673 - Attachment is obsolete: true
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
> 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...
Whiteboard: [need review] → [need review][MemShrink]
> 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?
> Is bsmedberg?

I think he could, yeah.
Actually, we don't even need MOZALLOC_HAVE_XMALLOC if we can just write our own implementation inside the default allocator.
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...
Attachment #685664 - Flags: review?(justin.lebar+bug) → review+
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.
Attachment #685674 - Flags: review?(justin.lebar+bug) → review+
Attachment #685666 - Flags: review?(bugs) → review+
> it seems clearly preferable if we were to make nsTArray<T> == InfallibleTArray<T> instead.

Totally agreed...
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?
Attachment #685675 - Flags: review?(justin.lebar+bug) → review+
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?  :)
Attachment #685677 - Flags: review?(justin.lebar+bug) → review+
Comment on attachment 685682 [details] [diff] [review]
part 10.  Make nsTArray constructors explicit.

Cool.
Attachment #685682 - Flags: review?(justin.lebar+bug) → review+
> 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.
Whiteboard: [need review][MemShrink] → [need review][MemShrink:P3]
Attachment #685678 - Flags: review?(benjamin) → review+
You need to log in before you can comment on or make changes to this bug.