1219415 - Remove unsafe FallibleTArray functions

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Biru [:poiru]]

FallibleTArray has e.g. a copy constructor, which does not provide a way to check for success. In order to get rid of FallibleTArray, we must first eradicate these cases.

Comment 1

[Biru [:poiru]]

Attachment: Part 1: Make MotionSegmentArray a nsTArray

Because all the relevant functions include the `fallible` parameter, this is effectively a no-op for the most part. The only exception is the the `new MotionSegmentArray(1)` call, which now becomes an infallible operation.

Comment 2

[Biru [:poiru]]

Attachment: Part 2: Make TransformArray a nsTArray

Because all the relevant functions include the `fallible` parameter, this is effectively a no-op for the most part. The only exception is the the `new TransformArray(1)` call, which now becomes an infallible operation.

Comment 3

[Biru [:poiru]]

Attachment: Part 3: Remove FallibleTArray capacity constructor

Comment 4

[Biru [:poiru]]

Attachment: Part 3: Remove FallibleTArray capacity constructor

Missed a hunk!

Comment 5

[Biru [:poiru]]

Attachment: Part 4: Check for copy failure in SVGSVGElement::SetTransformProperty

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8680196 [details] [diff] [review]
Part 1: Make MotionSegmentArray a nsTArray

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

r=me, with nits addressed.

First, the extended commit message is misleading -- it says "Because all the relevant functions include the `fallible` parameter", but really they don't. In two of the functions, you're *removing* the 'fallible' parameter -- but you know it's safe because Init() will have allocated 1 slot, and that's the slot we're filling here.

Please massage that language to be clearer.

::: dom/svg/SVGMotionSMILType.cpp
@@ +437,5 @@
>  
>    // Construct the intermediate result segment, and put it in our outparam.
>    // AppendElement has guaranteed success here, since Init() allocates 1 slot.
> +  resultArr.AppendElement(MotionSegment(path, resultDist,
> +                                        rotateType, rotateAngle));

Please update the comment before this. The "has guaranteed success" statement was explaining MOZ_ALWAYS_TRUE, but you've removed MOZ_ALWAYS_TRUE and you're using an infallible method with no success/failure concept.

I think it should now say "AppendElement won't need to realloc here, ..." or something like that.

@@ +479,5 @@
>    nsSMILValue smilVal(&SVGMotionSMILType::sSingleton);
>    MotionSegmentArray& arr = ExtractMotionSegmentArray(smilVal);
>  
>    // AppendElement has guaranteed success here, since Init() allocates 1 slot.
> +  arr.AppendElement(MotionSegment(aPath, aDist, aRotateType, aRotateAngle));

Same here.

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8680197 [details] [diff] [review]
Part 2: Make TransformArray a nsTArray

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

Like in the previous patch, the extended commit message note about fallible parameters is a bit misleading; please adjust that if you do end up actually removing 'fallible' parameters.

(BUT but see my final note in this comment, too; I'm not actually sure we want to remove fallible parameters...)

::: dom/svg/SVGTransformListSMILType.cpp
@@ +337,5 @@
>    if (!transforms.SetCapacity(transforms.Length() + aList.Length(), fallible))
>      return false;
>  
>    for (uint32_t i = 0; i < aList.Length(); ++i) {
> +    transforms.AppendElement(SVGTransformSMILData(aList[i]));

Please preserve some form of the old comment here, to explain why we're (seemingly) being inconsistent & not passing 'fallibile' here.

The comment here (removed in this patch) used to say:
  // No need to check the return value below since we have already allocated
  // the necessary space

Maybe tweak it to say:
  // No need to bother passing 'fallible' to AppendElement here, since [...]

@@ +358,5 @@
>        return false;
>  
>    for (uint32_t i = 0; i < smilTransforms.Length(); ++i) {
> +    MOZ_ALWAYS_TRUE(
> +      aTransforms.AppendElement(smilTransforms[i].ToSVGTransform(), fallible));

Why are you using MOZ_ALWAYS_TRUE & 'fallible' here, whereas elsewhere in this patch & the previous one you're just using the infallible API for cases like this?

We should be consistent one way or the other. I actually tend to think we should use 'fallible' everywhere & use MOZ_ALWAYS_TRUE assertions where appropriate, which means maybe the earlier part of this patch & the previous patches should stay as they were...

Comment 8

[Biru [:poiru]]

Attachment: Part 5: Make {Index,Moof}::mIndex a nsTArray

Because all the relevant functions include the `fallible` parameter, this mostly a no-op. The only exceptions are the implicit copy constructors generated for Moof and Index. Before, the implicit copy constructor could fail to copy mIndex because it was a FallibleTArray. With this change, failing to copy will OOM.

Comment 9

[Daniel Holbert [:dholbert]]

So with these fallibly-maintained-arrays, for guaranteed-to-succeed append (where we've pre-allocated), should we be standardizing on...
  (a) aArr.AppendElement(foo);
...or...
  (b) MOZ_ALWAYS_TRUE(aArr.AppendElement(foo, fallible));
?

I think I prefer (b), from an expressiveness, consistency, & sanity-checking perspective. It seems like you might prefer the former, but I'm not sure -- you're converting us to (a) in "part 1" and the first part of "part 2", vs. you're converting us to (b) in the second part of "part 2".

What is your preference?  And if it's (a), why is that your preference?

Comment 10

[Biru [:poiru]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> @@ +358,5 @@
> >        return false;
> >  
> >    for (uint32_t i = 0; i < smilTransforms.Length(); ++i) {
> > +    MOZ_ALWAYS_TRUE(
> > +      aTransforms.AppendElement(smilTransforms[i].ToSVGTransform(), fallible));
> 
> Why are you using MOZ_ALWAYS_TRUE & 'fallible' here, whereas elsewhere in
> this patch & the previous one you're just using the infallible API for cases
> like this?

It's not visible in the hunk, but in this particular case, aTransforms is a FallibleTArray<nsSVGTransform> (instead of a TransformArray as in the other cases). As such, it would fail to compile without `fallible`.

(In reply to Daniel Holbert [:dholbert] from comment #9)
> So with these fallibly-maintained-arrays, for guaranteed-to-succeed append
> (where we've pre-allocated), should we be standardizing on...
>   (a) aArr.AppendElement(foo);
> ...or...
>   (b) MOZ_ALWAYS_TRUE(aArr.AppendElement(foo, fallible));

I don't have a preference. I switch to (b) you want.

Comment 11

[Biru [:poiru]]

(In reply to Birunthan Mohanathas [:poiru] from comment #10)
> (In reply to Daniel Holbert [:dholbert] from comment #9)
> > So with these fallibly-maintained-arrays, for guaranteed-to-succeed append
> > (where we've pre-allocated), should we be standardizing on...
> >   (a) aArr.AppendElement(foo);
> > ...or...
> >   (b) MOZ_ALWAYS_TRUE(aArr.AppendElement(foo, fallible));
> 
> I don't have a preference. I switch to (b) you want.

Erm, I mean: I can switch to (b) if you want.

Comment 12

[Daniel Holbert [:dholbert]]

Thanks -- I'd prefer that (in parts 1 and 2 at least; I haven't looked at other patches).  If we're managing an array with fallible-appends in some places, it seems best to do that consistently (albeit with assertions/comments).

Disregard my review feedback on parts 1 and 2 then, since it was mostly about you *removing* `fallbile`, but now you won't be.

Comment 13

[Biru [:poiru]]

Attachment: Part 6: Remove FallibleTArray copy constructor

Comment 14

[Daniel Holbert [:dholbert]]

Comment on attachment 8680196 [details] [diff] [review]
Part 1: Make MotionSegmentArray a nsTArray

[bumping r+ to f+ on part 1, since it'll be changing per comment 12. IIUC, you can just leave the AppendElement calls untouched, so I suspect the next version of this patch will be much shorter.]

Comment 15

[Biru [:poiru]]

Attachment: Part 1: Make MotionSegmentArray a nsTArray

The `fallible` parameter is already included in all calls that are supposed to be fallible so this is effectively a no-op. Note that the `new MotionSegmentArray(1)` call now becomes an infallible operation.

Comment 16

[Biru [:poiru]]

Attachment: Part 2: Make TransformArray a nsTArray

The `fallible` parameter is already included in all calls that are supposed to be fallible so this is effectively a no-op. Note that the `new TransformArray(1)` call now becomes an infallible operation.

Comment 17

[Daniel Holbert [:dholbert]]

Per IRC, I'm uncomfortable switching from FallibleTArray (which enforces that it *only* gets extended using fallible APIs) to nsTArray (which has no such enforcement).

I'd like to keep these instances (which are currently FallibleTArray) "tagged" as only being usable w/ fallible APIs, if at all possible (even with a debug-only flag).  The conversion here should be s/FallibleTArray/nsTArray-which-is-enforcibly-fallible/, instead of just s/FallibleTArray/nsTArray-which-can-be-used-fallibly-or-infallibly/.

(poiru says he's got an idea for how to do this & will file a separate bug on allowing us to tag nsTArray instances thusly. The conversions here should use whatever facility he adds in that bug.)

Comment 18

[Daniel Holbert [:dholbert]]

Comment on attachment 8680235 [details] [diff] [review]
Part 1: Make MotionSegmentArray a nsTArray

r- on part 1 and part 2 for now, due to loss-of-enforcement concerns in comment 17.

Comment 19

[Daniel Holbert [:dholbert]]

Comment on attachment 8680207 [details] [diff] [review]
Part 4: Check for copy failure in SVGSVGElement::SetTransformProperty

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

r=me on part 4, with nits below addressed.

::: dom/svg/SVGSVGElement.cpp
@@ +1222,4 @@
>      return false;
>    }
> +
> +  transformOverride.release();

Add a comment at the end of this line, saying e.g.
  // SetProperty() call above took ownership.

Otherwise this release() kinda comes out of nowhere.  (In an ideal world, SetProperty would take the result of release(); in a slightly-less-ideal-world, we'd call release() *immediately* after SetProperty (if it succeeds) so that the association would be pretty clear.  In the current world, where you're tweaking existing code without messing with the logic structure too much, a comment will make things clear enough.)

::: dom/svg/SVGTransformList.h
@@ +41,3 @@
>    ~SVGTransformList() {}
>  
> +  static UniquePtr<SVGTransformList> CreateFrom(const SVGTransformList& aList);

Documentation nit:  So, just skimming this class, it's non-obvious why this new function exists, and why the copy constructor is explicitly removed, and that these two things are related.

Please add a comment documenting this method, and explaining the explicit copy-constructor deletion (either all as one comment or as 2 separate comments).

Comment 20

[Biru [:poiru]]

Attachment: Part 1: Avoid fallible constructor with MotionSegmentArray

This is an alternative solution to avoid blocking this bug.

Comment 21

[Biru [:poiru]]

Attachment: Part 2: Get rid of unnecessary minimum capacity for TransformArray

It seems like we don't actually need that single element capacity in this case.

Comment 22

[Daniel Holbert [:dholbert]]

Comment on attachment 8680263 [details] [diff] [review]
Part 2: Get rid of unnecessary minimum capacity for TransformArray

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

part 2 looks good; we do indeed seem to call (and check) SetCapacity before we append to a TransformArray, so the initial min-capacity does seem unnecessary.

Comment 23

[Daniel Holbert [:dholbert]]

Comment on attachment 8680253 [details] [diff] [review]
Part 1: Avoid fallible constructor with MotionSegmentArray

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

Hmm, this patch feels a bit hacky.  (I guess it's equivalent to what we do right now, but adding the explicit MOZ_CRASH in place of a "just-works" under-the-hood crash seems messier.)

I'd rather we just lightly refactor this class to drop the minimum-capacity stuff entirely, and handle alloc-failure when we call AppendElement. (since it wouldn't be able to be MOZ_ALWAYS_TRUE anymore) This is what we already do in SVGMotionSMILType::SandwichAdd -- we return NS_ERROR_OUT_OF_MEMORY for alloc failure.

This is a pretty straightforward change for one of the AppendElement calls, in SVGMotionSMILType::Interpolate().

For the other one, in SVGMotionSMILType::ConstructSMILValue, we'll need a minor refactoring to make it work.  It should take e.g. "nsSMILValue& aResult" as an outparam, and return bool or nsresult, and its one caller (GenerateValuesForPathAndPoints) should instantiate a local nsSMILValue and pass that in & check for success before using it.

Comment 24

[Daniel Holbert [:dholbert]]

The caller -- GenerateValuesForPathAndPoints -- should probably create the nsSMILValue with the correct type up-front -- like the following:
  nsSMILValue smilVal(&SVGMotionSMILType::sSingleton);

...and then the method, it calls, currently "ConstructSMILValue", should probably be renamed to "PopulateSMILValue", and should assert that the passed-in nsSMILValue has mType == SVGMotionSMILType::sSingleton to be sure we're messing with its data in a valid way.

(If you like, I'm happy to take this part, too; hopefully my explanation is clear enough, but if this seems tedious or doesn't make sense, I can spin up a patch myself.)

Comment 25

[Nathan Froyd [:froydnj]]

Comment on attachment 8680228 [details] [diff] [review]
Part 6: Remove FallibleTArray copy constructor

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

r=me on the nsTArray part, I guess, but I think bz would be a better reviewer on the DOM code, especially the bindings change.

::: dom/bindings/BindingDeclarations.h
@@ +458,5 @@
> +  Sequence(const Sequence& aOther)
> +  {
> +    // TODO: Make Codegen use Assign() instead of blindly using the assignment
> +    // operator. See bug 1179299.
> +    Unused << this->Assign(aOther, fallible);

Silently failing to copy stuff does not seem like a win...

::: dom/crypto/CryptoBuffer.h
@@ +26,5 @@
> +  CryptoBuffer(const CryptoBuffer& aOther)
> +  {
> +    // TODO: Remove uses of this copy constructor and then get rid of this. See
> +    // bug 1219415.
> +    Assign(aOther);

So we're using infallible assignment (?) here, but fallible assignment in Sequence<T>?  That doesn't seem quite right, though failing hard on Crypto stuff does seem like a good idea...

Comment 26

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8680228 [details] [diff] [review]
Part 6: Remove FallibleTArray copy constructor

CryptoBuffer is fallible for a reason: its size is under under page control.

I _thought_ at least as of when the domcrypto code landed that all consumers checked whether the CryptoBuffer size is correct after construction.  Is that not the case?

So I think we should use fallible append in CryptoBuffer as we do now and file a followup bug to sort it out.  Right now the bug number you have in the comment points to this bug, which is not very helpful.

r=me with that.

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 8680253 [details] [diff] [review]
Part 1: Avoid fallible constructor with MotionSegmentArray

(Setting r- on attachment 8680253 [details] [diff] [review] since I'd rather we do something a bit different, per comment 23)

Comment 28

[Biru [:poiru]]

Marking as WONTFIX because I think bug 1221614 is a better approach.