63911 2011-07-04 08:22:42 -0700 REGRESSION (r89774): svg/W3C-SVG-1.1/animate-elem-82-t.svg hits assertion failure. 2011-07-14 09:20:19 -0700 1 1 1 Unclassified WebKit Layout and Rendering 528+ (Nightly build) All All RESOLVED FIXED http://dev.w3.org/SVG/profiles/1.1F2/test/svg/animate-elem-82-t.svg P2 Normal --- 0 joybro201 webkit-unassigned krit webkit.review.bot oldest_to_newest 431939 0 joybro201 2011-07-04 08:22:42 -0700 ASSERTION FAILED: i < size() /Users/dirk/Downloads/webkit-trunk/WebKitBuild/Debug/JavaScriptCore.framework/PrivateHeaders/Vector.h(542) : const T& WTF::Vector<T, inlineCapacity>::at(size_t) const [with T = float, long unsigned int inlineCapacity = 0ul] 1 WTF::Vector<float, 0ul>::at(unsigned long) const 2 WTF::Vector<float, 0ul>::operator[](unsigned long) const 3 WebCore::SVGAnimationElement::currentValuesForValuesAnimation(float, float&, WTF::String&, WTF::String&) const 4 WebCore::SVGAnimationElement::updateAnimation(float, unsigned int, WebCore::SVGSMILElement*) 5 WebCore::SVGSMILElement::progress(WebCore::SMILTime, WebCore::SVGSMILElement*) 6 WebCore::SMILTimeContainer::updateAnimations(WebCore::SMILTime, double, WTF::String const&) 7 WebCore::SMILTimeContainer::timerFired(WebCore::Timer<WebCore::SMILTimeContainer>*) 8 WebCore::Timer<WebCore::SMILTimeContainer>::fired() 431961 1 joybro201 2011-07-04 09:30:42 -0700 svg/W3C-SVG-1.1/animate-elem-33-t.svg also hits assertion. 432001 2 99648 joybro201 2011-07-04 11:32:04 -0700 Created attachment 99648 Patch 432049 3 99648 krit 2011-07-04 13:50:50 -0700 Comment on attachment 99648 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=99648&action=review This is a regression, so we need a regression test. Can you add a test to svg/animation please? I also have a question to your fix. r- because of the missing test. > Source/WebCore/svg/SVGAnimationElement.cpp:494 > + if (percent == 1) { > + from = m_values[valuesCount - 1]; > + to = m_values[valuesCount - 1]; > + effectivePercent = 1; > + return; > + } I just wonder. Here we return always effectivePercent = 1 if percent is 1,... > Source/WebCore/svg/SVGAnimationElement.cpp:520 > if (calcMode == CalcModeDiscrete) { > if (!keyTimesCount) > - index = percent == 1 ? valuesCount - 1 : static_cast<unsigned>(percent * valuesCount); > + index = static_cast<unsigned>(percent * valuesCount); > from = m_values[index]; > to = m_values[index]; > effectivePercent = 0; here we return effectivePercent = 0 for discrete animations. Sure that this doesn't cause new problems? 432083 4 joybro201 2011-07-04 19:01:46 -0700 (In reply to comment #3) > (From update of attachment 99648 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=99648&action=review > > This is a regression, so we need a regression test. Can you add a test to svg/animation please? I also have a question to your fix. r- because of the missing test. I'm not sure what kind of test could be added in this case because this regression has no symptoms, looks like working correctly, apart from assertion failures, and the assertion failures are not even occurred if the testcase has runAnimationTests() like our testcases in svg/animations. > > > Source/WebCore/svg/SVGAnimationElement.cpp:494 > > + if (percent == 1) { > > + from = m_values[valuesCount - 1]; > > + to = m_values[valuesCount - 1]; > > + effectivePercent = 1; > > + return; > > + } > > I just wonder. Here we return always effectivePercent = 1 if percent is 1,... > > > Source/WebCore/svg/SVGAnimationElement.cpp:520 > > if (calcMode == CalcModeDiscrete) { > > if (!keyTimesCount) > > - index = percent == 1 ? valuesCount - 1 : static_cast<unsigned>(percent * valuesCount); > > + index = static_cast<unsigned>(percent * valuesCount); > > from = m_values[index]; > > to = m_values[index]; > > effectivePercent = 0; > > here we return effectivePercent = 0 for discrete animations. Sure that this doesn't cause new problems? Yes, the effectivePercent is meaningful only if the 'from' and 'to' have different values. I checked that this doesn't affect the calculation result. 432875 5 joybro201 2011-07-06 11:00:55 -0700 On second thoughts, we already have the regression test, svg/W3C-SVG-1.1/animate-elem-82-t.svg, in the LayoutTests! how about that? 433572 6 99648 joybro201 2011-07-07 10:21:13 -0700 Comment on attachment 99648 Patch Dirk, would you review this again? I believe that this patch doesn't need another test because of the above-mentioned reasons. 437122 7 99648 webkit.review.bot 2011-07-14 09:20:14 -0700 Comment on attachment 99648 Patch Clearing flags on attachment: 99648 Committed r91002: <http://trac.webkit.org/changeset/91002> 437123 8 webkit.review.bot 2011-07-14 09:20:19 -0700 All reviewed patches have been landed. Closing bug. 99648 2011-07-04 11:32:04 -0700 2011-07-14 09:20:14 -0700 Patch bug-63911-20110705033201.patch text/plain 4132 joybro201 U3VidmVyc2lvbiBSZXZpc2lvbjogOTAwMTAKZGlmZiAtLWdpdCBhL1NvdXJjZS9XZWJDb3JlL0No YW5nZUxvZyBiL1NvdXJjZS9XZWJDb3JlL0NoYW5nZUxvZwppbmRleCBhNjE2Y2EyNzU5NDE5NjE1 Y2RiMjM3YTI4MzdmMzUzYzg0NWVmNGE1Li40ZTliZWFmN2U0ODRmYzdjZjI2MzVhM2JmZDM4Zjc4 MjU0NjY0YjRiIDEwMDY0NAotLS0gYS9Tb3VyY2UvV2ViQ29yZS9DaGFuZ2VMb2cKKysrIGIvU291 cmNlL1dlYkNvcmUvQ2hhbmdlTG9nCkBAIC0xLDMgKzEsMjMgQEAKKzIwMTEtMDctMDQgIFlvdW5n IEhhbiBMZWUgIDxqb3licm9AY29tcGFueTEwMC5uZXQ+CisKKyAgICAgICAgUmV2aWV3ZWQgYnkg Tk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgUkVHUkVTU0lPTiAocjg5Nzc0KTogc3ZnL1czQy1T VkctMS4xL2FuaW1hdGUtZWxlbS04Mi10LnN2ZyBoaXRzIGFzc2VydGlvbiBmYWlsdXJlLgorICAg ICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9NjM5MTEKKworICAg ICAgICBTb21lIGZ1bmN0aW9ucyBhc3N1bWVkIGNhbGN1bGF0ZUtleVRpbWVzSW5kZXgoKSBkb2Vz IG5vdCByZXR1cm4gdGhlIGxhc3QgaW5kZXggb2YKKyAgICAgICAgdGhlIGtleVRpbWVzLCBidXQg aXQgd291bGQgYmUgcmlnaHQgYmVoYXZpb3IgZm9yIGNhbGN1bGF0ZUtleVRpbWVzSW5kZXgoKSB3 aGVuIGl0CisgICAgICAgIGFjY2VwdHMgMSBhcyB0aGUgcGVyY2VudCBhcmd1bWVudC4KKworICAg ICAgICBUbyBhdm9pZCB0aGUgYXNzdW1wdGlvbiwgYW5kIGZvciBtb3JlIGVmZmljaWVuY3ksIHRo aXMgcGF0Y2ggYWxsb3dzIHRoZSBmdW5jdGlvbnMKKyAgICAgICAgdXNpbmcgY2FsY3VsYXRlS2V5 VGltZXNJbmRleCgpIHRvIHJldHVybiBlYXJseSB3aGVuIGl0IGFjY2VwdHMgMSBhcyB0aGUgcGVy Y2VudCBhcmd1bWVudC4KKyAgICAgICAgVGhlc2UgZnVuY3Rpb25zIGhhdmUgYWx3YXlzIHJldHVy bmVkIHRoZSBsYXN0IGVsZW1lbnQgb2YgdGhlIGNvcnJlc3BvbmRpbmcgdmVjdG9yCisgICAgICAg IGluIHRoYXQgc2l0dWF0aW9uLgorCisgICAgICAgICogc3ZnL1NWR0FuaW1hdGlvbkVsZW1lbnQu Y3BwOgorICAgICAgICAoV2ViQ29yZTo6U1ZHQW5pbWF0aW9uRWxlbWVudDo6Y2FsY3VsYXRlUGVy Y2VudEZyb21LZXlQb2ludHMpOgorICAgICAgICAoV2ViQ29yZTo6U1ZHQW5pbWF0aW9uRWxlbWVu dDo6Y3VycmVudFZhbHVlc0ZvclZhbHVlc0FuaW1hdGlvbik6CisKIDIwMTEtMDYtMjkgIE1pa2Ug V2VzdCAgPG1rd3N0QGNocm9taXVtLm9yZz4KIAogICAgICAgICBSZXZpZXdlZCBieSBQYXZlbCBG ZWxkbWFuLgpkaWZmIC0tZ2l0IGEvU291cmNlL1dlYkNvcmUvc3ZnL1NWR0FuaW1hdGlvbkVsZW1l bnQuY3BwIGIvU291cmNlL1dlYkNvcmUvc3ZnL1NWR0FuaW1hdGlvbkVsZW1lbnQuY3BwCmluZGV4 IDEyNGE4M2JlMGVlMTg5OTZmMDc3MjkzMjFiYTAyMDNkMjUwMGQyYmIuLjcyMDg0YzgyMzg5MDRi MmNmMTY0YTIxODY4ZjdkMjg4NDRlM2NjODQgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9XZWJDb3JlL3N2 Zy9TVkdBbmltYXRpb25FbGVtZW50LmNwcAorKysgYi9Tb3VyY2UvV2ViQ29yZS9zdmcvU1ZHQW5p bWF0aW9uRWxlbWVudC5jcHAKQEAgLTQ0OCw2ICs0NDgsOSBAQCBmbG9hdCBTVkdBbmltYXRpb25F bGVtZW50OjpjYWxjdWxhdGVQZXJjZW50RnJvbUtleVBvaW50cyhmbG9hdCBwZXJjZW50KSBjb25z dAogICAgIEFTU0VSVChtX2tleVRpbWVzLnNpemUoKSA+IDEpOwogICAgIEFTU0VSVChtX2tleVBv aW50cy5zaXplKCkgPT0gbV9rZXlUaW1lcy5zaXplKCkpOwogCisgICAgaWYgKHBlcmNlbnQgPT0g MSkKKyAgICAgICAgcmV0dXJuIG1fa2V5UG9pbnRzW21fa2V5UG9pbnRzLnNpemUoKSAtIDFdOwor CiAgICAgdW5zaWduZWQgaW5kZXggPSBjYWxjdWxhdGVLZXlUaW1lc0luZGV4KHBlcmNlbnQpOwog ICAgIGZsb2F0IGZyb21QZXJjZW50ID0gbV9rZXlUaW1lc1tpbmRleF07CiAgICAgZmxvYXQgdG9Q ZXJjZW50ID0gbV9rZXlUaW1lc1tpbmRleCArIDFdOwpAQCAtNDU1LDkgKzQ1OCw5IEBAIGZsb2F0 IFNWR0FuaW1hdGlvbkVsZW1lbnQ6OmNhbGN1bGF0ZVBlcmNlbnRGcm9tS2V5UG9pbnRzKGZsb2F0 IHBlcmNlbnQpIGNvbnN0CiAgICAgZmxvYXQgdG9LZXlQb2ludCA9IG1fa2V5UG9pbnRzW2luZGV4 ICsgMV07CiAgICAgCiAgICAgaWYgKGNhbGNNb2RlKCkgPT0gQ2FsY01vZGVEaXNjcmV0ZSkKLSAg ICAgICAgcmV0dXJuIHBlcmNlbnQgPT0gMSA/IHRvS2V5UG9pbnQgOiBmcm9tS2V5UG9pbnQ7Cisg ICAgICAgIHJldHVybiBmcm9tS2V5UG9pbnQ7CiAgICAgCi0gICAgZmxvYXQga2V5UG9pbnRQZXJj ZW50ID0gcGVyY2VudCA9PSAxID8gMSA6IChwZXJjZW50IC0gZnJvbVBlcmNlbnQpIC8gKHRvUGVy Y2VudCAtIGZyb21QZXJjZW50KTsKKyAgICBmbG9hdCBrZXlQb2ludFBlcmNlbnQgPSAocGVyY2Vu dCAtIGZyb21QZXJjZW50KSAvICh0b1BlcmNlbnQgLSBmcm9tUGVyY2VudCk7CiAgICAgCiAgICAg aWYgKGNhbGNNb2RlKCkgPT0gQ2FsY01vZGVTcGxpbmUpIHsKICAgICAgICAgQVNTRVJUKG1fa2V5 U3BsaW5lcy5zaXplKCkgPT0gbV9rZXlQb2ludHMuc2l6ZSgpIC0gMSk7CkBAIC00ODMsNiArNDg2 LDEzIEBAIHZvaWQgU1ZHQW5pbWF0aW9uRWxlbWVudDo6Y3VycmVudFZhbHVlc0ZvclZhbHVlc0Fu aW1hdGlvbihmbG9hdCBwZXJjZW50LCBmbG9hdCYKICAgICBBU1NFUlQobV9hbmltYXRpb25WYWxp ZCk7CiAgICAgQVNTRVJUKHZhbHVlc0NvdW50ID4gMSk7CiAgICAgCisgICAgaWYgKHBlcmNlbnQg PT0gMSkgeworICAgICAgICBmcm9tID0gbV92YWx1ZXNbdmFsdWVzQ291bnQgLSAxXTsKKyAgICAg ICAgdG8gPSBtX3ZhbHVlc1t2YWx1ZXNDb3VudCAtIDFdOworICAgICAgICBlZmZlY3RpdmVQZXJj ZW50ID0gMTsKKyAgICAgICAgcmV0dXJuOworICAgIH0KKwogICAgIENhbGNNb2RlIGNhbGNNb2Rl ID0gdGhpcy0+Y2FsY01vZGUoKTsKICAgICBpZiAoaGFzVGFnTmFtZShTVkdOYW1lczo6YW5pbWF0 ZVRhZykgfHwgaGFzVGFnTmFtZShTVkdOYW1lczo6YW5pbWF0ZUNvbG9yVGFnKSkgewogICAgICAg ICBjb25zdCBTVkdBbmltYXRlRWxlbWVudCogYW5pbWF0ZUVsZW1lbnQgPSBzdGF0aWNfY2FzdDxj b25zdCBTVkdBbmltYXRlRWxlbWVudCo+KHRoaXMpOwpAQCAtNTA0LDcgKzUxNCw3IEBAIHZvaWQg U1ZHQW5pbWF0aW9uRWxlbWVudDo6Y3VycmVudFZhbHVlc0ZvclZhbHVlc0FuaW1hdGlvbihmbG9h dCBwZXJjZW50LCBmbG9hdCYKICAgICB1bnNpZ25lZCBpbmRleCA9IGNhbGN1bGF0ZUtleVRpbWVz SW5kZXgocGVyY2VudCk7CiAgICAgaWYgKGNhbGNNb2RlID09IENhbGNNb2RlRGlzY3JldGUpIHsK ICAgICAgICAgaWYgKCFrZXlUaW1lc0NvdW50KSAKLSAgICAgICAgICAgIGluZGV4ID0gcGVyY2Vu dCA9PSAxID8gdmFsdWVzQ291bnQgLSAxIDogc3RhdGljX2Nhc3Q8dW5zaWduZWQ+KHBlcmNlbnQg KiB2YWx1ZXNDb3VudCk7CisgICAgICAgICAgICBpbmRleCA9IHN0YXRpY19jYXN0PHVuc2lnbmVk PihwZXJjZW50ICogdmFsdWVzQ291bnQpOwogICAgICAgICBmcm9tID0gbV92YWx1ZXNbaW5kZXhd OwogICAgICAgICB0byA9IG1fdmFsdWVzW2luZGV4XTsKICAgICAgICAgZWZmZWN0aXZlUGVyY2Vu dCA9IDA7CkBAIC01MjcsNyArNTM3LDcgQEAgdm9pZCBTVkdBbmltYXRpb25FbGVtZW50OjpjdXJy ZW50VmFsdWVzRm9yVmFsdWVzQW5pbWF0aW9uKGZsb2F0IHBlcmNlbnQsIGZsb2F0JgogICAgIGZy b20gPSBtX3ZhbHVlc1tpbmRleF07CiAgICAgdG8gPSBtX3ZhbHVlc1tpbmRleCArIDFdOwogICAg IEFTU0VSVCh0b1BlcmNlbnQgPiBmcm9tUGVyY2VudCk7Ci0gICAgZWZmZWN0aXZlUGVyY2VudCA9 IHBlcmNlbnQgPT0gMSA/IDEgOiAocGVyY2VudCAtIGZyb21QZXJjZW50KSAvICh0b1BlcmNlbnQg LSBmcm9tUGVyY2VudCk7CisgICAgZWZmZWN0aXZlUGVyY2VudCA9IChwZXJjZW50IC0gZnJvbVBl cmNlbnQpIC8gKHRvUGVyY2VudCAtIGZyb21QZXJjZW50KTsKICAgICAKICAgICBpZiAoY2FsY01v ZGUgPT0gQ2FsY01vZGVTcGxpbmUpIHsKICAgICAgICAgQVNTRVJUKG1fa2V5U3BsaW5lcy5zaXpl KCkgPT0gbV92YWx1ZXMuc2l6ZSgpIC0gMSk7Cg==