Closed
Bug 1166805
Opened 10 years ago
Closed 10 years ago
Preemptively SetCapacity before calling DOMSVG*List::MaybeInsertNullInAnimValListAt, to prevent fallible InsertElementAt calls from failing
Categories
(Core :: SVG, defect)
Core
SVG
Tracking
()
RESOLVED
FIXED
mozilla45
People
(Reporter: poiru, Assigned: longsonr)
Details
Attachments
(2 files, 2 obsolete files)
5.43 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
14.09 KB,
patch
|
dholbert
:
review+
|
Details | Diff | Splinter Review |
See bug 1166544.
Comment 1•10 years ago
|
||
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•10 years ago
|
||
Assignee: nobody → longsonr
Attachment #8681680 -
Flags: review?(dholbert)
Comment 3•10 years ago
|
||
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•10 years ago
|
||
Flags: needinfo?(longsonr)
Attachment #8682084 -
Flags: review?(dholbert)
Assignee | ||
Comment 5•10 years ago
|
||
part 2 would be the diff between the original patch and part 1.
Comment 6•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
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)
Updated•10 years ago
|
Attachment #8682116 -
Flags: review+
Comment 9•10 years ago
|
||
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•10 years ago
|
||
Assignee | ||
Comment 11•10 years ago
|
||
Attachment #8682084 -
Attachment is obsolete: true
Attachment #8682084 -
Flags: review?(dholbert)
Attachment #8682199 -
Flags: review?(dholbert)
Assignee | ||
Comment 12•10 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 13•10 years ago
|
||
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 | ||
Comment 14•10 years ago
|
||
Keywords: leave-open
Assignee | ||
Comment 15•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Flags: in-testsuite-
Assignee | ||
Comment 16•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
Comment 17•10 years ago
|
||
Comment 18•10 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 10 years ago
status-firefox45:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla45
Comment 19•10 years ago
|
||
bugherder |
You need to log in
before you can comment on or make changes to this bug.
Description
•