853548 - Make nsTArray::SetCapacity return void

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[(no longer active)]

Currently it returns bool, so if you have code which calls SetCapacity and tries to deal with false return values but say nsTArray when you really mean FallibleTArray, we don't catch that at compile time, and then we end up with bugs like bug 852838.

I have a patch to fix this, and catch other similar cases in the tree.

Comment 1

[(no longer active)]

Attachment: Patch (v1)

Comment 2

[Justin Lebar (not reading bugmail)]

We have code in nsTArray to allow const nsTArray& and const nsFallibleTArray& to be cast between each other.

I suspect this will mess that up very badly.  I don't think we can have both.

Comment 3

[(no longer active)]

(In reply to comment #2)
> We have code in nsTArray to allow const nsTArray& and const nsFallibleTArray&
> to be cast between each other.
> 
> I suspect this will mess that up very badly.  I don't think we can have both.

Why do we want to allow such a cast?

Comment 4

[Justin Lebar (not reading bugmail)]

Because we want to be able to pass "anything" to a method which takes const nsTArray&.  We do this in a few places in our code (I suppose you could find them by removing the cast operators).

Comment 5

[(no longer active)]

(In reply to comment #4)
> Because we want to be able to pass "anything" to a method which takes const
> nsTArray&.  We do this in a few places in our code (I suppose you could find
> them by removing the cast operators).

Hmm, I see.  But then again, if the cast operator only casts between const references, SetCapacity cannot be called on those casted objects, right?

Comment 6

[Boris Zbarsky [:bzbarsky]]

The cast operator definitely only works on const references, and the assumption is that callees do not cast away const and try to mutate the array...

Justin, what cases do you think would break with this change?

Comment 7

[Justin Lebar (not reading bugmail)]

> SetCapacity cannot be called on those casted objects, right?

Oh, duh.  Sorry.

If we only change non-const methods in this way, it should be fine.

Comment 8

[(no longer active)]

Yeah, I would expect only non-const methods attempt to allocate/reallocate.  If not, then we should mark those methods as non-const.

BTW, this is a bit of black magic C++.  I have added some comments to the darkest trick that I used here, the rest is not that complicated.  Fortunately this infrastructure can later be used to do the same thing for other nsTArray methods which are really infallible and always return true.

(And so far, it has found another footgun: bug 853554.)

Justin, please let me know if you think I can add more comments to make this easier to understand.

Comment 9

[Justin Lebar (not reading bugmail)]

Comment on attachment 727808 [details] [diff] [review]
Patch (v1)

This looks good to me, with my |const| misunderstanding out of the way.  Just
some nits.

>@@ -92,44 +92,44 @@ bool nsTArray_base<Alloc>::UsesAutoArrayBuffer() const {
>   NS_ABORT_IF_FALSE(diff >= 0 && diff <= 4, "GetAutoArrayBuffer doesn't do what we expect.");
> #endif
> 
>   return mHdr == GetAutoArrayBuffer(4) || mHdr == GetAutoArrayBuffer(8);
> }
> 
> 
> template<class Alloc>
>-bool
>+typename Alloc::SuccessResultProxy
> nsTArray_base<Alloc>::EnsureCapacity(size_type capacity, size_type elemSize) {

Ideally we want the fallible TArray functions to emit a compile warning if
their return results are unused.  It's not clear to me how to do so without
abusing macros; do you have an idea for accomplishing that?  We don't have to
do it here, of course.

>diff --git a/xpcom/glue/nsTArray.h b/xpcom/glue/nsTArray.h
>index 1f20c31..527483c 100644
>--- a/xpcom/glue/nsTArray.h
>+++ b/xpcom/glue/nsTArray.h

>+// nsTArrayFallibleSuccessResult and nsTArrayInfallibleSuccess results are
>+// proxy types which are used because you cannot use a templated type which
>+// is bound to void as an argument to a void function.  In order to work
>+// around that, we encode either a void or a boolean inside these proxy
>+// objects, and pass them to the aforementioned function instead, and then
>+// use the type information to decide what to do in the function.

So I should return AllocSuccessProxy from internal TArray methods, and I should
return AllocSuccess from public methods?  And I can't call methods returning
AllocSuccess from within nsTArray (i.e., public methods must be strictly
public)?  If that's right, perhaps you could write that somewhere.

>@@ -93,16 +120,35 @@ struct nsTArrayFallibleAllocator
>   }
> 
>   static void Free(void* ptr) {
>     moz_free(ptr);
>   }
> 
>   static void SizeTooBig() {
>   }
>+
>+  typedef bool SuccessResult;
>+  typedef nsTArrayFallibleSuccessResult SuccessResultProxy;

I don't think SuccessResult is a great name, because it kind of sounds like
methods which return SuccessResult never fail, but I don't have a better
suggestion.  I dislike SucceededResult about as much, and it's longer.

>@@ -112,16 +158,34 @@ struct nsTArrayInfallibleAllocator
>+  static SuccessResultProxy Failure() {
>+    return SuccessResultProxy();
>+  }
> };

Can we assert (at least at debug time) that this isn't called?  I'm tempted to
say we should assert in release builds too.

>@@ -161,16 +244,34 @@ struct nsTArrayInfallibleAllocator
>+  typedef void SuccessResult;
>+  typedef nsTArrayInfallibleSuccessResult SuccessResultProxy;
>+
>+  static SuccessResult Result(SuccessResultProxy result) {
>+  }
>+
>+  static bool Successful(SuccessResultProxy) {
>+    return true;
>+  }
>+
>+  static SuccessResultProxy Success() {
>+    return SuccessResultProxy();
>+  }
>+
>+  static SuccessResultProxy Failure() {
>+    return SuccessResultProxy();
>+  }

I'm not a huge fan of copy-pasting this (as opposed to, say, using
inheritance), but you should do whatever you think is clearer; I don't want to
over-complexify the code for the sake of avoiding some copy-paste.

Comment 10

[(no longer active)]

(In reply to Justin Lebar [:jlebar] from comment #9)
> Ideally we want the fallible TArray functions to emit a compile warning if
> their return results are unused.  It's not clear to me how to do so without
> abusing macros; do you have an idea for accomplishing that?  We don't have to
> do it here, of course.

Hmm, no I can't think of any elegant solutions.

> >diff --git a/xpcom/glue/nsTArray.h b/xpcom/glue/nsTArray.h
> >index 1f20c31..527483c 100644
> >--- a/xpcom/glue/nsTArray.h
> >+++ b/xpcom/glue/nsTArray.h
> 
> >+// nsTArrayFallibleSuccessResult and nsTArrayInfallibleSuccess results are
> >+// proxy types which are used because you cannot use a templated type which
> >+// is bound to void as an argument to a void function.  In order to work
> >+// around that, we encode either a void or a boolean inside these proxy
> >+// objects, and pass them to the aforementioned function instead, and then
> >+// use the type information to decide what to do in the function.
> 
> So I should return AllocSuccessProxy from internal TArray methods, and I
> should
> return AllocSuccess from public methods?  And I can't call methods returning
> AllocSuccess from within nsTArray (i.e., public methods must be strictly
> public)?  If that's right, perhaps you could write that somewhere.

Yes, I'll document that.

> >@@ -93,16 +120,35 @@ struct nsTArrayFallibleAllocator
> >   }
> > 
> >   static void Free(void* ptr) {
> >     moz_free(ptr);
> >   }
> > 
> >   static void SizeTooBig() {
> >   }
> >+
> >+  typedef bool SuccessResult;
> >+  typedef nsTArrayFallibleSuccessResult SuccessResultProxy;
> 
> I don't think SuccessResult is a great name, because it kind of sounds like
> methods which return SuccessResult never fail, but I don't have a better
> suggestion.  I dislike SucceededResult about as much, and it's longer.

OK I used ResultType and ResultTypeProxy.

> >@@ -112,16 +158,34 @@ struct nsTArrayInfallibleAllocator
> >+  static SuccessResultProxy Failure() {
> >+    return SuccessResultProxy();
> >+  }
> > };
> 
> Can we assert (at least at debug time) that this isn't called?  I'm tempted
> to
> say we should assert in release builds too.

OK.

> >@@ -161,16 +244,34 @@ struct nsTArrayInfallibleAllocator
> >+  typedef void SuccessResult;
> >+  typedef nsTArrayInfallibleSuccessResult SuccessResultProxy;
> >+
> >+  static SuccessResult Result(SuccessResultProxy result) {
> >+  }
> >+
> >+  static bool Successful(SuccessResultProxy) {
> >+    return true;
> >+  }
> >+
> >+  static SuccessResultProxy Success() {
> >+    return SuccessResultProxy();
> >+  }
> >+
> >+  static SuccessResultProxy Failure() {
> >+    return SuccessResultProxy();
> >+  }
> 
> I'm not a huge fan of copy-pasting this (as opposed to, say, using
> inheritance), but you should do whatever you think is clearer; I don't want
> to
> over-complexify the code for the sake of avoiding some copy-paste.

I'll use inheritance, it should not be too bad.

Comment 11

[(no longer active)]

Attachment: Patch (v2)

Comment 12

[(no longer active)]

Turns out that on Linux, X11/X.h #defines Success, so I will rename Success/Failure to SuccessProxy/FailureProxy to make this compile. :(

Comment 13

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=549962bee6fb
https://tbpl.mozilla.org/?tree=Try&rev=589e3b3173b4
https://tbpl.mozilla.org/?tree=Try&rev=dc77c41673bc
https://tbpl.mozilla.org/?tree=Try&rev=7e2f8a9b30d1

Comment 14

[(no longer active)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc0a76b193e3

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/cc0a76b193e3

Comment 16

[(no longer active)]

Talking to Benoit about this today, he said that there might have been some simpler C++ tricks that I could have used here.  Needinfo'ing him to ask him to take a look at the patch.  (Please look at how I have this pattern |return Alloc::SuccessResult();| instead of just |return true;| and also how I have checks such as |if (Alloc::Successful(foo()))| to do a check for the fallible case and do a |if (true)| for the infallible case.

Comment 17

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

Indeed, two things:


 1. You can use MFBT's mozilla::Conditional to define the return type:

   typename mozilla::Conditional<IsFallible, bool, void>::Type

  (as usual, 'typename' is needed to get at member typedefs in types depending on template parameters).


 2. You then don't need to wrap the return value in any kind of proxy class, you can simply return true or false cased to the type that you obtained at step 1.


Putting this together, here is a toy nsTArray::SetCapacity():

template<typename T, bool IsFallible>
typename mozilla::Conditional<IsFallible, bool, void>::Type
nsTArray<T, IsFallible>::SetCapacity(size_t newCapacity)
{
  bool success = do_work(newCapacity);
  return typename mozilla::Conditional<IsFallible, bool, void>::Type(success);
}

You could evaluate once and for all that awkward Conditional expression in a member typedef in nsTArray.

Comment 18

[(no longer active)]

You are a true C++ guru!  Filed bug 969847 for that.