1166805 - Preemptively SetCapacity before calling DOMSVG*List::MaybeInsertNullInAnimValListAt, to prevent fallible InsertElementAt calls from failing

Status: RESOLVED FIXED

Description

[Birunthan Mohanathas [:poiru]]

See bug 1166544.

Comment 1

[Daniel Holbert [:dholbert] (PTO Fri Nov 16)]

See bug 1166544 comment 8 and bug 1166544 comment 10 in particular.

Comment 2

[Robert Longson [:longsonr]]

Created attachment 8681680 [details] [diff] [review]
call setcapacity to ensure insert succeeds

Comment 3

[Daniel Holbert [:dholbert] (PTO Fri Nov 16)]

Thanks for the patch!

So -- right now, this patch simultaneously refactors some existing code (via introducing "AnimListMirrorsBaseList" API), and *also* it adds some new SetCapacity calls.

Could you split this into two patches, with the refactoring first and the new code second? Specifically:
 - "part 1" would be everything in this patch except for the new SetCapacity calls.
 - "part 2" would add the new SetCapacity calls.

That would make this a bit easier to review, and it would make regression-hunting easier as well, in the (perhaps unlikely) event that this causes regressions.

Comment 4

[Robert Longson [:longsonr]]

Created attachment 8682084 [details] [diff] [review]
part 1

Comment 5

[Robert Longson [:longsonr]]

part 2 would be the diff between the original patch and part 1.

Comment 6

[Daniel Holbert [:dholbert] (PTO Fri Nov 16)]

Created attachment 8682116 [details] [diff] [review]
part 2 (author=longsonr)

Thanks! I generated "part 2" locally with these commands:
> hg qimport -p https://bugzilla.mozilla.org/attachment.cgi?id=8682084 --name longsonr1.patch
> patch -p1 -R < .hg/patches/longsonr1.patch
> curl -l https://bugzilla.mozilla.org/attachment.cgi?id=8681680 | patch -p1
> hg qnew longsonr2.patch

Attaching it here for reference & for review purposes.

Comment 7

[Daniel Holbert [:dholbert] (PTO Fri Nov 16)]

Comment on attachment 8682116 [details] [diff] [review]
part 2 (author=longsonr)

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

r=me on part 2, with the following typo (I think?) addressed:

::: dom/svg/DOMSVGPointList.cpp
@@ +317,5 @@
>        !InternalList().SetCapacity(InternalList().Length() + 1)) {
>      aError.Throw(NS_ERROR_OUT_OF_MEMORY);
>      return nullptr;
>    }
> +  if (!AnimListMirrorsBaseList()) {

I don't think this wants the "!" -- the condition is the opposite (no "!") in every other file here.

Comment 8

[Daniel Holbert [:dholbert] (PTO Fri Nov 16)]

Comment on attachment 8681680 [details] [diff] [review]
call setcapacity to ensure insert succeeds

(Canceling review on & obsoleting the first patch, since this should land in parts as noted above.)

Comment 9

[Daniel Holbert [:dholbert] (PTO Fri Nov 16)]

Comment on attachment 8682084 [details] [diff] [review]
part 1

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

Two nits that apply to various places in part 1:

::: dom/svg/DOMSVGLengthList.cpp
@@ +374,4 @@
>      return;
>    }
>  
> +  DOMSVGLengthList* animVal = mAList->mAnimVal;

It's a bit iffy that we proceed to dereference animVal right after declaring it, here & everywhere in this patch, when the thing we're assigning to it is generally allowed to be null.  Of course, you & I know that our earlier AnimListMirrorsBaseList() check guarantees that it'll be non-null at this point -- but that's non-obvious to someone skimming the code here. (and wondering about null-checks elsewhere to lack-of-null-checks here.)

Could you add an assertion after all of these no-longer-null-checked "animVal" decls, to explicitly state (& verify) the assumption that it's non-null? e.g. this one-liner:

  MOZ_ASSERT(animVal, "AnimListMirrorsBaseList() promised a non-null animVal");

Otherwise, in the unlikely event that we end up with a null pointer, we'll crash *while evaluating the condition for a different assertion** (about MOZ_ASSERT(animVal->mItems) in many cases here, which is awkward.

(This applies throughout this patch.)

@@ +392,5 @@
>    // This needs to be a strong reference; otherwise, the RemovingFromList call
>    // below might drop the last reference to animVal before we're done with it.
>    RefPtr<DOMSVGLengthList> animVal = mAList->mAnimVal;
>  
> +  if (!AnimListMirrorsBaseList()) {

This early-return should probably move up to be the first thing in this function now, to avoid AddRef/Release churn from the unused "animVal" declaration above it.

(That declaration was formerly used as part of this "if" check, but it's not used anymore, as of this patch.)

This also goes for this same code in DOMSVGNumberList.cpp and DOMSVGTransformList.cpp.

Comment 10

[Daniel Holbert [:dholbert] (PTO Fri Nov 16)]

(Part 1 is essentially r=me once comment 9 is addressed, but I'm not marking r+ yet -- it's probably worth another sanity-check review on the final version, since comment 9 represents a bunch of (small) changes throughout the patch.)

Comment 11

[Robert Longson [:longsonr]]

Created attachment 8682199 [details] [diff] [review]
part 1 with review comments addressed

Comment 12

[Robert Longson [:longsonr]]

(In reply to Daniel Holbert [:dholbert] from comment #7)
> Comment on attachment 8682116 [details] [diff] [review]
> part 2 (author=longsonr)
> 
> Review of attachment 8682116 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> r=me on part 2, with the following typo (I think?) addressed:
> 
> ::: dom/svg/DOMSVGPointList.cpp
> @@ +317,5 @@
> >        !InternalList().SetCapacity(InternalList().Length() + 1)) {
> >      aError.Throw(NS_ERROR_OUT_OF_MEMORY);
> >      return nullptr;
> >    }
> > +  if (!AnimListMirrorsBaseList()) {
> 
> I don't think this wants the "!" -- the condition is the opposite (no "!")
> in every other file here.

Indeed, well spotted. I'll fix that on check in.

Comment 13

[Daniel Holbert [:dholbert] (PTO Fri Nov 16)]

Comment on attachment 8682199 [details] [diff] [review]
part 1 with review comments addressed

>+++ b/dom/svg/DOMSVGLengthList.cpp
>   DOMSVGLengthList* animVal = mAList->mAnimVal;
> 
>+  MOZ_ASSERT(animVal, "AnimListMirrorsBaseList() promised a non-null animVal");
> 
>   MOZ_ASSERT(animVal->mItems.Length() == mItems.Length(),
>              "animVal list not in sync!");

Optional nit: seems like the blank line between the assertions wants to be dropped here (& throughout the patch), to make things a bit shorter & to visually group these consecutive sanity-checks together.  But, no big deal.

r=me either way, thanks!

Comment 14

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b97a299e6516

Comment 15

[Robert Longson [:longsonr]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9b23e8ce60ad

Comment 16

[Robert Longson [:longsonr]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=21ff7733ee3a

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/95762a9d2ee8

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9b23e8ce60ad

Comment 19

[Wes Kocher (:KWierso)]

https://hg.mozilla.org/mozilla-central/rev/95762a9d2ee8