Closed
Bug 1333418
Opened 8 years ago
Closed 8 years ago
Array bounds crash [@ BuildSegmentsFromValueEntries]
Categories
(Core :: DOM: Animation, defect)
Core
DOM: Animation
Tracking
()
RESOLVED
FIXED
mozilla54
Tracking | Status | |
---|---|---|
firefox51 | --- | unaffected |
firefox52 | --- | unaffected |
firefox53 | --- | fixed |
firefox54 | --- | fixed |
People
(Reporter: truber, Assigned: hiro)
References
(Blocks 1 open bug)
Details
(Keywords: crash, csectype-bounds, testcase)
Attachments
(2 files, 1 obsolete file)
402 bytes,
text/html
|
Details | |
59 bytes,
text/x-review-board-request
|
birtles
:
review+
ritu
:
approval-mozilla-aurora+
|
Details |
The attached testcase crashes in mozilla-central rev 8ff550409e. It also crashes inbound rev cdc8e1a140e2 with the patches from 1331704. ==25570==ERROR: AddressSanitizer: SEGV on unknown address 0x000000000000 (pc 0x7f3a0b1744be bp 0x7ffe451438f0 sp 0x7ffe451438d0 T0) #0 0x7f3a0b1744bd in InvalidArrayIndex_CRASH(unsigned long, unsigned long) /home/truber/src/m/unified/xpcom/glue/nsTArray.cpp:35:3 #1 0x7f3a0d6e993b in ElementAt /home/truber/src/m/unified/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:1160:7 #2 0x7f3a0d6e993b in operator[] /home/truber/src/m/unified/obj-x86_64-pc-linux-gnu/dist/include/nsTArray.h:1198 #3 0x7f3a0d6e993b in BuildSegmentsFromValueEntries /home/truber/src/m/unified/dom/animation/KeyframeUtils.cpp:1350 #4 0x7f3a0d6e993b in mozilla::KeyframeUtils::GetAnimationPropertiesFromKeyframes(nsTArray<mozilla::Keyframe> const&, nsTArray<nsTArray<mozilla::PropertyStyleAnimationValuePair> > const&, mozilla::dom::CompositeOperation, nsStyleContext*) /home/truber/src/m/unified/dom/animation/KeyframeUtils.cpp:715 #5 0x7f3a0d6cc98b in mozilla::dom::KeyframeEffectReadOnly::BuildProperties(nsStyleContext*) /home/truber/src/m/unified/dom/animation/KeyframeEffectReadOnly.cpp:860:5 #6 0x7f3a0d6baba2 in mozilla::dom::KeyframeEffectReadOnly::UpdateProperties(nsStyleContext*) /home/truber/src/m/unified/dom/animation/KeyframeEffectReadOnly.cpp:286:44 #7 0x7f3a0d6cc2e8 in mozilla::dom::KeyframeEffectReadOnly::SetKeyframes(nsTArray<mozilla::Keyframe>&&, nsStyleContext*) /home/truber/src/m/unified/dom/animation/KeyframeEffectReadOnly.cpp:208:5 #8 0x7f3a0d6cbb35 in mozilla::dom::KeyframeEffectReadOnly::SetKeyframes(JSContext*, JS::Handle<JSObject*>, mozilla::ErrorResult&) /home/truber/src/m/unified/dom/animation/KeyframeEffectReadOnly.cpp:185:3 #9 0x7f3a0d6c3e98 in already_AddRefed<mozilla::dom::KeyframeEffect> mozilla::dom::KeyframeEffectReadOnly::ConstructKeyframeEffect<mozilla::dom::KeyframeEffect, mozilla::dom::UnrestrictedDoubleOrKeyframeEffectOptions>(mozilla::dom::GlobalObject const&, mozilla::dom::Nullable<mozilla::dom::ElementOrCSSPseudoElement> const&, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeEffectOptions const&, mozilla::ErrorResult&) /home/truber/src/m/unified/dom/animation/KeyframeEffectReadOnly.cpp:784:3 #10 0x7f3a0d6c346d in mozilla::dom::KeyframeEffect::Constructor(mozilla::dom::GlobalObject const&, mozilla::dom::Nullable<mozilla::dom::ElementOrCSSPseudoElement> const&, JS::Handle<JSObject*>, mozilla::dom::UnrestrictedDoubleOrKeyframeEffectOptions const&, mozilla::ErrorResult&) /home/truber/src/m/unified/dom/animation/KeyframeEffect.cpp:45:10 #11 0x7f3a0f5003b1 in mozilla::dom::KeyframeEffectBinding::_constructor(JSContext*, unsigned int, JS::Value*) /home/truber/src/m/unified/obj-x86_64-pc-linux-gnu/dom/bindings/KeyframeEffectBinding.cpp:1815:64
Comment 1•8 years ago
|
||
Regression from bug 1305325.
Blocks: 1305325
status-firefox51:
--- → unaffected
status-firefox52:
--- → unaffected
status-firefox53:
--- → affected
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(hikezoe)
Assignee | ||
Comment 2•8 years ago
|
||
OK. 'spacing: paced(perspactive)' computes computed offsets as 0 in the second keyframe. After ApplySpacing, the keyframes are: { perspective: 'none', width: 'auto', computedOffset: 0 } { perspective: '172.17866832in', width: 'auto', computedOffset: 0 } { perspective: 0, computedOffset: 1 } Boris, I need your help. Is this expected behavior? If so, we need to handle the case in BuildSegmentsFromValueEntries().
Flags: needinfo?(hikezoe)
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(boris.chiou)
Comment 3•8 years ago
|
||
(In reply to Hiroyuki Ikezoe (:hiro) from comment #2) > OK. 'spacing: paced(perspactive)' computes computed offsets as 0 in the > second keyframe. > After ApplySpacing, the keyframes are: > > { perspective: 'none', width: 'auto', computedOffset: 0 } > { perspective: '172.17866832in', width: 'auto', computedOffset: 0 } > { perspective: 0, computedOffset: 1 } > > Boris, I need your help. > Is this expected behavior? If so, we need to handle the case in > BuildSegmentsFromValueEntries(). Yes, I think so. We treat zero perspective as inf perspective [1], so the 3rd keyframe is infinite perspective, so the 2nd computedOffset becomes 0. [1] http://searchfox.org/mozilla-central/rev/02a56df6474a97cf84d94bbcfaa126979970905d/layout/style/nsStyleTransformMatrix.h#32-41
Flags: needinfo?(boris.chiou)
Assignee | ||
Comment 4•8 years ago
|
||
Thank you Boris. I just checked the case again. I think 'j + 1 < n' check is necessary at [1] like we do just below another while(). [1] https://hg.mozilla.org/mozilla-central/file/6dccae211ae5/dom/animation/KeyframeUtils.cpp#l1350
Assignee | ||
Comment 5•8 years ago
|
||
https://treeherder.mozilla.org/#/jobs?repo=try&revision=62c018904beb94830b3f8d8c16ebaff30dfc3e3e
Assignee: nobody → hikezoe
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•8 years ago
|
||
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8830240 [details]
Don't exceed index of KeyframeValueEntry more than entry's length
Gosh, why didn't I use MozReview?
Attachment #8830240 -
Attachment is obsolete: true
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•8 years ago
|
||
mozreview-review |
Comment on attachment 8830242 [details] Bug 1333418 - Don't exceed index of KeyframeValueEntry more than entry's length. https://reviewboard.mozilla.org/r/107136/#review108436 ::: dom/animation/test/chrome/test_animation_properties.html:821 (Diff revision 1) > + { desc: 'a missing property in initial keyframe with duplicate offset ' > + + 'along with other values', > + frames: [ { left: '10px', offset: 0 }, > + { left: '8px', right: '8px', offset: 1 }, > + { left: '5px', right: '5px', offset: 1 } ], > + expected: [ { property: 'left', > + values: [ value(0, '10px', 'replace', 'linear'), > + value(1, '8px', 'replace'), > + value(1, '5px', 'replace') ] }, > + { property: 'right', > + values: [ value(0, undefined, 'add', 'linear'), This second test case don't hit this bug at all because this case is processed another while loop which has boundary check[1] [1] https://hg.mozilla.org/mozilla-central/file/24d9eb148461/dom/animation/KeyframeUtils.cpp#l1359 The first case of course hits this bug.
Assignee | ||
Updated•8 years ago
|
Flags: needinfo?(bbirtles)
Comment 10•8 years ago
|
||
mozreview-review |
Comment on attachment 8830242 [details] Bug 1333418 - Don't exceed index of KeyframeValueEntry more than entry's length. https://reviewboard.mozilla.org/r/107136/#review109174 ::: dom/animation/test/chrome/test_animation_properties.html:806 (Diff revision 1) > + { desc: 'a missing property in final keyframe with duplicate offset along' > + + 'with other values', Nit: Missing a space between 'along' and 'with'
Attachment #8830242 -
Flags: review+
Comment hidden (mozreview-request) |
Comment 13•8 years ago
|
||
Pushed by hikezoe@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/df653a73e414 Don't exceed index of KeyframeValueEntry more than entry's length. r=birtles
Comment 14•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/df653a73e414
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
Assignee | ||
Comment 15•8 years ago
|
||
Comment on attachment 8830242 [details] Bug 1333418 - Don't exceed index of KeyframeValueEntry more than entry's length. Approval Request Comment [Feature/Bug causing the regression]: bug 1305325. [User impact if declined]: Crash. [Is this code covered by automated tests?]: Yes. [Has the fix been verified in Nightly?]: Not yet at this point, just landed on mozilla-central. [Needs manual test from QE? If yes, steps to reproduce]: No. [List of other uplifts needed for the feature/fix]: None. [Is the change risky?]: I don't think so. [Why is the change risky/not risky?]: Because the change just adds a boundary check. [String changes made/needed]: None.
Attachment #8830242 -
Flags: approval-mozilla-aurora?
Comment on attachment 8830242 [details] Bug 1333418 - Don't exceed index of KeyframeValueEntry more than entry's length. Crash fix, Aurora53+
Attachment #8830242 -
Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Comment 17•8 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-aurora/rev/cb0502e9fcd7
Flags: in-testsuite+
You need to log in
before you can comment on or make changes to this bug.
Description
•