Closed Bug 550593 Opened 14 years ago Closed 14 years ago

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


(Core :: SVG, defect)

Not set





(Reporter: dholbert, Assigned: dholbert)




(1 file, 1 obsolete file)

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|.
Hrm, actually nsSVGTransformSMILType::Assign has this chunk:

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

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...
(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:
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)
Depends on: 550611
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.
Assignee: nobody → dholbert
Attachment #431120 - Flags: review?(roc)
How about we just wait for bug 550611 to be done and use the infallible vector there?
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.
Attachment #431120 - Attachment description: patch v1 (with explicit NS_RUNTIMEABORT on SetCapacity failure) → patch A v1 (with explicit NS_RUNTIMEABORT on SetCapacity failure)
Isn't the length of the transforms array actually under author control?
Yes.  Is that a problem?
(I mean, other than in a sg:dos sense)
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.
(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.
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.
Same as the last patch, but with all the changes to Assign() removed.
Attachment #431120 - Attachment is obsolete: true
Attachment #433672 - Flags: review?(roc)
Summary: Remove return value (and |new| success-checking) from implementations of nsISMILType::Init & Assign → Remove return value (and |new| success-checking) from implementations of nsISMILType::Init
Pushed patch v2:
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9.3a4
You need to log in before you can comment on or make changes to this bug.