968520 - Require use of mozilla::fallible with fallible FallibleTArray calls

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[(no longer active)]

See the discussion in bug 967167.

Comment 1

[Scott West]

I've contacted Benjamin out of band (by email) and he got me started on this bug. I have a first draft ready now that compiles (and even runs).

Comment 2

[Scott West]

Attachment: bug-968520.patch

I'm concerned mostly about a couple of things:

1) that the clients were updated correctly, some usages its not clear to me which is the proper variant to use (as the usage sites were the same before). I'm not sure how one would test for this, as one would have to trigger out-of-memory situations in somewhat arbitrary places, but I'd be happy to find out that its not so difficult to do!

2) if the patch should be broken up into more logical pieces that are easier to debug. I can't immediately think of any split other than adding the functions to nsTArray separately from removing InfallibleTArray, but this doesn't seem like a huge benefit.

Comment 3

[Benjamin Smedberg]

Comment on attachment 8377802 [details] [diff] [review]
bug-968520.patch

I'll have a look at this patch tomorrow. My only suggestion would be to see if you can break it up into reviewable pieces a bit more. So I'd review the core nsTArray changes and any mechanical typedef fixups. But for client code which you switched to use fallible_t those should maybe be separate patches. For example:

nsLineBreaker.cpp and the nsHyphenator.h/cpp changes should be together.

Comment 4

[(no longer active)]

A comment on the general approach of the patch.  If there is code like:

void ManipulateArray(nsTArray<Foo>& array) {
  array.AppendElement(...);
}

void f() {
  FallibleTArray<Foo> arr;
  ManipulateArray(array);
}

Then you need to pass fallible_t() to AppendElement in ManipulateArray when converting the above to nsTArray.

Now, if in addition to the above, you also have:

void g() {
  nsTArray<Foo> arr;
  ManipulateArray(array);
}

Then you need to make ManipulateArray aware of whether it should do fallible memory allocation or not.  This means that reviewing this patch will need a careful auditing of all of the places where FallibleTArray's can flow into in the code base.  Honestly I think that's impractical to do with a monolithic patch, which is why perhaps we should consider breaking this down into very small bits (perhaps in individual bugs?)

Comment 5

[Scott West]

I was also unhappy with how big the changes were, and with no real automated checking besides some instances of elem_type* turning to void (and thus failing to compile if the return value was checked).

I was thinking that it may good to break it down not by which files change, but by what aspects of the API we want to break:

1) remove the implicit conversions between FallibleTArray and nsTArray (to find cases like Ehsan indicated)
2) have FallibleTArray privately inherit from nsTArray and reexport the interface, where nsTArray removes `failing' return values where appropriate (nsTArray clients will have to consider if they're used as the fallible or infallible version)
3) add dummy fallible_t parameters to the FallibleTArray class (so that FallibleTArray clients update their usages appropriately)
4) finally, merge the `fallible_t' methods into nsTArray, remove the Alloc template paramter, and delete FallibleTArray (hopefully this just means renaming FallibleTArray declarations into nsTArray)

I think not trying to change all of the API at the same time will make it easier to spot problems.

Comment 6

[(no longer active)]

I think that is a good plan.  Note that with that plan you still need to switch all FallibleTArray's wholesale.  If you think that is not practical, then one approach would be to add a new temporary type implementing the new interface and move FallibleTArray's to that type one by one, and finalluy move all of the instances of that type to nsTArray when the fallible_t interface is implemented in nsTArray in one go.

But please double check any plans here with Benjamin before spending time to work on it.  Thanks!

Comment 7

[Benjamin Smedberg]

I think comment 5 is a good plan, but I'm not sure why that requires switching all FallibleTArrays at the same time. It should be possible to switch individual fallible arrays to nsTArray one API-surface at a time, no?

Also for the case in comment 4, the solution I would recommend is making ManipulateArray always fallible and have the client do checking. But module owners make the final call there.

Comment 8

[Scott West]

Benjamin, you're right, this can probably be done piecemeal.

What is the preferred size the patches? I.e., for the `conversion-less API', would it be proper to have all of the content/svg/ modifications in one patch?

Comment 9

[Benjamin Smedberg]

That's probably fine. Basically we're trying to divide it up by who needs to review it and not make patches so large that they can't be reviewed.

Comment 10

[(no longer active)]

(In reply to comment #7)
> I think comment 5 is a good plan, but I'm not sure why that requires switching
> all FallibleTArrays at the same time. It should be possible to switch
> individual fallible arrays to nsTArray one API-surface at a time, no?

I was assuming that we don't want to end up with two different semantics one based on the argument at at the allocation site and one based on the type in the tree at the same time.  Is that not the case?

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to Scott West from comment #5)
> I was also unhappy with how big the changes were, and with no real automated
> checking besides some instances of elem_type* turning to void (and thus
> failing to compile if the return value was checked).
> 
> I was thinking that it may good to break it down not by which files change,
> but by what aspects of the API we want to break:
> 
> 1) remove the implicit conversions between FallibleTArray and nsTArray (to
> find cases like Ehsan indicated)
> 2) have FallibleTArray privately inherit from nsTArray and reexport the
> interface, where nsTArray removes `failing' return values where appropriate
> (nsTArray clients will have to consider if they're used as the fallible or
> infallible version)
> 3) add dummy fallible_t parameters to the FallibleTArray class (so that
> FallibleTArray clients update their usages appropriately)

an alternative and possibly easier way to get to this state is to start by adding real fallible and non fallible version to nsTArray_impl and just reinterpret cast your way to that working with the allocator template arg.  Then you can update all of the usage piecemeal.


(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #10)
> (In reply to comment #7)
> > I think comment 5 is a good plan, but I'm not sure why that requires switching
> > all FallibleTArrays at the same time. It should be possible to switch
> > individual fallible arrays to nsTArray one API-surface at a time, no?
> 
> I was assuming that we don't want to end up with two different semantics one
> based on the argument at at the allocation site and one based on the type in
> the tree at the same time.  Is that not the case?

the semantics would be its determined by the type only if not by the call site which is only sort of both.  Any way I don't think its a big deal if the type based stuff is going away soon.

Comment 12

[Scott West]

(In reply to Trevor Saunders (:tbsaunde) from comment #11)
> (In reply to Scott West from comment #5)
> > I was also unhappy with how big the changes were, and with no real automated
> > checking besides some instances of elem_type* turning to void (and thus
> > failing to compile if the return value was checked).
> > 
> > I was thinking that it may good to break it down not by which files change,
> > but by what aspects of the API we want to break:
> > 
> > 1) remove the implicit conversions between FallibleTArray and nsTArray (to
> > find cases like Ehsan indicated)
> > 2) have FallibleTArray privately inherit from nsTArray and reexport the
> > interface, where nsTArray removes `failing' return values where appropriate
> > (nsTArray clients will have to consider if they're used as the fallible or
> > infallible version)
> > 3) add dummy fallible_t parameters to the FallibleTArray class (so that
> > FallibleTArray clients update their usages appropriately)
> 
> an alternative and possibly easier way to get to this state is to start by
> adding real fallible and non fallible version to nsTArray_impl and just
> reinterpret cast your way to that working with the allocator template arg. 
> Then you can update all of the usage piecemeal.
> 

Hello Trevor, thank you for your suggestion.

However, I think breaking up the process along distinct API changes may help me focus on particular types of changes. One of the changes is adding the new methods, another is that we need to be sure that we're getting the same behaviours as before, now that we no longer have different types to keep that sorted. That's what I'm hoping this style of process will help ensure, both for myself being a newcomer and for the reviewer(s) to check.

Comment 13

[Scott West]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #4)
> A comment on the general approach of the patch.  If there is code like:
> 
> void ManipulateArray(nsTArray<Foo>& array) {
>   array.AppendElement(...);
> }
> 
> void f() {
>   FallibleTArray<Foo> arr;
>   ManipulateArray(array);
> }
> 
> Then you need to pass fallible_t() to AppendElement in ManipulateArray when
> converting the above to nsTArray.
> 
> Now, if in addition to the above, you also have:
> 
> void g() {
>   nsTArray<Foo> arr;
>   ManipulateArray(array);
> }
> 

I just started on my first phase (removing the conversions) and it doesn't seem like the situation you mention can arise: the operator() definitions from nsTArray_Impl return const (thus the difference in behaviour cannot be seen, because no allocations can happen on the result).

Can you think of any other situation where one can convert between nsTArray and FallibleTArray? It seems that the API is pretty carefully constructed in this respect.

If not, then I can switch directly to the next API change (FallibleTArray must use fallible_t).

Comment 14

[(no longer active)]

(In reply to comment #13)
> Can you think of any other situation where one can convert between nsTArray and
> FallibleTArray? It seems that the API is pretty carefully constructed in this
> respect.

Yes, see the ReadTArray function in this patch for example: <https://hg.mozilla.org/mozilla-central/rev/f5be8660f07b>

If you go to the previous changeset, the ReadTArray there accepts an nsTArray_impl, and there are callers that pass both nsTArray and FallibleTArray to it.  The other way this can potentially happen is template functions accepting things with the nsTArray interface.

Another way that these array types can interact are functions such as SwapElemets().

One effective way of catching the former category is removing the implicit conversion.  I'm not sure about the other possibilities.

Comment 15

[Scott West]

(In reply to :Ehsan Akhgari (needinfo? me!) (slow responsiveness, emailapocalypse) from comment #14)
> 
> Yes, see the ReadTArray function in this patch for example:
> <https://hg.mozilla.org/mozilla-central/rev/f5be8660f07b>
> 
> If you go to the previous changeset, the ReadTArray there accepts an
> nsTArray_impl, and there are callers that pass both nsTArray and
> FallibleTArray to it.  The other way this can potentially happen is template
> functions accepting things with the nsTArray interface.
> 
> Another way that these array types can interact are functions such as
> SwapElemets().

Another good example, thanks. 

> One effective way of catching the former category is removing the implicit
> conversion.  I'm not sure about the other possibilities.

I can see how it goes with removing the conversion (then probably the inheritance to weasel more cases out).

Perhaps a late question but: it seems like the current setup with Fallible/nsTArray is really carrying around important behaviour-changing information. Now we're going back and taking the information away, and trying to recreate it by being very careful clients of nsTArray, adding fallible_t where we remember. So assuming there's a successful consolidation of the two classes, what is the end benefit?

Comment 16

[Benjamin Smedberg]

The desired result is that when you are reading the code, you can immediately see whether a particular call is fallible and make sure that the error handling around that callsite is correct. It's not good to hide that information in the declaration, which in the case of c++ is often in a different file entirely.

Comment 17

[Scott West]

Benjamin, thank you for the clarification. So currently, the type documents the intention, but it's not clear at the call site which behaviour will occur. With the proposed change, the type will say nothing of the intention of the object, but it will be very clear at the call site what failure behaviour is desired.

It seems that, from the existing code-base, that only one behaviour from a given type is really wanted (i.e., you don't want to mix Fallible and Infallible usages on a single object), but the new consolidation would open the door to this.

Would it be desired to keep both the type-level and the call-site-level distinctions?

Comment 18

[Benjamin Smedberg]

I don't think the type-level distinction is especially valuable, no.

Comment 19

[(no longer active)]

(In reply to comment #17)
> It seems that, from the existing code-base, that only one behaviour from a
> given type is really wanted (i.e., you don't want to mix Fallible and
> Infallible usages on a single object), but the new consolidation would open the
> door to this.

Also note that it's possible that you may want fallible allocation on the same array object in one place but infallible allocation everywhere else, which is currently not possible because the type dictates the allocation strategy.

Comment 20

[Scott West]

Benjamin, Ehsan, thank you for clearing that up; I'll take a pass at the first stage now (finding usages of Fallible through upcasting).

Comment 21

[Scott West]

So I have the first part done, which I suppose (hope :)) will be the hard part, that takes away the inheritance relationship between FallibleTArray and nsTArray_Impl. I may have gone a little far in breaking the patches down, and have about 18 of them, ranging in size from 5 line changes to 500 (this is for the main changes to nsTArray.h to replicate the nsTArray_Impl interface for the Fallible array replacement).

I'm not sure the best size of patch: should I try to combine some of the patches, or just post them as-is?

Comment 22

[Benjamin Smedberg]

Scott, feel free to post them as is, and if I want them combined I'll let you know.

Comment 23

[Scott West]

Attachment: bug-968520-1-1.patch

Introduces a FallibleTArray variant that privately inherits from nsTArray_Impl. It only allows implicit casting to const nsTArray, and re-exports all other methods. It also augments nsTArray_Impl so it SwapElements can work with the new FallibleTArray variant.

Comment 24

[Scott West]

Attachment: bug-968520-1-2.patch

Updating content/svg for the first phase.

Comment 25

[Scott West]

Attachment: bug-968520-1-3.patch

Updating dom/smil for this first phase.

Comment 26

[Scott West]

Attachment: bug-968520-1-4.patch

Updating content/canvas for this first phase.

Comment 27

[Scott West]

Attachment: bug-968520-1-5.patch

Updating gfx/thebes for this first phase.

Comment 28

[Scott West]

Attachment: bug-968520-1-6.patch

Updating content/media for this first phase.

Comment 29

[Scott West]

Attachment: bug-968520-1-7.patch

Updating dom/network for this first phase.

Comment 30

[Scott West]

Attachment: bug-968520-1-8.patch

Updating dom/base for this first phase.

Comment 31

[Scott West]

Attachment: bug-968520-1-9.patch

Updating layout for this first phase.

Comment 32

[Scott West]

Attachment: bug-968520-1-10.patch

Updating network/base for this first phase.

Comment 33

[Scott West]

Attachment: bug-968520-1-11.patch

Updating intl for this first phase.

Comment 34

[Scott West]

Attachment: bug-968520-1-12.patch

Updating dom/plugins/ipc for this first phase.

Comment 35

[Scott West]

Attachment: bug-968520-1-13.patch

Updating dom/bindings for this first phase.

Comment 36

[Scott West]

Attachment: bug-968520-1-14.patch

Updating xpcom/io for this first phase.

Comment 37

[Scott West]

Attachment: bug-968520-1-15.patch

Updating toolkit/components/url-classifier for this first phase.

Comment 38

[Scott West]

Attachment: bug-968520-1-16.patch

Updating ipc/glue for this first phase.

Comment 39

[Scott West]

Attachment: bug-968520-1-17.patch

Updating misc in gfx/2d, image/src, storage/src, and xpcom/base for this first phase.

Comment 40

[Scott West]

Attachment: bug-968520-1-18.patch

Entirely removes the original FallibleTArray. Also removes a few intermediate conversion routines as well as updates the ipdl generator to use the new APIs (just the new FallibleTArray_1 type and the modified SwapElements_1 type).

Comment 41

[Scott West]

Sorry for the large number of patches. As a reminder: the purpose of this patch set is to remove the (visible) inheritance relationship of FallibleTArray with nsTArray_Impl. This was to draw out all the places where this relationship was being used, and polymorphic behaviour of allocation methods on nsTArray_Impl may have been used.

The next phase will be to change the signatures in FallibleTArray_1 to require the fallible_t struct when allocation may occur, ensuring that when these methods are merged into nsTArray that all the clients will be using the appropriate calls.

Comment 42

[Scott West]

Attachment: bug-968520-1-16.patch

Updating ipc/glue for this first phase.

This fixes the previous version; somehow it didn't even compile on a fresh build, though I certainly compiled everything many times before submitting...

Comment 43

[Benjamin Smedberg]

I lost track of this bug. Feel free to set NEEDINFO on me when you get to a point you need feedback! I'll go through the patches and mark reasonable reviewers on them shortly.

Comment 44

[Scott West]

Well, I'm not sure what the protocol would be here :) These are not "complete" patches that would fix the bug, just the first stage where I try to wean FallibleTArray away from nsTArray_Impl so the points where they really overlap (not just coincidentally by API). The next stages should (I hope) be fairly easy, as they should be mostly adding fallible_t arguments to the appropriate places and then merging the two classes back again. 

All of that just to say: I'm not sure how the review process should work here, whether you want all these stages together or one stage at a time. Each stage will probably touch quite a few files, but most of the changes should be trivial (renaming, adding fallible_t, etc).

Comment 45

[Nathan Froyd [:froydnj]]

Scott, are you still interested in working on this?

Comment 46

[Scott West]

Interested yes, but not enough time to push it through entirely. Please feel free to assign it to someone with both interest and time :).

Comment 47

[Nathan Froyd [:froydnj]]

Birunthan, would you be interested in tackling this?

Comment 48

[Biru [:poiru]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #47)
> Birunthan, would you be interested in tackling this?

Yep.

Comment 49

[Biru [:poiru]]

Attachment: Explicitly specify the allocator for nsTArray_base functions

Plan outlined here: https://gist.github.com/poiru/7f67ee156b4df3ee0f2e

Apologies for the delay!

Comment 50

[Biru [:poiru]]

Attachment: Explicitly specify the allocator for nsTArray_base functions

Comment 51

[Biru [:poiru]]

Attachment: Add fallible variant of nsTArray::SetCapacity

Comment 52

[Biru [:poiru]]

Attachment: Replace FallibleTArray with fallible nsTArray methods in ns{Unichar,Converter}InputStream

Comment 53

[Biru [:poiru]]

Attachment: Add fallible variant of nsTArray::SetCapacity

Comment 54

[Biru [:poiru]]

Attachment: Add mozilla::fallible to Fallible{Auto,}TArray::SetCapacity calls

I used the following Clang AST matcher to find these:

  memberCallExpr(
    callee(
      methodDecl(
        maybeInfallibleMethod(),
        ofClass(
          classTemplateSpecializationDecl(
            hasAnyTemplateArgument(
              refersToType(asString("struct nsTArrayFallibleAllocator")))))
      )
    )
  )

  where maybeInfallibleMethod() matches functions marked with `__attribute__((annotate("moz_mayble_infallible_method")))`.

I use a slightly hacky approach to automatically add the `fallible` args, but I will make a clang-tidy version and upload it somewhere later.

Comment 55

[Biru [:poiru]]

Attachment: Add fallible variant of nsTArray::InsertElementsAt

Comment 56

[Biru [:poiru]]

Attachment: Add mozilla::fallible to Fallible{Auto,}TArray::InsertElementsAt calls

Comment 57

[Biru [:poiru]]

Attachment: Add fallible variant of nsTArray::EnsureLengthAtLeast

Comment 58

[Biru [:poiru]]

Attachment: Add fallible variant of nsTArray::SetLength

Comment 59

[Biru [:poiru]]

Attachment: Add mozilla::fallible to Fallible{Auto,}TArray::SetLength calls

We should probably add MOZ_WARN_UNUSED_RESULT to e.g. the SetLength function in SVGLengthList.h. I'[l file another bug.

Comment 60

[Biru [:poiru]]

Attachment: Add fallible variant of nsTArray::ReplaceElementsAt

Comment 61

[Biru [:poiru]]

Attachment: Add mozilla::fallible to Fallible{Auto,}TArray::ReplaceElementsAt calls

Comment 62

[Biru [:poiru]]

Attachment: Add fallible variants of nsTArray::InsertElementAt

Comment 63

[Biru [:poiru]]

FallibleTArray has an operator=, but that doesn't play well with error checking. I propose we add Assign functions to cover fallible assignments. After this lands, we will have to avoid operator= when we want fallible allocation, but that is not particularly worse than the status quo where we can't check if an assignment succeeded. What do you think?

Comment 64

[Nathan Froyd [:froydnj]]

Comment on attachment 8606507 [details] [diff] [review]
Explicitly specify the allocator for nsTArray_base functions

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

It's always going to be a good patch when you have to use |.template MethodName| syntax.

Comment 65

[Nathan Froyd [:froydnj]]

Comment on attachment 8606849 [details] [diff] [review]
Add mozilla::fallible to Fallible{Auto,}TArray::SetCapacity calls

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

::: dom/svg/SVGLengthList.cpp
@@ +22,4 @@
>      // Yes, we do want fallible alloc here
>      return NS_ERROR_OUT_OF_MEMORY;
>    }
>    mLengths = rhs.mLengths;

I was kind of hoping we could avoid assignments to fallible arrays, but I guess not...

Comment 66

[Biru [:poiru]]

Attachment: Add fallible variants of nsTArray::AppendElements

Comment 67

[Biru [:poiru]]

Attachment: Add fallible variants of nsTArray::AppendElement

Comment 68

[Nathan Froyd [:froydnj]]

Comment on attachment 8606855 [details] [diff] [review]
Add mozilla::fallible to Fallible{Auto,}TArray::SetLength calls

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

r=me, but see below for a meta-comment on all these patches (apologies for not noticing this a little sooner).

::: dom/canvas/WebGLElementArrayCache.cpp
@@ +375,5 @@
>  
>      // Step #0: If needed, resize our tree data storage.
>      if (requiredNumLeaves != NumLeaves()) {
>          // See class comment for why we the tree storage size is 2 * numLeaves.
> +        if (!mTreeData.SetLength(2 * requiredNumLeaves, mozilla::fallible)) {

I would be willing to bet that 99%+ of the instances of mozilla::fallible that you've added can just be |fallible|, because we are either inside the mozilla:: namespace or we have declared |using namespace mozilla;| earlier.

Would it be possible to either:

a) Rewrite your auto-fallible-adder to detect if |fallible| is in-scope and use that, otherwise use mozilla::fallible; or
b) Use |fallible| always but trial-and-error compile to use |mozilla::fallible| instead?  You could probably automate it with something like:

while true do
  # -j1 is slower, but also avoids botching up the log's error messages.
  if mach build -j1 --log log binaries; then
    break
  fi

  # Something failed to compile because |fallible| was not in-scope.
  extract file:linenumber from error messages
  sed-and-replace s/fallible/mozilla::fallible/
done

Run that overnight, and voila!

Though I see that there are some instances of |fallible| and some instances of |mozilla::fallible| in these patches.  Very curious!  Maybe you just need to modify your heuristic for (not) adding the prefix?

Comment 69

[Biru [:poiru]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #68)
> Though I see that there are some instances of |fallible| and some instances
> of |mozilla::fallible| in these patches.  Very curious!  Maybe you just need
> to modify your heuristic for (not) adding the prefix?

The current heuristic is to just search the file for `using namespace mozilla;`. I'll improve it.

Comment 70

[Nathan Froyd [:froydnj]]

(In reply to Birunthan Mohanathas [:poiru] from comment #63)
> FallibleTArray has an operator=, but that doesn't play well with error
> checking. I propose we add Assign functions to cover fallible assignments.
> After this lands, we will have to avoid operator= when we want fallible
> allocation, but that is not particularly worse than the status quo where we
> can't check if an assignment succeeded. What do you think?

I think that's reasonable.  I'd |= delete| the copying version of |operator=| just to make sure people use Assign() (I'm not sure whether you were proposing to do that in the above).

Comment 71

[Nathan Froyd [:froydnj]]

I'm not so sure comment 18 and comment 19 are a good idea; in particular, with a single nsTArray type, it seems like it'd be too easy to introduce infallible operation(s) on an array when fallible operations were used everywhere else.  Ehsan and Benjamin claimed that this is desirable, but I am not so sure...Ehsan, can you provide a concrete example for comment 19?

Comment 72

[Benjamin Smedberg]

That's the way our string code works now. Any time you want a fallible operation, you have to use the fallible_t symbol. That makes it easy for reviewers to see where fallibility is happening and ensure that we have the proper error correction code. I strongly encourage keeping that pattern.

Comment 73

[Nathan Froyd [:froydnj]]

Are you holding up our string code as the proper way to design an API? :p

I think there are two things tied up in this bug:

1) Making fallibility explicit at call sites.
2) Unifying the type hierarchy of nsTArray.

I think the first one is unambiguously good.

My question is whether the second one is really worth it.  In DOM code, for instance, it seems valuable to explicitly say that certain things are *always* going to require fallible operations.  If we duplicate information in the type declaration and the code that uses the variable/member/etc., that doesn't seem like such a bad thing.

Comment 74

[Biru [:poiru]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #73)
> My question is whether the second one is really worth it.  In DOM code, for
> instance, it seems valuable to explicitly say that certain things are
> *always* going to require fallible operations.

I think we should be able to have our cake and eat it, too. Here is a simplified example of what I think we should aim for:

  struct fallible_t {};
  const fallible_t fallible = {};

  template<typename T, bool Fallible = false>
  class Array
  {
  public:
    void Allocate()
    {
      static_assert(!Fallible,
                    "Infallible allocation can only be used with infallible arrays");
    }

    bool Allocate(const fallible_t&)
    {
      return true;
    }
  };

  template<typename T>
  using FallibleArray = Array<T, true>;

  int main()
  {
    Array<int> a;
    a.Allocate();
    a.Allocate(fallible);

    FallibleArray<int> fa;
    fa.Allocate(); // This triggers the static_assert.
    fa.Allocate(fallible);
  }

So basically we keep FallibleTArray around, but get rid of the type-level allocation strategy. This means that `mozilla::fallible` must be included when calling any allocating FallibleTArray function. This approach should also allow us to flatten the type hierarchy. What do you think?

Comment 75

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84dcea35596f
https://hg.mozilla.org/integration/mozilla-inbound/rev/7464e2ba391c
https://hg.mozilla.org/integration/mozilla-inbound/rev/b589dae2c293
https://hg.mozilla.org/integration/mozilla-inbound/rev/808811474fe7
https://hg.mozilla.org/integration/mozilla-inbound/rev/e2260ff7078a
https://hg.mozilla.org/integration/mozilla-inbound/rev/69ad6f8f8266
https://hg.mozilla.org/integration/mozilla-inbound/rev/e1cd799bafd4
https://hg.mozilla.org/integration/mozilla-inbound/rev/5bf8a0b4add0
https://hg.mozilla.org/integration/mozilla-inbound/rev/8e8c7edadef6
https://hg.mozilla.org/integration/mozilla-inbound/rev/1141051601ab
https://hg.mozilla.org/integration/mozilla-inbound/rev/733b193e2be5
https://hg.mozilla.org/integration/mozilla-inbound/rev/feec85f6b0c6
https://hg.mozilla.org/integration/mozilla-inbound/rev/59df9a9aad11

Comment 76

[Biru [:poiru]]

I landed these with MOZ_WARN_UNUSED_RESULT due to build failures on other platforms. I'll fix those later.

Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=caeedd105ec1

Comment 77

[Biru [:poiru]]

(In reply to Birunthan Mohanathas [:poiru] from comment #76)
> I landed these with MOZ_WARN_UNUSED_RESULT due to build failures on other
> platforms. I'll fix those later.

Gah, I meant to say "with MOZ_WARN_UNUSED_RESULT commented out".

Comment 78

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/84dcea35596f
https://hg.mozilla.org/mozilla-central/rev/7464e2ba391c
https://hg.mozilla.org/mozilla-central/rev/b589dae2c293
https://hg.mozilla.org/mozilla-central/rev/808811474fe7
https://hg.mozilla.org/mozilla-central/rev/e2260ff7078a
https://hg.mozilla.org/mozilla-central/rev/69ad6f8f8266
https://hg.mozilla.org/mozilla-central/rev/e1cd799bafd4
https://hg.mozilla.org/mozilla-central/rev/5bf8a0b4add0
https://hg.mozilla.org/mozilla-central/rev/8e8c7edadef6
https://hg.mozilla.org/mozilla-central/rev/1141051601ab
https://hg.mozilla.org/mozilla-central/rev/733b193e2be5
https://hg.mozilla.org/mozilla-central/rev/feec85f6b0c6
https://hg.mozilla.org/mozilla-central/rev/59df9a9aad11

Comment 79

[Nathan Froyd [:froydnj]]

(In reply to Birunthan Mohanathas [:poiru] from comment #74)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #73)
> > My question is whether the second one is really worth it.  In DOM code, for
> > instance, it seems valuable to explicitly say that certain things are
> > *always* going to require fallible operations.
> 
> I think we should be able to have our cake and eat it, too. Here is a
> simplified example of what I think we should aim for:

Yes, that was what I was thinking, except that we wouldn't expose infallible allocation on the fallible array type at all.  (Depending on how the code works out, maybe it would be easier for people to discover things through static_assert than the compiler complaining about methods not found on the fallible array type...)

Comment 80

[(no longer active)]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #71)
> I'm not so sure comment 18 and comment 19 are a good idea; in particular,
> with a single nsTArray type, it seems like it'd be too easy to introduce
> infallible operation(s) on an array when fallible operations were used
> everywhere else.  Ehsan and Benjamin claimed that this is desirable, but I
> am not so sure...Ehsan, can you provide a concrete example for comment 19?

Yes, consider the case where we may populate an array with N elements, N being a value received from user input (for example a Web facing API) and that we also append one element to it in a bunch of places.  You'd want the first allocation to be fallible and all of the other ones to be infallible.

Comment 81

[Biru [:poiru]]

Attachment: Add mozilla::fallible to Fallible{Auto,}TArray::AppendElements calls

Comment 82

[Biru [:poiru]]

Attachment: Add mozilla::fallible to Fallible{Auto,}TArray::AppendElement calls

Comment 83

[Biru [:poiru]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #73)
> My question is whether the second one is really worth it.  In DOM code, for
> instance, it seems valuable to explicitly say that certain things are
> *always* going to require fallible operations.  If we duplicate information
> in the type declaration and the code that uses the variable/member/etc.,
> that doesn't seem like such a bad thing.

If we keep the type-level distinction for fallible allocation, I wonder if we should provide a AsInfallible() function to use a FallibleTArray as a infallible nsTArray? I think a static_cast would be safe in this case. See bug 1167414 for a use case.

Comment 84

[Nathan Froyd [:froydnj]]

(In reply to Birunthan Mohanathas [:poiru] from comment #83)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #73)
> > My question is whether the second one is really worth it.  In DOM code, for
> > instance, it seems valuable to explicitly say that certain things are
> > *always* going to require fallible operations.  If we duplicate information
> > in the type declaration and the code that uses the variable/member/etc.,
> > that doesn't seem like such a bad thing.
> 
> If we keep the type-level distinction for fallible allocation, I wonder if
> we should provide a AsInfallible() function to use a FallibleTArray as a
> infallible nsTArray? I think a static_cast would be safe in this case. See
> bug 1167414 for a use case.

If we did, something like that might be worthwhile.  Are you at the point in the refactoring where we need to make a decision on one type vs. two?

Comment 85

[Biru [:poiru]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #84)
> If we did, something like that might be worthwhile.  Are you at the point in
> the refactoring where we need to make a decision on one type vs. two?

The blocking bugs still need to be resolved, but yes, a decision will be required soon.

Comment 86

[Biru [:poiru]]

Attachment: Add mozilla::fallible to Fallible{Auto,}TArray::InsertElementAt calls

Comment 87

[Biru [:poiru]]

Attachment: Use FallibleTArray::Clear instead of SetLength(0)

Comment 88

[Biru [:poiru]]

Attachment: Use FallibleTArray::TruncateLength instead of SetLength where possible

Comment 89

[Biru [:poiru]]

Attachment: Add mozilla::fallible to Fallible{Auto,}Array calls in tests

Comment 90

[Nathan Froyd [:froydnj]]

I confess to not immediately understanding why we need attachment 8611390 [details] [diff] [review] (SetLength(0) -> Clear) and attachment 8611391 [details] [diff] [review] (SetLength -> TruncateLength).  (I can believe they make the code clearer, but am unclear how they are related to this bug.)  Can you provide a brief explanation?

Comment 91

[Nathan Froyd [:froydnj]]

ni? for comment 90

Comment 92

[Biru [:poiru]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #90)
> I confess to not immediately understanding why we need attachment 8611390 [details] [diff] [review]
> [details] [diff] [review] (SetLength(0) -> Clear) and attachment 8611391 [details] [diff] [review]
> [details] [diff] [review] (SetLength -> TruncateLength).  (I can believe
> they make the code clearer, but am unclear how they are related to this
> bug.)  Can you provide a brief explanation?

The SetLength calls in question are fallible. By using infallible Clear/TruncateLength, we will avoid the upcoming -Wunused-result warnings.

Comment 93

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9e8e733b412
https://hg.mozilla.org/integration/mozilla-inbound/rev/3e0b4fc575a1
https://hg.mozilla.org/integration/mozilla-inbound/rev/5d83a9455008
https://hg.mozilla.org/integration/mozilla-inbound/rev/182efe94d6b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/2e84ad186322

Comment 94

[Biru [:poiru]]

This landed with the wrong bug number: https://hg.mozilla.org/integration/mozilla-inbound/rev/e7061b445ad7

Apparently I forgot to upload it for review, so 'r=froydnj' is a lie. It is a trivial patch, though!

Comment 95

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d9e8e733b412
https://hg.mozilla.org/mozilla-central/rev/3e0b4fc575a1
https://hg.mozilla.org/mozilla-central/rev/5d83a9455008
https://hg.mozilla.org/mozilla-central/rev/182efe94d6b3
https://hg.mozilla.org/mozilla-central/rev/2e84ad186322

Comment 96

[Biru [:poiru]]

Attachment: Make nsTArray::InsertElementSorted support moves

Comment 97

[Biru [:poiru]]

Attachment: Add fallible variants of nsTArray::InsertElementSorted

Comment 98

[Biru [:poiru]]

Comment on attachment 8614292 [details] [diff] [review]
Make nsTArray::InsertElementSorted support moves

Oops, "This is required by the next patch to avoid const mismatch" was supposed to be the comment, not the description.

Comment 99

[Biru [:poiru]]

Attachment: Add mozilla::fallible to FallibleTArray::InsertElementSorted calls

Comment 100

[Biru [:poiru]]

Attachment: Add more fallible variants of nsTArray::InsertElementsAt

Comment 101

[Biru [:poiru]]

Attachment: Add mozilla::fallible to more FallibleTArray::InsertElementsAt calls

Comment 102

[Biru [:poiru]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #84)
> If we did, something like that might be worthwhile.  Are you at the point in
> the refactoring where we need to make a decision on one type vs. two?

I think I have reached this point now. We will have to decide if we want two types or not in order to proceed.

Comment 103

[Nathan Froyd [:froydnj]]

(In reply to Birunthan Mohanathas [:poiru] from comment #102)
> (In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #84)
> > If we did, something like that might be worthwhile.  Are you at the point in
> > the refactoring where we need to make a decision on one type vs. two?
> 
> I think I have reached this point now. We will have to decide if we want two
> types or not in order to proceed.

Let's go with a single type.  I still like the idea of having separate types, but:

1) Having a single type lets us avoid doing weird things in some places (e.g. IPC code); and
2) A single type is closer to MFBT's Vector, which is desirable for unification.

I also trust Ehsan's actual experience with DOM code overrides my theoretical musing about DOM code. :)

Comment 104

[Nathan Froyd [:froydnj]]

Comment on attachment 8614319 [details] [diff] [review]
Add mozilla::fallible to more FallibleTArray::InsertElementsAt calls

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

::: dom/network/TCPSocketChild.cpp
@@ +243,5 @@
>          }
>      }
>      InfallibleTArray<uint8_t> arr;
>      arr.SwapElements(fallibleArr);
>      SendData(arr, aTrackingNumber);

I guess this is a good example of the hoops we don't have to jump through if we have a single array type. ;)

Comment 105

[Nathan Froyd [:froydnj]]

Comment on attachment 8614292 [details] [diff] [review]
Make nsTArray::InsertElementSorted support moves

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

What is the error here if we don't support moves?

Comment 106

[Biru [:poiru]]

Now that we're going to a single type, we'll have to do something about the assignment operators, the copy constructor, and the constructor with a specified capacity.

One option would be to get rid of them. So we'd go from:

  nsTArray<int> arrayA(8);

  nsTArray<int> arrayB(arrayA);

  nsTArray<int> arrayC;
  arrayC = arrayA;

to:

  nsTArray<int> arrayA;
  arrayA.SetCapacity(8);

  nsTArray<int> arrayB;
  arrayB.Assign(arrayA);

  nsTArray<int> arrayC;
  arrayC.Assign(arrayA);

Any thoughts?

Comment 107

[Nathan Froyd [:froydnj]]

I think using Assign() in place of the copy constructor and assignment operators is fine.  Let's keep the move constructor and assignment operators, since those don't require memory allocation.

The constructor with a specified capacity also needs to go, in favor of an explicit SetCapacity.  This makes me a little sad, but that's life.

ni? back for comment 105.

Comment 108

[Biru [:poiru]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #105)
> What is the error here if we don't support moves?

This:

  ../dist/include/nsTArray.h:1399:12: error: no matching member function for call to 'InsertElementAt'
      return InsertElementAt<Item, ActualAlloc>(index, aItem);
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
  ../dist/include/nsTArray.h:1335:14: note: candidate function [with Item = gfxShapedText::DetailedGlyphStore::DGRec, ActualAlloc = nsTArrayInfallibleAllocator] not viable: 2nd argument ('const gfxShapedText::DetailedGlyphStore::DGRec') would lose const qualifier
    elem_type* InsertElementAt(index_type aIndex, Item&& aItem)
               ^

InsertElementSorted should arguably support moves anyway so I didn't try working around this.

Comment 109

[Nathan Froyd [:froydnj]]

Comment on attachment 8614292 [details] [diff] [review]
Make nsTArray::InsertElementSorted support moves

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

Losing the const here is unfortunate, but since our codebase isn't exactly const-correct anyway...

Comment 110

[Biru [:poiru]]

Attachment: Always require fallible argument for FallibleTArray functions

The protected/public ugliness is only temporary and used only because it's
simpler than the type traits equivalent. They will be removed when
FallibleTArray merges into nsTArray.

Comment 111

[Nathan Froyd [:froydnj]]

Comment on attachment 8616810 [details] [diff] [review]
Always require fallible argument for FallibleTArray functions

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

Just to make sure I understand, we're hiding away the fully-templated versions, but re-exposing them only in nsTArray/InfallibleTArray?  Works for me.

I think this may be the point where these changes start breaking comm-central (if we're not already there yet).  Please file a bug for comm-central about this change and/or post on dev-platform about things breaking, even if you're going to break things again in relatively short order.

Comment 112

[Biru [:poiru]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #111)
> Just to make sure I understand, we're hiding away the fully-templated
> versions, but re-exposing them only in nsTArray/InfallibleTArray?  Works for
> me.

Yep.

> I think this may be the point where these changes start breaking
> comm-central (if we're not already there yet).  Please file a bug for
> comm-central about this change and/or post on dev-platform about things
> breaking, even if you're going to break things again in relatively short
> order.

Will do.

Comment 113

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/117d25cbd07c
https://hg.mozilla.org/integration/mozilla-inbound/rev/ca04ee4db747
https://hg.mozilla.org/integration/mozilla-inbound/rev/4614a1e2baac
https://hg.mozilla.org/integration/mozilla-inbound/rev/ab220fda6c8a
https://hg.mozilla.org/integration/mozilla-inbound/rev/31e36b164974
https://hg.mozilla.org/integration/mozilla-inbound/rev/87b7bc3f561a

Comment 114

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/117d25cbd07c
https://hg.mozilla.org/mozilla-central/rev/ca04ee4db747
https://hg.mozilla.org/mozilla-central/rev/4614a1e2baac
https://hg.mozilla.org/mozilla-central/rev/ab220fda6c8a
https://hg.mozilla.org/mozilla-central/rev/31e36b164974
https://hg.mozilla.org/mozilla-central/rev/87b7bc3f561a

Comment 115

[Biru [:poiru]]

Attachment: Add mozilla::fallible to more FallibleTArray calls

Comment 116

[Biru [:poiru]]

Sent dev-platform email: https://lists.mozilla.org/pipermail/dev-platform/2015-June/010367.html

comm-central doesn't seem to use FallibleTArrays: https://github.com/mozilla/releases-comm-central/search?utf8=%E2%9C%93&q=FallibleTArray

Comment 117

[Nathan Froyd [:froydnj]]

Comment on attachment 8620490 [details] [diff] [review]
Add mozilla::fallible to more FallibleTArray calls

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

I understand the Set{Length,Capacity} changes, but I'm a bit unclear why the AppendElement calls noted below need to be fallible, since AFAICT all of them have sufficient capacity by virtue of the surrounding code.

r=me with the AppendElement calls reverted or a sufficiently convincing explanation of what's going on.

::: dom/nfc/gonk/NfcService.cpp
@@ +167,5 @@
>  
>        for (int i = 0; i < length; i++) {
>          NFCTechType tech = static_cast<NFCTechType>(mEvent.mTechList[i]);
>          MOZ_ASSERT(tech < NFCTechType::EndGuard_);
> +        *event.mTechList.Value().AppendElement(fallible) = tech;

This doesn't need to be fallible, since we SetCapacity'd above?

@@ +186,5 @@
>  
>        for (int i = 0; i < length; i++) {
>          NDEFRecordStruct& recordStruct = mEvent.mRecords[i];
> +        MozNDEFRecordOptions& record =
> +          *event.mRecords.Value().AppendElement(fallible);

Likewise.

::: gfx/thebes/gfxFontconfigFonts.cpp
@@ +183,5 @@
>            mFTFace(nullptr), mFTFaceInitialized(false)
>      {
>          cairo_font_face_reference(mFontFace);
>          cairo_font_face_set_user_data(mFontFace, &sFontEntryKey, this, nullptr);
> +        MOZ_ALWAYS_TRUE(mPatterns.AppendElement(fallible));

I'm not sure this change is worthwhile.

@@ +428,5 @@
>                  return; // OOM
>  
>              AdjustPatternToCSS(pattern);
>  
> +            MOZ_ALWAYS_TRUE(mPatterns.AppendElement(fallible));

This doesn't need to be fallible.

@@ +616,5 @@
>      FcPatternAddFTFace(pattern, FC_FT_FACE, mFace);
>      AddDownloadedFontEntry(pattern, this);
>  
>      // There is never more than one pattern
> +    MOZ_ALWAYS_TRUE(mPatterns.AppendElement(fallible));

I think likewise for this one.

::: media/libstagefright/binding/Index.cpp
@@ +255,5 @@
>                                           indice.end_offset);
>        sample.mCompositionRange = Interval<Microseconds>(indice.start_composition,
>                                                          indice.end_composition);
>        sample.mSync = indice.sync;
> +      MOZ_ALWAYS_TRUE(mIndex.AppendElement(sample, fallible));

This doesn't need to be fallible because of the SetCapacity.

::: media/libstagefright/binding/MP4Metadata.cpp
@@ +81,5 @@
>      indice.end_offset = s_indice.end_offset;
>      indice.start_composition = s_indice.start_composition - aMediaTime;
>      indice.end_composition = s_indice.end_composition - aMediaTime;
>      indice.sync = s_indice.sync;
> +    MOZ_ALWAYS_TRUE(aDest.AppendElement(indice, mozilla::fallible));

This doesn't need to be fallible because of the previous call to SetCapacity.

::: media/libstagefright/binding/MoofParser.cpp
@@ +496,5 @@
>      // Sometimes audio streams don't properly mark their samples as keyframes,
>      // because every audio sample is a keyframe.
>      sample.mSync = !(sampleFlags & 0x1010000) || aIsAudio;
>  
> +    MOZ_ALWAYS_TRUE(mIndex.AppendElement(sample, fallible));

This doesn't need to be fallible because of the previous call to SetCapacity.

Comment 118

[Biru [:poiru]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #117)
> I understand the Set{Length,Capacity} changes, but I'm a bit unclear why the
> AppendElement calls noted below need to be fallible, since AFAICT all of
> them have sufficient capacity by virtue of the surrounding code.
> 
> r=me with the AppendElement calls reverted or a sufficiently convincing
> explanation of what's going on.

As discussed on IRC, those AppendElement calls are already fallible due to their type (FallibleTArray). This patch merely makes that visible at the call site.

At the moment, it is not possible to make those calls truly infallible due to the type limitation. After this bug is done, we can simply remove `fallible` to make those calls infallible (which they already are due to the SetCapacity/SetLength calls). I'll add FIXMEs.

Comment 119

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/46bd5b92a395
https://hg.mozilla.org/integration/mozilla-inbound/rev/15f67d969310

Comment 120

[Jean-Yves Avenard [:jya]]

any call to nsTArray<T>.AppendElements(..) with the arguments being either const nsTArray<T>& or T*, size_t now give me /Users/jyavenard/Work/Mozilla/mozilla-central/dom/media/mediasource/TrackBuffersManager.cpp:928:19: 'AppendElements' is a protected member of 'nsTArray_Impl<unsigned char, nsTArrayFallibleAllocator>'

is this intended ???

can't push to inbound now... seems like a major drawback. how do you append more than one element at a time now ???????

Comment 121

[Jean-Yves Avenard [:jya]]

nm... only needs the fallible keyword. jumped the gun too quickly.

Comment 122

[Biru [:poiru]]

Attachment: Add nsTArray::Assign

The existing (non-GTest) TestTArray.cpp file is a bit of a mess so I chose to introduce a new GTest. If you are OK with this, I'll migrate the old tests later on.

Comment 123

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/46bd5b92a395
https://hg.mozilla.org/mozilla-central/rev/15f67d969310

Comment 124

[Nathan Froyd [:froydnj]]

Comment on attachment 8621219 [details] [diff] [review]
Add nsTArray::Assign

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

::: xpcom/tests/gtest/TestTArray.cpp
@@ +28,5 @@
> +  static nsTArray<int> sArray;
> +#ifdef DEBUG
> +  if (sArray.IsEmpty()) {
> +    sArray.AppendElement();
> +    ((nsTArrayHeader*)sArray.DebugGetHeader())->mLength = UINT32_MAX;

Wow, awful.  I'm not sure there's a better way, though...

Comment 125

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/37bee149c935

Comment 126

[Ryan VanderMeulen [:RyanVM]]

Backed out for checktest failures.
https://treeherder.mozilla.org/logviewer.html#?job_id=11220616&repo=mozilla-inbound

https://hg.mozilla.org/integration/mozilla-inbound/rev/049144705ae2

Comment 127

[Biru [:poiru]]

Attachment: Use move assignment instead of fallible assignment

Comment 128

[Biru [:poiru]]

Attachment: Use nsTArray::Assign instead of assignment operator to make fallibility explicit

Note that the fallible parameter has intentionally been omitted from the dom/crypto/ files because they use CryptoBuffer::Assign (which is already fallible).

Comment 129

[Nathan Froyd [:froydnj]]

Comment on attachment 8628456 [details] [diff] [review]
Use nsTArray::Assign instead of assignment operator to make fallibility explicit

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

r=me with the changes below.

::: dom/crypto/CryptoKey.cpp
@@ +386,5 @@
>  
>  void CryptoKey::SetSymKey(const CryptoBuffer& aSymKey)
>  {
> +  // FIXME: Check return value or make infallible after bug 968520 lands.
> +  mSymKey.Assign(aSymKey);

Why are we not explicitly marking this as fallible right now?  That way, the compiler will complain at us when we do change it...

@@ +1264,5 @@
>    }
>  
>    if (sym.Length() > 0)  {
> +    // FIXME: Check return value or make infallible after bug 968520 lands.
> +    mSymKey.Assign(sym);

Same here.

::: dom/crypto/WebCryptoTask.cpp
@@ +1288,5 @@
>  
>    void SetKeyData(const CryptoBuffer& aKeyData)
>    {
> +    // FIXME: Check return value or make infallible after bug 968520 lands.
> +    mKeyData.Assign(aKeyData);

Same here.

@@ +1947,5 @@
>          return NS_OK;
>        }
>  
> +      // FIXME: Check return value or make infallible after bug 968520 lands.
> +      mResult.Assign(mSymKey);

Same here.

::: layout/base/TouchCaret.cpp
@@ +1148,5 @@
>  
>    sel->Stringify(init.mSelectedText);
>  
> +  // FIXME: Check return value or make infallible after bug 968520 lands.
> +  init.mStates.AppendElement(dom::SelectionState::Taponcaret, fallible);

This isn't the same thing as the original code, right?  Because before, |init| would only contain Taponcaret...now it contains Taponcaret as the last element...and may or may not have stuff before that.  How about:

dom::Sequence<> state;
state.AppendElement(...);
init.mStates.SwapElements(state); // guaranteed to be OK.

Comment 130

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ff469c680a83
https://hg.mozilla.org/integration/mozilla-inbound/rev/a7fc634a80cc

Comment 131

[Biru [:poiru]]

(In reply to Nathan Froyd [:froydnj] [:nfroyd] from comment #129)
> Comment on attachment 8628456 [details] [diff] [review]
> Use nsTArray::Assign instead of assignment operator to make fallibility
> explicit
> 
> Review of attachment 8628456 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me with the changes below.
> 
> ::: dom/crypto/CryptoKey.cpp
> @@ +386,5 @@
> >  
> >  void CryptoKey::SetSymKey(const CryptoBuffer& aSymKey)
> >  {
> > +  // FIXME: Check return value or make infallible after bug 968520 lands.
> > +  mSymKey.Assign(aSymKey);
> 
> Why are we not explicitly marking this as fallible right now?  That way, the
> compiler will complain at us when we do change it...

Sorry, I should have been clearer in comment 128. `mSymKey` is a CryptoBuffer, which has several implicitly fallible Assign methods. We can't use FallibleTArray::Assign here because of C++ name hiding rules. I am hesitant to make FallibleTArray::Assign visible since CryptoBuffer also has several other Assign overloads (none of which take a `fallible_t` parameter).

Comment 132

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/ff469c680a83
https://hg.mozilla.org/mozilla-central/rev/a7fc634a80cc

Comment 133

[Biru [:poiru]]

Attachment: Explicitly use infallible allocator for ShiftData call in nsTArray::RemoveElementsAt

This call will never fail so this merely makes the intent clear.

Comment 134

[Biru [:poiru]]

Attachment: Always use fallible allocator with nsTArray_base::ShrinkCapacity

We don't need infallible allocation here because failure to shrink the
capacity will leave the array unchanged.

Comment 135

[Nathan Froyd [:froydnj]]

Comment on attachment 8636768 [details] [diff] [review]
Always use fallible allocator with nsTArray_base::ShrinkCapacity

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

::: xpcom/glue/nsTArray-inl.h
@@ +195,5 @@
>  
>    return ActualAlloc::SuccessResult();
>  }
>  
>  template<class Alloc, class Copy>

Maybe un-name the Alloc parameter, and write a short comment on why we don't use the Alloc parameter below?  A comment in nsTArray.h for ShrinkCapacity stating that ShrinkCapacity may not resize the vector also seems appropriate.

Comment 138

[Andrew McCreight [:mccr8]]

Can we close this and file another bug for followup work? The code has been on trunk since July.

Comment 139

[Biru [:poiru]]

(In reply to Andrew McCreight [:mccr8] from comment #138)
> Can we close this and file another bug for followup work? The code has been
> on trunk since July.

There are still a couple of unlanded patches here. I'll get them landed and file a follow-up. Sorry for late response.

Comment 140

[Biru [:poiru]]

Attachment: Always use fallible allocator with nsTArray_base::ShrinkCapacity

We don't need infallible allocation here because failure to shrink the
capacity will leave the array unchanged.

Comment 141

[Biru [:poiru]]

Comment on attachment 8680234 [details] [diff] [review]
Always use fallible allocator with nsTArray_base::ShrinkCapacity

Oops, I bzexported the wrong patch. Please ignore.

Comment 142

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a31f4c38280b
https://hg.mozilla.org/integration/mozilla-inbound/rev/826e977db3bb

Comment 143

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/a31f4c38280b
https://hg.mozilla.org/mozilla-central/rev/826e977db3bb

Comment 144

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/a31f4c38280b
https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/826e977db3bb

Comment 145

[Biru [:poiru]]

Going to close this in favor of new bugs for the remaining work.