819791 - Fix the nsTArray / InfallibleTArray / FallibleTArray type madness, and make InfallibleTArray and FallibleTArray's copy constructors explicit

Status: RESOLVED FIXED

(Core :: General, defect)

Description

[Justin Lebar (not reading bugmail)]

In nsTArray.h, the declarations of InfallibleTArray and FallibleTArray are preceded by the following comment:

> // Convenience subtypes of nsTArray.

But I disagree that InfallibleTArray and FallibleTArray are convenient.

The problem is that entirely equivalent types don't match up, due to the way they're declared.  To wit:

  1) typeof InfallibleTArray<T> != typeof nsTArray<T, nsTArrayInfallibleAlloc>,
  2) typeof InfallibleTArray<T> != typeof nsTArray<T>,
  3) typeof FallibleTArray<T> != typeof nsTArray<T, nsTArrayFallibleAlloc>.
  4) typeof nsTArray<T, nsTArrayInfallibleAlloc> != typeof nsTArray<T>

Fixing (4) is relatively simple, and I'll post a patch for that shortly.  This will change behavior on platforms/in files where MOZALLOC_HAVE_XMALLOC is not defined (because plain old nsTArray will always be infallible), but I think this a move in the right direction, for two reasons:

 a) we've decided infallibility trumps fallibility, so more infallibility is good, and
 b) when reading any piece of code, you can now safely assume that nsTArray is infallible.

Any affected code (I don't know if there even is any, but maybe binary extensions, or that sort of thing) can switch to using FallibleTArray where it really wants fallibility.

Fixing (1-3) is more complex.  The simple thing to do would be to use alias templates [1], but they are C++11-only.

One piece of data is that as far a I can tell, we don't use custom nsTArray Allocs anywhere; all consumers use nsInfallibleTArrayAlloc or nsFallibleTArrayAlloc [2].

Here are a few different solutions we can choose from:

I.   Keep FallibleTArray<T> and nsTArray<T>.  nsTArray is infallible.  Remove
     InfallibleTArray<T> from the tree and replace them with nsTArray<T>.

     Add an internal base class (e.g.  mozilla::internal::TArray<T, Alloc>), and
     then make

       class nsTArray<T> : public TArray<T, nsTArrayInfallibleAlloc>
       class FallibleTArray<T> : public TArray<T, nsTArrayFallibleAlloc>

II.  Remove InfallibleTArray<T> and FallibleTArray<T>.  nsTArray<T, Alloc> is
     infallible by default.

     In place of FallibleArray<T>, we have nsTArray<T>::FallibleT, which is
     implemented as

       class nsTArray<T, Alloc=nsTArrayInfallibleAlloc> {
	 typedef nsTArray<T, nsTArrayFallibleAlloc> FallibleT;
       };

III. Like II, but also create mozilla::internal::TArray<T, Alloc>.  Then
     nsTArray<T> doesn't take an Alloc template parameter.

     That is, we'd have

       class nsTArray<T> : public TArray<T, nsTArrayInfallibleAlloc> {
         typedef TArray<T, nsTArrayFallibleAlloc> FallibleT;
       };

I'm tempted to go with (I), because it has the least typedef magic and because,
unlike (II), it accomplishes my goal of making the public nsTArray class not
take an Alloc template parameter.

I'm curious if anyone else has an opinion on this.

[1] http://en.wikipedia.org/wiki/C%2B%2B11#Alias_templates
[2] hg locate | xargs egrep 'nsTArray<[^,>]+,[^>]+'

Comment 1

[(no longer active)]

I also support (I) above.  I don't think that the other two solutions have any advantage over (I) and they're both awkward at least.

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Part 1: Remove nsTArrayDefaultAllocator, replacing it unconditionally with nsTArrayInfallibleAllocator.

Comment 3

[Masatoshi Kimura [:emk]]

(In reply to Justin Lebar [:jlebar] from comment #0)
> This will change behavior on platforms/in files where MOZALLOC_HAVE_XMALLOC
> is not defined (because plain old nsTArray will always be infallible),
What exactly platforms? MOZALLOC_HAVE_XMALLOC is unconditionally defined in mozalloc.h.

> Any affected code (I don't know if there even is any, but maybe binary
> extensions, or that sort of thing) can switch to using FallibleTArray where
> it really wants fallibility.
Is FallibleTArray usable from external linkage? It looks like MOZALLOC_HAVE_XMALLOC is not defined in external linkage.

Comment 4

[Boris Zbarsky [:bzbarsky]]

I like I as well.

If we do that, can we still typedef nsTArray to InfallibleTArray if desired?

Comment 5

[Justin Lebar (not reading bugmail)]

> If we do that, can we still typedef nsTArray to InfallibleTArray if desired?

We could #define it, but I don't think we could typedef it.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Sigh, C++.  OK.

Comment 7

[Justin Lebar (not reading bugmail)]

This turned out to be more involved than I expected, but I think I got it to work and fixed some inadvertent array copies while I was at it.

I'll split up the patches and ask for review once I get this green on try.

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

Comment 8

[Justin Lebar (not reading bugmail)]

erm, https://tbpl.mozilla.org/?tree=Try&rev=58dc36452e2f

Comment 9

[Justin Lebar (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=1cd2c535cf5d

Comment 10

[Justin Lebar (not reading bugmail)]

And Android fixes: https://tbpl.mozilla.org/?tree=Try&rev=a3afd75e391a

Comment 11

[Justin Lebar (not reading bugmail)]

Getting there...https://tbpl.mozilla.org/?tree=Try&rev=810f1576005f

Comment 12

[Justin Lebar (not reading bugmail)]

https://tbpl.mozilla.org/?tree=Try&rev=017ca1b4c2d8

Comment 13

[Justin Lebar (not reading bugmail)]

Attachment: Part 0: Deserialize fallibly in IPCMessageUtils.h.

Comments in the code are asking for us to deserialize fallibly; it looks
like we just forgot to change this when we made nsTArray infallible.

Comment 14

[Justin Lebar (not reading bugmail)]

Attachment: Part 1: Remove nsTArrayDefaultAllocator, replacing it unconditionally with nsTArrayInfallibleAllocator.

Comment 15

[Justin Lebar (not reading bugmail)]

Attachment: Part 2: Don't use nsTArray allocators in IPCMessageUtils traits.

The allocators will be restricted to non-public use in a later patch.

Comment 16

[Justin Lebar (not reading bugmail)]

Attachment: Part 3: Make nsTArray == InfallibleTArray and nsAutoTArray == AutoInfallibleTArray, and switch files to using nsTArrayForwardDeclare.h.

Comment 17

[Justin Lebar (not reading bugmail)]

Attachment: Part 4: Remove an unnecessary TArray copy in OrientationObserver.cpp.

Comment 18

[Justin Lebar (not reading bugmail)]

Attachment: Part 5: Remove an unnecessary TArray copy in hal/sandbox.

Comment 19

[Justin Lebar (not reading bugmail)]

Attachment: Part 6: Remove unnecessary TArray copies in dom/indexedDB.

Comment 20

[Justin Lebar (not reading bugmail)]

Attachment: Part 7: Use explicit TArray copy constructors in Bluetooth code.

Comment 21

[Justin Lebar (not reading bugmail)]

Attachment: Part 8: Use explicit TArray copy constructors in FrameLayerBuilder.

Comment 22

[Justin Lebar (not reading bugmail)]

Attachment: Part 9: Use explicit TArray copy constructors in IPDL generated code.

This makes two nop changes to generated IPDL code:

1) Change an instance of

  InfallibleTArray<Foo> foo = InfallibleTArray<Foo>();

to

  InfallibleTArray<Foo> foo;

2) Change an instance of

  InfallibleTArray<Foo> foo = bar;

to

  InfallibleTArray<Foo> foo(bar);

Comment 23

[Justin Lebar (not reading bugmail)]

Attachment: Part 10: Rewrite storage/src/Variant.h's element traits not to rely on returning FallibleTArrays by value, since that's not compatible with explicit TArray copy constructors.

Comment 24

[Justin Lebar (not reading bugmail)]

Attachment: Part 11: Make nsTArray and friends' copy constructors explicit.

Comment 25

[Justin Lebar (not reading bugmail)]

FTR, there's a lot of intermittent BC orange corresponding to known bugs in my try pushes.  It seems pretty unlikely that this is caused by my patches here, so my plan is to push to m-i and be ready to back out if it turns out that I'm actually somehow tickling these oranges.

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 693445 [details] [diff] [review]
Part 1: Remove nsTArrayDefaultAllocator, replacing it unconditionally with nsTArrayInfallibleAllocator.

>+class nsTArrayInfallibleAllocator;

struct, no?

>+    fputs("OOM\n", stderr);

Is that purposeful?  If so, maybe "Out of memory allocating nsTArray buffer" at least?

r=me

Comment 27

[Justin Lebar (not reading bugmail)]

>+    fputs("OOM\n", stderr);

I was matching/cargo-culting what mozalloc does; on OOM it eventually calls into mozalloc_abort.cpp::mozalloc_abort and does fputs("out of memory", stderr);.  I have no idea if that's actually useful; if you think it isn't, I can remove the fputs in nsTArray!  Otherwise I'll change the message as you suggest.

Comment 28

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 693448 [details] [diff] [review]
Part 3: Make nsTArray == InfallibleTArray and nsAutoTArray == AutoInfallibleTArray, and switch files to using nsTArrayForwardDeclare.h.

r=me given that those non-explicit ctors go away later.

Comment 29

[Chris Jones [:cjones] inactive; ni?/f?/r? if you need me]

Comment on attachment 693456 [details] [diff] [review]
Part 9: Use explicit TArray copy constructors in IPDL generated code.

># HG changeset patch
># User Justin Lebar <[e-mail]>
>
>Bug 819791 - Part 9: Use explicit TArray copy constructors in IPDL generated code.
>
>diff --git a/ipc/ipdl/ipdl/lower.py b/ipc/ipdl/ipdl/lower.py
>index dd62fad..29cd721 100644
>--- a/ipc/ipdl/ipdl/lower.py
>+++ b/ipc/ipdl/ipdl/lower.py
>@@ -3287,17 +3287,17 @@ class _GenerateProtocolActorCode(ipdl.ast.Visitor):
> 
>             block = StmtBlock()
>             block.addstmts([
>                 Whitespace(
>                     '// Recursively shutting down %s kids\n'% (managed.name()),
>                     indent=1),
>                 StmtDecl(
>                     Decl(p.managedVarType(managed, self.side), kidsvar.name),
>-                    init=p.managedVar(managed, self.side)),
>+                    initargs=[p.managedVar(managed, self.side)]),

Nit: [ spaceInsideBrackets ]

Comment 30

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 693458 [details] [diff] [review]
Part 11: Make nsTArray and friends' copy constructors explicit.

r=me

Comment 31

[Boris Zbarsky [:bzbarsky]]

Leaving the fputs is fine, with the better message.

Comment 32

[Ben Turner (not reading bugmail, use the needinfo flag!)]

Comment on attachment 693452 [details] [diff] [review]
Part 6: Remove unnecessary TArray copies in dom/indexedDB.

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

Oops!

Comment 33

[Marco Bonardo [:mak]]

Comment on attachment 693457 [details] [diff] [review]
Part 10: Rewrite storage/src/Variant.h's element traits not to rely on returning FallibleTArrays by value, since that's not compatible with explicit TArray copy constructors.

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

It took a bit more, since I didn't recall this part of the code very much, but I think I got it and the changes look correct.

::: storage/src/Variant.h
@@ +58,5 @@
>  struct variant_storage_traits
>  {
>    typedef DataType ConstructorType;
>    typedef DataType StorageType;
> +  static inline void storage_conversion(const ConstructorType aData, StorageType* aStorage)

For coherence with this module code style, I'd prefer if output params would be prefixed with "_", so or example "_storage" in this case... not going to repeat, but applies multiple times.

Also, I'd prefer if you'd keep consistent naming across the change, here you call the in param aData and out param aStorage, but below aData is the out param. That's a bit confusing. So what about keeping aData here for in param, but rename all out params to _outData?

Comment 34

[Justin Lebar (not reading bugmail)]

Wow, I feel like Christmas came early -- thanks for the fast reviews, everyone!

Comment 35

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2c74293986cf
https://hg.mozilla.org/integration/mozilla-inbound/rev/24e3a92d51d4
https://hg.mozilla.org/integration/mozilla-inbound/rev/28b5c8d25fb9
https://hg.mozilla.org/integration/mozilla-inbound/rev/1a0cd3aa1864
https://hg.mozilla.org/integration/mozilla-inbound/rev/3877a7a5fdf6
https://hg.mozilla.org/integration/mozilla-inbound/rev/160c727a336b
https://hg.mozilla.org/integration/mozilla-inbound/rev/269b0c198fdb
https://hg.mozilla.org/integration/mozilla-inbound/rev/200efa29960e
https://hg.mozilla.org/integration/mozilla-inbound/rev/18289b3b138e
https://hg.mozilla.org/integration/mozilla-inbound/rev/fa47cd60942c
https://hg.mozilla.org/integration/mozilla-inbound/rev/80ca371b52d5
https://hg.mozilla.org/integration/mozilla-inbound/rev/1dfb85987ee0

Comment 36

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2c74293986cf
https://hg.mozilla.org/mozilla-central/rev/24e3a92d51d4
https://hg.mozilla.org/mozilla-central/rev/28b5c8d25fb9
https://hg.mozilla.org/mozilla-central/rev/1a0cd3aa1864
https://hg.mozilla.org/mozilla-central/rev/3877a7a5fdf6
https://hg.mozilla.org/mozilla-central/rev/160c727a336b
https://hg.mozilla.org/mozilla-central/rev/269b0c198fdb
https://hg.mozilla.org/mozilla-central/rev/200efa29960e
https://hg.mozilla.org/mozilla-central/rev/18289b3b138e
https://hg.mozilla.org/mozilla-central/rev/fa47cd60942c
https://hg.mozilla.org/mozilla-central/rev/80ca371b52d5
https://hg.mozilla.org/mozilla-central/rev/1dfb85987ee0

Comment 37

[Ginn Chen]

Got this error while compiling with Solaris Studio.

"/export/home/moz/tools/ff_slave/mozilla-central-solaris-x86/build/storage/src/Variant.h", line 318: Error: Cannot define a reference or pointer to a reference.
"/export/home/moz/tools/ff_slave/mozilla-central-solaris-x86/build/storage/src/mozStorageRow.cpp", line 46:     Where: While specializing "mozilla::storage::Variant<nsString>".
"/export/home/moz/tools/ff_slave/mozilla-central-solaris-x86/build/storage/src/mozStorageRow.cpp", line 46:     Where: Specialized in non-template code.

Comment 38

[Justin Lebar (not reading bugmail)]

Could you please file a follow-up bug?  I'm happy to review a patch fixing the error.

Comment 39

[Philip Chee]

I'm seeing similar errors with MSVC2008SP1 :
cc:\t1\hg\comm-central\mozilla\storage\src\Variant.h(318) : warning C4181: qualifier applied to reference type; ignored
:\t1\hg\comm-central\mozilla\storage\src\Variant.h(318) : warning C4181: qualifier applied to reference type; ignored
         c:/t1/hg/comm-central/mozilla/storage/src/mozStorageRow.cpp(46) : see reference to class template instantiation 'mozilla::storage::Variant<DataType>' being compiled
       c:/t1/hg/comm-central/mozilla/storage/src/mozStoragePrivateHelpers.cpp(128) : see reference to class template instantiation 'mozilla::storage::Variant<DataType>' being compiled
         with
       with
         [
       [
             DataType=nsString
           DataType=nsString
         ]
       ]
cc:\t1\hg\comm-central\mozilla\storage\src\Variant.h(318) : warning C4181: qualifier applied to reference type; ignored
:\t1\hg\comm-central\mozilla\storage\src\Variant.h(318) : warning C4181: qualifier applied to reference type; ignored
cc:\t1\hg\comm-central\mozilla\storage\src\Variant.h(318) : error C2529: 'aData' : reference to reference is illegal
:\t1\hg\comm-central\mozilla\storage\src\Variant.h(318) : error C2529: 'aData' : reference to reference is illegal
cc:/t1/hg/comm-central/mozilla/storage/src/mozStoragePrivateHelpers.cpp(128) : error C2664: 'mozilla::storage::Variant<DataType>::Variant(mozilla::storage::variant_storage_traits<nsString>::ConstructorType (&))' : cannot convert parameter 1 from 'nsDependentJSString' to 'mozilla::storage::variant_storage_traits<nsString>::ConstructorType (&)'
:/t1/hg/comm-central/mozilla/storage/src/mozStorageRow.cpp(46) : error C2664: 'mozilla::storage::Variant<DataType>::Variant mozilla::storage::variant_storage_traits<nsString>::ConstructorType (&))' :cannot convert parameter 1 from 'nsDependentString' to 'mozilla::storage::variant_storage_traits<nsString>::ConstructorType (&)'
         with
       with
         [
       [
             DataType=nsString
           DataType=nsString
         ]
       ]

etc etc.

Comment 40

[[e-mail]]

Comment on attachment 693448 [details] [diff] [review]
Part 3: Make nsTArray == InfallibleTArray and nsAutoTArray == AutoInfallibleTArray, and switch files to using nsTArrayForwardDeclare.h.

>-// specializations for N = 0. this makes the inheritance model easier for
>-// templated users of nsAutoTArray.
Huh, with this misleading comment it's no wonder you accidentally removed these. (They were restored in bug 852393.)