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

RESOLVED FIXED in mozilla20

Status

()

defect
RESOLVED FIXED
7 years ago
5 years ago

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

Tracking

Trunk
mozilla20
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(12 attachments, 1 obsolete attachment)

801 bytes, patch
cjones
: review+
Details | Diff | Splinter Review
6.46 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
2.10 KB, patch
cjones
: review+
Details | Diff | Splinter Review
32.75 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
1.00 KB, patch
mounir
: review+
Details | Diff | Splinter Review
1.08 KB, patch
mounir
: review+
Details | Diff | Splinter Review
2.28 KB, patch
bent.mozilla
: review+
Details | Diff | Splinter Review
5.79 KB, patch
qdot
: review+
Details | Diff | Splinter Review
968 bytes, patch
roc
: review+
Details | Diff | Splinter Review
1.95 KB, patch
cjones
: review+
Details | Diff | Splinter Review
5.57 KB, patch
mak
: review+
Details | Diff | Splinter Review
3.09 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
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<[^,>]+,[^>]+'
(Assignee)

Updated

7 years ago
Assignee: nobody → justin.lebar+bug

Comment 1

7 years ago
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.
(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.
I like I as well.

If we do that, can we still typedef nsTArray to InfallibleTArray if desired?
> 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.
Sigh, C++.  OK.
(Assignee)

Updated

6 years ago
Depends on: 819523
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
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.
Attachment #693444 - Flags: review?(jones.chris.g)
The allocators will be restricted to non-public use in a later patch.
Attachment #693446 - Flags: review?(jones.chris.g)
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);
Attachment #693456 - Flags: review?(jones.chris.g)
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.
Summary: Fix the nsTArray / InfallibleTArray / FallibleTArray type madness → Fix the nsTArray / InfallibleTArray / FallibleTArray type madness, and make InfallibleTArray and FallibleTArray's copy constructors explicit
Attachment #693449 - Flags: review?(bzbarsky) → review+
Attachment #693450 - Flags: review?(mounir) → review+
Attachment #693453 - Flags: review?(kyle) → review+
Attachment #693444 - Flags: review?(jones.chris.g) → review+
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
Attachment #693445 - Flags: review?(bzbarsky) → review+
>+    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 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.
Attachment #693448 - Flags: review?(bzbarsky) → review+
Attachment #693446 - Flags: review?(jones.chris.g) → review+
Comment on attachment 693456 [details] [diff] [review]
Part 9: Use explicit TArray copy constructors in IPDL generated code.

># HG changeset patch
># User Justin Lebar <justin.lebar@gmail.com>
>
>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 ]
Attachment #693456 - Flags: review?(jones.chris.g) → review+
Comment on attachment 693458 [details] [diff] [review]
Part 11: Make nsTArray and friends' copy constructors explicit.

r=me
Attachment #693458 - Flags: review?(bzbarsky) → review+
Leaving the fputs is fine, with the better message.
Comment on attachment 693452 [details] [diff] [review]
Part 6: Remove unnecessary TArray copies in dom/indexedDB.

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

Oops!
Attachment #693452 - Flags: review?(bent.mozilla) → review+
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?
Attachment #693457 - Flags: review?(mak77) → review+
Wow, I feel like Christmas came early -- thanks for the fast reviews, everyone!

Comment 37

6 years ago
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.
Could you please file a follow-up bug?  I'm happy to review a patch fixing the error.

Comment 39

6 years ago
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.
(Assignee)

Updated

6 years ago
Depends on: 823553
Depends on: 824817
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.)
You need to log in before you can comment on or make changes to this bug.