550593 - Remove return value (and |new| success-checking) from implementations of nsISMILType::Init

Status: RESOLVED FIXED

(Core :: SVG, defect)

Description

[Daniel Holbert [:dholbert]]

nsISMILType::Init and Assign both currently return type |nsresult|, so that they can fail on OOM.

Now that we have infallible |new|, there's no reason these methods should be able to fail -- they should be changed to have return type |void|.

Comment 1

[Daniel Holbert [:dholbert]]

Hrm, actually nsSVGTransformSMILType::Assign has this chunk:

> 84   // Before we assign, ensure we have sufficient memory
> 85   PRBool result = dstTransforms->SetCapacity(srcTransforms->Length());
> 86   NS_ENSURE_TRUE(result,NS_ERROR_OUT_OF_MEMORY);

nsTArary::SetCapacity wraps a call to nsTArray::EnsureCapacity.  And EnsureCapacity uses NS_Alloc() under the hood. And NS_Alloc is still "fallible for now, until Stage 2". 

So, it would seem we can't make this nsISMILType API-change yet, since nsSVGTransformSMILType::Assign can still fail at allocating memory, due to this under-the-hood usage of NS_Alloc.

I suppose we could make this Assign method explicitly abort if SetCapacity fails.  Or maybe we'll get an infallible version of |nsTArray::SetCapacity| soon...

Comment 2

[Daniel Holbert [:dholbert]]

(In reply to comment #1)
> And NS_Alloc is still "fallible
> for now, until Stage 2". 

That quote is from cjones' initial post in this thread: http://groups.google.com/group/mozilla.dev.platform/browse_thread/thread/60329e4f9f98f1c4

Comment 3

[Daniel Holbert [:dholbert]]

Looks like cjones is working on an infallible vector/nsTArray class in bug 550611, which would help with resolving comment 1. (/me marks dependency on that bug)

Comment 4

[Daniel Holbert [:dholbert]]

Attachment: patch A v1 (with explicit NS_RUNTIMEABORT on SetCapacity failure)

This patch is basically done, except for an explicit NS_RUNTIMEABORT in nsSVGTransformSMILType::Assign() on nsTArray::SetCapacity failure. (per comment 1)

It sounds like cjones should have bug 550611 ready very soon -- once that has landed, I'll post another patch that switches nsSVGTransformSMILType over to the new vector type and removes this NS_RUNTIMEABORT.

Things in this patch:
 - Switch nsISMILType::Init() &  nsISMILType::Assign() to not null-check |new|, and to return |void| instead of |nsresult|.
 - Fix callers of the above methods (in nsSMILValue) to no longer expect a return val.
 - Make the precondition for Init in nsISMILType.h a little stricter (to reflect the reality of how it's being used & implemented), and make the same change in the NS_PRECONDITION() lines of nsSMILFloatType & SMILEnumType.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about we just wait for bug 550611 to be done and use the infallible vector there?

Comment 6

[Daniel Holbert [:dholbert]]

Yeah, that's what I'm planning on doing. I sort of mentioned that in comment 4, but I wasn't very clear -- sorry about that.

To be clear, this bug will have 2 patches that will land in a single push:
 (A) the currently-attached patch (changing the Init/Assign API)
 (B) a patch to switch nsSVGTransformSMILType over to the infallible vector (once it's available)

I'll remove the NS_RUNTIMEABORT in patch B, or perhaps in a followup to patch A after we've got patch B.  But for now I've got it in there right now as a reminder that it's not OOM-safe yet without an explicit abort there.  I don't intend for that code to make it onto mozilla-central, though.

Comment 7

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Isn't the length of the transforms array actually under author control?

Comment 8

[Daniel Holbert [:dholbert]]

Yes.  Is that a problem?

Comment 9

[Daniel Holbert [:dholbert]]

(I mean, other than in a sg:dos sense)

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Yes, we want to be able to survive an OOM attack.

One option would be to simply limit the length of the transforms array to some reasonable limit, say 10, in DOM code or elsewhere. Then we can make allocation of the array infallible.

Comment 11

[Daniel Holbert [:dholbert]]

(In reply to comment #10)
> Yes, we want to be able to survive an OOM attack.

A noble goal. :)

> One option would be to simply limit the length of the transforms array

That sounds reasonable, modulo some possible tweaking of the reasonable limit.

FWIW, these are all ways that the author could add more entries to the transforms array that we use for SMIL:
 (a) Append more to the |transform| DOM attribute.
    (e.g. transform="scale(2) rotate(45) scale(3)" should yield 3 entries)
 (b) Add more <animateTransform> elements that target the element. (These get stacked in an array via calls to SandwichAdd)
 (c) Add another <animateMotion> elements that target the element. (When we support <animateMotion>, it will add its effects into the same transform-array.)

We can fix (a) by capping the |transform| attribute's length in DOM code (wherever we parse the value of |transform|).

We can fix (b) and (c) by adding code in nsSVGTransformSMILType::SandwichAdd to check the current array-length against some arbitrary limit before appending.

Comment 12

[Daniel Holbert [:dholbert]]

After some more thought while reviewing jwatt's current patch on bug 515116, I think it'd be safer & simpler to leave nsISMILType::Assign() as-is for now, with a nsresult return-value.  That means we won't have to worry about the defense-against-OOM-exhaustion issues discussed here in comment 7 through comment 11 (and the similar issues that we would run into in bug 515116 if we needed to change that bug's patch so that SVGLengthListSMILType::Assign would never fail.)

Init() can still be infallible, though.  And most Assign() implementations won't actually need a failure case -- it'll just be the ones that use e.g. nsTArray::EnsureCapacity that would need a failure case.

Comment 13

[Daniel Holbert [:dholbert]]

Attachment: patch v2: Make nsISMILType::Init infallible

Same as the last patch, but with all the changes to Assign() removed.

Comment 14

[Daniel Holbert [:dholbert]]

Pushed patch v2: http://hg.mozilla.org/mozilla-central/rev/de234fec8da7