1221614 - Make FallibleTArray's constructors and assignment operators infallible

Status: NEW

(Core :: XPCOM, defect)

Description

[Biru [:poiru]]

The following FallibleTArray constructors/operators are fallible:
- FallibleTArray(size_type aCapacity)
- FallibleTArray(const FallibleTArray<E>& aOther)
- FallibleTArray(const nsTArray_Impl<E, Allocator>& aOther)
- self_type& operator=(const self_type& aOther)
- self_type& operator=(const nsTArray_Impl<E, Allocator>& aOther)

The problem is that we don't have a straightforward mechanism to check if these operations succeeded. As a result, we have many (100+) instances where we use one of these operations without checking for success. This is also a problem with e.g. implictly generated copy assignment operators, which could silently fail to copy a member FallibleTArray and thus leave the object in an inconsistent state.

The easiest way out is to make these operations infallible. This would of course be in conflict with the name "FallibleTArray", but I think OOM-ing would be preferable to silently failing in these cases. Users can always use e.g. `Assign(..., fallible)` if they actually need fallible behaviour.

We could also fix all of these by hand, but I think that is more effort than it's worth. E.g. we would have to fix bug 1179299.

bz, froydnj, thoughts?

Comment 1

[Benjamin Smedberg]

I thought we were getting rid of FallibleTArray and just putting fallible operations on TArray?

Comment 2

[Biru [:poiru]]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #1)
> I thought we were getting rid of FallibleTArray and just putting fallible
> operations on TArray?

Yes, we are. Here is the what has been done so far:
- Allow nsTArray functions to take `fallible` parameter
- Enforce that `fallible` parameter is used with fallible FallibleTArray functions

If it weren't for FallibleTArray's constructors and assignment operators, we could more or less just replace all instances of FallibleTArray with nsTArray and be done with it.

If we decide that we are fine the operations in comment 0 becoming infallible, it would make it easy to switch to nsTArray (since it is infallible by default). On the other hand, if we decide that we want to keep those operations fallible even after switching to nsTArray, we would need to manually fix all uses by hand (which would not be trivial if we want to propagate errors correctly etc.).

Comment 3

[Boris Zbarsky [:bzbarsky]]

I basically stand by bug 1179299 comment 3.  Our policy is generally that content-controllable allocations should be fallible, and the ones mentioned in that bug are content-controllable.....

Comment 4

[Biru [:poiru]]

Attachment: sequence-unchecked-fallibe.txt

(In reply to Boris Zbarsky [:bz] from comment #3)
> I basically stand by bug 1179299 comment 3.  Our policy is generally that
> content-controllable allocations should be fallible, and the ones mentioned
> in that bug are content-controllable.....

I would argue that an OOM crash is preferable to allocating fallibly without checking for success. I've attached a list of unchecked fallible operations for Sequence<T>. There are 95 total operations with 74 in generated bindings code. And of course there is more for a bare FallibleTArray<T>.

In the vast majority (all?) of these cases, the allocations are so small that we are simply going to crash somewhere else. If you don't have a memory for a few bytes or kilobytes, all bets are off.

Is it worth it to fix all of these by hand properly (i.e. propagating the allocation failure correctly)? Or would it be better to simply use infallible allocation and switch specific operations to use fallible allocation as necessary based on crash-stats.m.o?

Comment 5

[Boris Zbarsky [:bzbarsky]]

> I would argue that an OOM crash is preferable to allocating fallibly without checking for
> success.

That's probably true as a blanket statement, though specific cases might not fit the general pattern.

> In the vast majority (all?) of these cases, the allocations are so small that we are
> simply going to crash somewhere else.

How do you know they're small?  The whole point of "content-controllable" is they can be as large as a web page wants them to be.

> Or would it be better to simply use infallible allocation and switch specific operations
> to use fallible allocation as necessary based on crash-stats.m.o?

That depends on whether you're trying to protect against accidental in-the-wild large allocations or against purposeful DoS attempts, to some extent.

But also, for the binding stuff it's not going to be possible to switch specific operations without switching all of the binding stuff in some way, right?  So I would rather do that normally than trying to do it as a chemspill because we shipped a release that crashes too much.  Unless, of course, you think that none of the binding bits are likely to need changing.

Comment 6

[Nathan Froyd [:froydnj]]

Sorry for the delay in responding.

It looks like operator= just silently fails for fallible arrays nowadays, which is not a great state of affairs.  But changing that to crash on failure wouldn't be helpful.  So it seems like s/operator=/Assign(..., fallible)/ and propagating that error would be an improvement on the status quo, even if it is a fair bit of work.

What's the relative frequency of the methods mentioned in comment 0, out of curiosity?  Could we at least disallow the constructors while operator= bits get worked on?