Require use of mozilla::fallible with fallible FallibleTArray calls

RESOLVED FIXED in mozilla43

Status

()

defect
RESOLVED FIXED
6 years ago
4 years ago

People

(Reporter: Ehsan, Assigned: poiru, Mentored)

Tracking

(Depends on 3 bugs, Blocks 3 bugs)

Trunk
mozilla43
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(b2g-v2.5 fixed)

Details

(Whiteboard: [lang=c++][requires some experience])

Attachments

(29 attachments, 26 obsolete attachments)

29.10 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
3.06 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
37.06 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.94 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.04 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
43.01 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
4.04 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
3.16 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
3.25 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
4.03 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.90 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
14.82 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
35.27 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
11.86 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.79 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
5.30 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.78 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
1.95 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.56 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
4.84 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.45 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
6.89 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
19.80 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
19.16 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
3.75 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
3.75 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
9.49 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
2.31 KB, patch
froydnj
: review+
Details | Diff | Splinter Review
4.76 KB, patch
poiru
: review+
Details | Diff | Splinter Review
Reporter

Description

6 years ago
See the discussion in bug 967167.

Updated

5 years ago
Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++][requires some experience]

Comment 1

5 years ago
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).
Assignee: nobody → scott.west
Status: NEW → ASSIGNED

Comment 2

5 years ago
Posted patch bug-968520.patch (obsolete) — Splinter Review
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

5 years ago
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.
Reporter

Comment 4

5 years ago
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

5 years ago
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.
Reporter

Comment 6

5 years ago
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

5 years ago
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

5 years ago
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

5 years ago
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.
Reporter

Comment 10

5 years ago
(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?
(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

5 years ago
(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

5 years ago
(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).
Reporter

Comment 14

5 years ago
(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

5 years ago
(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?
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

5 years ago
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?
I don't think the type-level distinction is especially valuable, no.
Reporter

Comment 19

5 years ago
(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

5 years ago
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

5 years ago
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?
Scott, feel free to post them as is, and if I want them combined I'll let you know.

Comment 23

5 years ago
Posted patch bug-968520-1-1.patch (obsolete) — Splinter Review
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.
Attachment #8377802 - Attachment is obsolete: true

Comment 24

5 years ago
Posted patch bug-968520-1-2.patch (obsolete) — Splinter Review
Updating content/svg for the first phase.

Comment 25

5 years ago
Posted patch bug-968520-1-3.patch (obsolete) — Splinter Review
Updating dom/smil for this first phase.

Comment 26

5 years ago
Posted patch bug-968520-1-4.patch (obsolete) — Splinter Review
Updating content/canvas for this first phase.

Comment 27

5 years ago
Posted patch bug-968520-1-5.patch (obsolete) — Splinter Review
Updating gfx/thebes for this first phase.

Comment 28

5 years ago
Posted patch bug-968520-1-6.patch (obsolete) — Splinter Review
Updating content/media for this first phase.

Comment 29

5 years ago
Posted patch bug-968520-1-7.patch (obsolete) — Splinter Review
Updating dom/network for this first phase.

Comment 30

5 years ago
Posted patch bug-968520-1-8.patch (obsolete) — Splinter Review
Updating dom/base for this first phase.

Comment 31

5 years ago
Posted patch bug-968520-1-9.patch (obsolete) — Splinter Review
Updating layout for this first phase.

Comment 32

5 years ago
Posted patch bug-968520-1-10.patch (obsolete) — Splinter Review
Updating network/base for this first phase.

Comment 33

5 years ago
Posted patch bug-968520-1-11.patch (obsolete) — Splinter Review
Updating intl for this first phase.

Comment 34

5 years ago
Posted patch bug-968520-1-12.patch (obsolete) — Splinter Review
Updating dom/plugins/ipc for this first phase.

Comment 35

5 years ago
Posted patch bug-968520-1-13.patch (obsolete) — Splinter Review
Updating dom/bindings for this first phase.

Comment 36

5 years ago
Posted patch bug-968520-1-14.patch (obsolete) — Splinter Review
Updating xpcom/io for this first phase.

Comment 37

5 years ago
Posted patch bug-968520-1-15.patch (obsolete) — Splinter Review
Updating toolkit/components/url-classifier for this first phase.

Comment 38

5 years ago
Posted patch bug-968520-1-16.patch (obsolete) — Splinter Review
Updating ipc/glue for this first phase.

Comment 39

5 years ago
Posted patch bug-968520-1-17.patch (obsolete) — Splinter Review
Updating misc in gfx/2d, image/src, storage/src, and xpcom/base for this first phase.

Comment 40

5 years ago
Posted patch bug-968520-1-18.patch (obsolete) — Splinter Review
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

5 years ago
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

5 years ago
Posted patch bug-968520-1-16.patch (obsolete) — Splinter Review
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...
Attachment #8380876 - Attachment is obsolete: true
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.
Flags: needinfo?(benjamin)

Comment 44

5 years ago
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).

Updated

5 years ago
Flags: needinfo?(benjamin)
Mentor: benjamin
Whiteboard: [mentor=benjamin@smedbergs.us][lang=c++][requires some experience] → [lang=c++][requires some experience]
Scott, are you still interested in working on this?
Flags: needinfo?(scott.west)

Comment 46

5 years ago
Interested yes, but not enough time to push it through entirely. Please feel free to assign it to someone with both interest and time :).
Flags: needinfo?(scott.gregory.west)
Birunthan, would you be interested in tackling this?
Flags: needinfo?(birunthan)
Assignee

Comment 48

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

Yep.
Assignee: scott.gregory.west → birunthan
Flags: needinfo?(birunthan)
Assignee

Comment 49

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

Apologies for the delay!
Attachment #8380855 - Attachment is obsolete: true
Attachment #8380859 - Attachment is obsolete: true
Attachment #8380860 - Attachment is obsolete: true
Attachment #8380861 - Attachment is obsolete: true
Attachment #8380862 - Attachment is obsolete: true
Attachment #8380864 - Attachment is obsolete: true
Attachment #8380865 - Attachment is obsolete: true
Attachment #8380866 - Attachment is obsolete: true
Attachment #8380867 - Attachment is obsolete: true
Attachment #8380868 - Attachment is obsolete: true
Attachment #8380869 - Attachment is obsolete: true
Attachment #8380871 - Attachment is obsolete: true
Attachment #8380872 - Attachment is obsolete: true
Attachment #8380873 - Attachment is obsolete: true
Attachment #8380874 - Attachment is obsolete: true
Attachment #8380877 - Attachment is obsolete: true
Attachment #8380878 - Attachment is obsolete: true
Attachment #8381630 - Attachment is obsolete: true
Attachment #8606506 - Flags: review?(nfroyd)
Assignee

Updated

4 years ago
Attachment #8606506 - Attachment is obsolete: true
Attachment #8606506 - Flags: review?(nfroyd)
Assignee

Comment 51

4 years ago
Attachment #8606508 - Flags: review?(nfroyd)
Assignee

Updated

4 years ago
Depends on: 1165735
Assignee

Updated

4 years ago
Depends on: 1165729
Assignee

Updated

4 years ago
Depends on: 1165731
Assignee

Updated

4 years ago
Depends on: 1165792
Assignee

Comment 53

4 years ago
Attachment #8606508 - Attachment is obsolete: true
Attachment #8606508 - Flags: review?(nfroyd)
Attachment #8606845 - Flags: review?(nfroyd)
Assignee

Comment 54

4 years ago
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.
Attachment #8606509 - Attachment is obsolete: true
Attachment #8606509 - Flags: review?(nfroyd)
Attachment #8606849 - Flags: review?(nfroyd)
Assignee

Comment 59

4 years ago
We should probably add MOZ_WARN_UNUSED_RESULT to e.g. the SetLength function in SVGLengthList.h. I'[l file another bug.
Attachment #8606855 - Flags: review?(nfroyd)
Assignee

Comment 63

4 years ago
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?
Flags: needinfo?(nfroyd)
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.
Attachment #8606507 - Flags: review?(nfroyd) → review+
Attachment #8606845 - Flags: review?(nfroyd) → review+
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...
Attachment #8606849 - Flags: review?(nfroyd) → review+
Attachment #8606850 - Flags: review?(nfroyd) → review+
Attachment #8606851 - Flags: review?(nfroyd) → review+
Attachment #8606852 - Flags: review?(nfroyd) → review+
Attachment #8606853 - Flags: review?(nfroyd) → review+
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?
Attachment #8606855 - Flags: review?(nfroyd) → review+
Assignee

Comment 69

4 years ago
(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.
Attachment #8606856 - Flags: review?(nfroyd) → review+
Attachment #8606857 - Flags: review?(nfroyd) → review+
Attachment #8606858 - Flags: review?(nfroyd) → review+
(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).
Flags: needinfo?(nfroyd)
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?
Flags: needinfo?(ehsan)
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.
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.
Attachment #8607087 - Flags: review?(nfroyd) → review+
Attachment #8607088 - Flags: review?(nfroyd) → review+
Assignee

Comment 74

4 years ago
(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?
Flags: needinfo?(nfroyd)
Assignee

Comment 76

4 years ago
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
Assignee

Updated

4 years ago
Keywords: leave-open
OS: Mac OS X → All
Hardware: x86 → All
Assignee

Comment 77

4 years ago
(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".
(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...)
Flags: needinfo?(nfroyd)
Reporter

Comment 80

4 years ago
(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.
Flags: needinfo?(ehsan)
Assignee

Updated

4 years ago
Depends on: 1166544
Assignee

Updated

4 years ago
Depends on: 1167414
Assignee

Updated

4 years ago
Depends on: 1167418
Assignee

Updated

4 years ago
Depends on: 1167420
Assignee

Updated

4 years ago
Depends on: 1167423
Attachment #8608875 - Flags: review?(nfroyd) → review+
Attachment #8608881 - Flags: review?(nfroyd) → review+
Assignee

Comment 83

4 years ago
(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.
Flags: needinfo?(nfroyd)
(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?
Flags: needinfo?(nfroyd) → needinfo?(birunthan)
Assignee

Comment 85

4 years ago
(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.
Flags: needinfo?(birunthan)
Attachment #8611389 - Flags: review?(nfroyd) → review+
Attachment #8611395 - Flags: review?(nfroyd) → review+
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?
ni? for comment 90
Flags: needinfo?(birunthan)
Assignee

Comment 92

4 years ago
(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.
Flags: needinfo?(birunthan)
Attachment #8611390 - Flags: review?(nfroyd) → review+
Attachment #8611391 - Flags: review?(nfroyd) → review+
Assignee

Comment 94

4 years ago
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!
Assignee

Comment 98

4 years ago
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.
Attachment #8614292 - Attachment description: This is required by the next patch to avoid const mismatch → Make nsTArray::InsertElementSorted support moves
Assignee

Updated

4 years ago
Attachment #8606850 - Attachment is obsolete: true
Assignee

Updated

4 years ago
Attachment #8606851 - Attachment is obsolete: true
(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.
Flags: needinfo?(nfroyd)
(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. :)
Flags: needinfo?(nfroyd)
Attachment #8614318 - Flags: review?(nfroyd) → review+
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. ;)
Attachment #8614319 - Flags: review?(nfroyd) → review+
Attachment #8614316 - Flags: review?(nfroyd) → review+
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?
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?
Flags: needinfo?(nfroyd)
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.
Flags: needinfo?(nfroyd) → needinfo?(birunthan)
(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.
Flags: needinfo?(birunthan)
Assignee

Updated

4 years ago
Depends on: 1172584
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...
Attachment #8614292 - Flags: review?(nfroyd) → review+
Attachment #8614293 - Flags: review?(nfroyd) → review+
Assignee

Updated

4 years ago
Depends on: 1172610
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.
Attachment #8616810 - Flags: review?(nfroyd)
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.
Attachment #8616810 - Flags: review?(nfroyd) → review+
(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 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.
Attachment #8620490 - Flags: review?(nfroyd) → review+
(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.
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 ???????
nm... only needs the fallible keyword. jumped the gun too quickly.
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.
Attachment #8621219 - Flags: review?(nfroyd)
Assignee

Updated

4 years ago
Depends on: 1174220
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...
Attachment #8621219 - Flags: review?(nfroyd) → review+
Assignee

Updated

4 years ago
Depends on: 1179282
Assignee

Updated

4 years ago
Depends on: 1179299
Note that the fallible parameter has intentionally been omitted from the dom/crypto/ files because they use CryptoBuffer::Assign (which is already fallible).
Attachment #8628456 - Flags: review?(nfroyd)
Attachment #8628436 - Flags: review?(nfroyd) → review+
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.
Attachment #8628456 - Flags: review?(nfroyd) → review+
(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).
Assignee

Updated

4 years ago
Depends on: 1182808
Assignee

Updated

4 years ago
Depends on: 1182824
Assignee

Updated

4 years ago
Depends on: 1182826
This call will never fail so this merely makes the intent clear.
Attachment #8636767 - Flags: review?(nfroyd)
We don't need infallible allocation here because failure to shrink the
capacity will leave the array unchanged.
Attachment #8636768 - Flags: review?(nfroyd)
Attachment #8636767 - Flags: review?(nfroyd) → review+
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.
Attachment #8636768 - Flags: review?(nfroyd) → review+

Updated

4 years ago
Duplicate of this bug: 1200835
Can we close this and file another bug for followup work? The code has been on trunk since July.
Flags: needinfo?(birunthan)
Depends on: 1201214
No longer depends on: 1201214
(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.
Flags: needinfo?(birunthan)
Assignee

Updated

4 years ago
Depends on: 1219415
We don't need infallible allocation here because failure to shrink the
capacity will leave the array unchanged.
Attachment #8680234 - Flags: review?(dholbert)
Assignee

Updated

4 years ago
Attachment #8636768 - Attachment is obsolete: true
Comment on attachment 8680234 [details] [diff] [review]
Always use fallible allocator with nsTArray_base::ShrinkCapacity

Oops, I bzexported the wrong patch. Please ignore.
Attachment #8680234 - Flags: review?(dholbert) → review+
Assignee

Updated

4 years ago
Depends on: 1221614
Going to close this in favor of new bugs for the remaining work.
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: leave-open
Resolution: --- → FIXED
Summary: Consolidate nsTArray, InfallibleTArray and FallibleTArray into the same type → Require use of mozilla::fallible with fallible FallibleTArray calls
Target Milestone: --- → mozilla43
Assignee

Updated

4 years ago
Blocks: 1235084
You need to log in before you can comment on or make changes to this bug.