1166544 - Handle return values of FallibleTArray functions in dom/{svg,smil}/

Status: RESOLVED FIXED

(Core :: SVG, defect)

Comment 1

[Biru [:poiru]]

Attachment: Surroud FallibleTArray functions that are known to succeed with MOZ_ALWAYS_TRUE in dom/{smil,svg}/

Comment 2

[Biru [:poiru]]

Attachment: Check InsertElementAt return value in DOMSVG*List::MaybeInsertNullInAnimValListAt

I used a `if` rather than `mozilla::unused` since this will change to an if anyway. I'll update "Bug N" after I have filed the other bug.

Comment 3

[Biru [:poiru]]

Attachment: Use ReplaceElementAt instead of Clear and InsertElementAt in SVGMotionSMILType::Add

Comment 4

[Daniel Holbert [:dholbert]]

Comment on attachment 8607826 [details] [diff] [review]
Surroud FallibleTArray functions that are known to succeed with MOZ_ALWAYS_TRUE in dom/{smil,svg}/

># HG changeset patch
># User Birunthan Mohanathas <[e-mail]>
># Date 1432077337 25200
>#      Tue May 19 16:15:37 2015 -0700
># Node ID 94618aeea78e4c92d37b3124c5c1855119adeacf
># Parent  43430f66af5b724789e3b72c987d014ff26e4764
>Bug 1166544 - Surroud FallibleTArray functions that are known to succeed with MOZ_ALWAYS_TRUE in dom/{smil,svg}/
>
>diff --git a/dom/smil/nsSMILAnimationFunction.cpp b/dom/smil/nsSMILAnimationFunction.cpp
>--- a/dom/smil/nsSMILAnimationFunction.cpp
>+++ b/dom/smil/nsSMILAnimationFunction.cpp
>@@ -781,30 +781,30 @@ nsSMILAnimationFunction::GetValues(const
>     }
> 
>     if (!parseOk || !result.SetCapacity(2, mozilla::fallible)) {
>       return NS_ERROR_FAILURE;
>     }
> 
>     if (!to.IsNull()) {
>       if (!from.IsNull()) {
>-        result.AppendElement(from);
>-        result.AppendElement(to);
>+        MOZ_ALWAYS_TRUE(result.AppendElement(from));
>+        MOZ_ALWAYS_TRUE(result.AppendElement(to));
>       } else {
>-        result.AppendElement(to);
>+        MOZ_ALWAYS_TRUE(result.AppendElement(to));

Could you add an explanatory comment above these 3 MOZ_ALWAYS_TRUE statements, saying e.g.
 // AppendElement() below must succeed, b/c SetCapacity() succeeded.

(This is less necessary in the other files, where there's just a single Insert|Append call after a SetCapacity call; IMO the connection is more immediately self-evident in those cases, because there's no if/else logic & only a single assertion.)


>diff --git a/dom/svg/DOMSVGPathSegList.cpp b/dom/svg/DOMSVGPathSegList.cpp
>--- a/dom/svg/DOMSVGPathSegList.cpp
>+++ b/dom/svg/DOMSVGPathSegList.cpp
>@@ -378,18 +378,20 @@ DOMSVGPathSegList::InsertItemBefore(DOMS
>-  InternalList().mData.InsertElementsAt(internalIndex, segAsRaw, 1 + argCount);
>-  mItems.InsertElementAt(aIndex, ItemProxy(domItem.get(), internalIndex));
>+  MOZ_ALWAYS_TRUE(
>+    InternalList().mData.InsertElementsAt(internalIndex, segAsRaw, 1 + argCount));

This line ends up too long, even with the de-indentation. Add a newline in the args somewhere (maybe before the last arg, "1 + argCount").

>+++ b/dom/svg/SVGMotionSMILType.cpp
>@@ -436,18 +436,18 @@ SVGMotionSMILType::Interpolate(const nsS
>-  resultArr.AppendElement(MotionSegment(path, resultDist,
>-                                        rotateType, rotateAngle));
>+  MOZ_ALWAYS_TRUE(resultArr.AppendElement(MotionSegment(path, resultDist,
>+                                                        rotateType, rotateAngle)));

This ends up too long, too. Just add a newline before "rotateAngle", probably.

r=me with the above

Comment 5

[Daniel Holbert [:dholbert]]

Comment on attachment 8607829 [details] [diff] [review]
Check InsertElementAt return value in DOMSVG*List::MaybeInsertNullInAnimValListAt

r=me with "bug N" filed per comment 2. Thanks!

Comment 6

[Daniel Holbert [:dholbert]]

Comment on attachment 8607831 [details] [diff] [review]
Use ReplaceElementAt instead of Clear and InsertElementAt in SVGMotionSMILType::Add

r=me

Comment 7

[Daniel Holbert [:dholbert]]

Comment on attachment 8607831 [details] [diff] [review]
Use ReplaceElementAt instead of Clear and InsertElementAt in SVGMotionSMILType::Add

>Bug 1166544 - Use ReplaceElementAt instead of Clear and InsertElementAt in SVGMotionSMILType::Add

I'd also give some weak encouragement to add "part 1", "part 2", etc to commit messages here.  Though I know not everyone follows that convention, so I won't block r+'ing on this point.

(IMO multi-part bugs should always use "part N" syntax.  Otherwise, each changeset -- when viewed directly on hg.m.o -- gives the mistaken impression that this bug is purely about doing that one thing in the commit message. E.g. that ^ commit message quoted above gives the impression that this bug is just about doing that one thing.  This doesn't matter too much, but it makes things clearer when a part is backed out & re-landed.  (It helps the backing-out sheriff realize that they might need to back out more than just a single cset -- and if they do backout & re-land a single cset from a multi-part bug, a "part N" hint will make it clearer that the re-landed cset is related to some other parts that landed earlier on, non-consecutively.))

Comment 8

[Robert Longson [:longsonr]]

For patch 2 (Check InsertElementAt return value in DOMSVG*List::MaybeInsertNullInAnimValListAt) the caller should call SetCapacity (if necessary) so that the functions themselves remain infallible.

Something like this untested and uncompiled code...


>   // Ensure we have enough memory so we can avoid complex error handling below:
>   if (!mItems.SetCapacity(mItems.Length() + 1, fallible) ||
>       !InternalList().SetCapacity(InternalList().Length() + 1)) {
>     error.Throw(NS_ERROR_OUT_OF_MEMORY);
>     return nullptr;
>   }
+   if (!mAList->mAnimVal && !mAList->IsAnimating()) {
+      if (!mAList->mAnimVal.mItems.SetCapacity(mAList->mAnimVal.mItems + 1)) {
+        error.Throw(NS_ERROR_OUT_OF_MEMORY);
+        return nullptr;
+      }
+   }

And just do the MOZ_ASSERT in the called functions.

Comment 9

[Biru [:poiru]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> I'd also give some weak encouragement to add "part 1", "part 2", etc to
> commit messages here.  Though I know not everyone follows that convention,
> so I won't block r+'ing on this point.

Will do. I usually include it when the order matters, but I might as well use it all the time.

(In reply to Daniel Holbert [:dholbert] from comment #5)
> r=me with "bug N" filed per comment 2. Thanks!

Filed bug 1166805.

Comment 10

[Daniel Holbert [:dholbert]]

(In reply to Robert Longson from comment #8)
> For patch 2 (Check InsertElementAt return value in
> DOMSVG*List::MaybeInsertNullInAnimValListAt) the caller should call
> SetCapacity (if necessary) so that the functions themselves remain
> infallible.

Hmm, I think I like that -- so these DOMSVG*List::MaybeInsertNullInAnimValListAt methods would then probably want to have a precondition that mAList->mAnimVal.mItems.Capacity() being > mAList->mAnimVal.mItems.Length(), so we know we can insert infallibly.

For the purposes of this bug here, I think that means part 2 should just use MOZ_ALWAYS_TRUE(...) instead of "if (...) { }".  These MOZ_ALWAYS_TRUE assertions are technically invalid, but they match what the code is already assuming, and we'll make them valid in bug 1166805.

Comment 11

[Robert Longson [:longsonr]]

Either that or punt the whole of the part 2 change to the other bug.

Comment 12

[Daniel Holbert [:dholbert]]

As I understand it, poiru's doing some work elsewhere to make nsTArray-with-fallible-alloc insert/append methods stricter (issuing build warnings/errors when their return value isn't checked).  "part 2" blocks that work landing.  I didn't want to make him block that work on solving the other bug (and refactoring our SVG code), so I'd suggested (in an IRC discussion) that he punt the refactoring to a followup, and do some form of checking with a FIXME/XXX/TODO comment in this bug.

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/63b4ab1748b3
https://hg.mozilla.org/integration/mozilla-inbound/rev/a995d14d8d87
https://hg.mozilla.org/integration/mozilla-inbound/rev/8eedc5073619

Comment 14

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/63b4ab1748b3
https://hg.mozilla.org/mozilla-central/rev/a995d14d8d87
https://hg.mozilla.org/mozilla-central/rev/8eedc5073619