Closed Bug 1166805 Opened 5 years ago Closed 4 years ago

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

Categories

(Core :: SVG, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla45
Tracking Status
firefox41 --- affected
firefox45 --- fixed

People

(Reporter: poiru, Assigned: longsonr)

Details

Attachments

(2 files, 2 obsolete files)

No longer depends on: 1166544
See bug 1166544 comment 8 and bug 1166544 comment 10 in particular.
Summary: Properly handle failing InsertElementAt call in DOMSVG*List::MaybeInsertNullInAnimValListAt → Preemptively SetCapacity before calling DOMSVG*List::MaybeInsertNullInAnimValListAt, to prevent fallible InsertElementAt calls from failing
Assignee: nobody → longsonr
Attachment #8681680 - Flags: review?(dholbert)
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.
Flags: needinfo?(longsonr)
Attached patch part 1 (obsolete) — Splinter Review
Flags: needinfo?(longsonr)
Attachment #8682084 - Flags: review?(dholbert)
part 2 would be the diff between the original patch and part 1.
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 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 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.)
Attachment #8681680 - Attachment is obsolete: true
Attachment #8681680 - Flags: review?(dholbert)
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.
(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.)
Attachment #8682084 - Attachment is obsolete: true
Attachment #8682084 - Flags: review?(dholbert)
Attachment #8682199 - Flags: review?(dholbert)
(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 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!
Attachment #8682199 - Flags: review?(dholbert) → review+
Flags: in-testsuite-
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/9b23e8ce60ad
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.