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

RESOLVED FIXED in Firefox 45

Status

()

RESOLVED FIXED
4 years ago
3 years ago

People

(Reporter: poiru, Assigned: longsonr)

Tracking

Trunk
mozilla45
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(firefox41 affected, firefox45 fixed)

Details

Attachments

(2 attachments, 2 obsolete attachments)

(Reporter)

Description

4 years ago
See bug 1166544.
(Reporter)

Updated

4 years ago
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)

Comment 2

3 years ago
Created attachment 8681680 [details] [diff] [review]
call setcapacity to ensure insert succeeds
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)
(Assignee)

Comment 4

3 years ago
Created attachment 8682084 [details] [diff] [review]
part 1
Flags: needinfo?(longsonr)
Attachment #8682084 - Flags: review?(dholbert)
(Assignee)

Comment 5

3 years ago
part 2 would be the diff between the original patch and part 1.
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 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.)
(Assignee)

Comment 11

3 years ago
Created attachment 8682199 [details] [diff] [review]
part 1 with review comments addressed
Attachment #8682084 - Attachment is obsolete: true
Attachment #8682084 - Flags: review?(dholbert)
Attachment #8682199 - Flags: review?(dholbert)
(Assignee)

Comment 12

3 years ago
(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+
(Assignee)

Updated

3 years ago
Flags: in-testsuite-
(Assignee)

Updated

3 years ago
Keywords: leave-open

Comment 18

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/9b23e8ce60ad
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
You need to log in before you can comment on or make changes to this bug.