Bug 48785 – Setting attr repeatDur=0 on SVG element causes hang

• Component: SVG
• Status: RESOLVED FIXED

[Matthew Delaney]

There was a hang being caused because the bound checking was just "<0" and not "<=0". As per the spec, a repeatDur of 0 should have no effect:

""
repeatDur = "<clock-value>"
This causes the element to play repeatedly (loop) for the specified duration. Each repeat iteration lasts for the simple duration. This attribute has no affect if the duration is 0. A repeat duration less than the simple duration specified in dur will cut short the duration. A repeat duration equal to the simple duration is a no-op. Legal values are clock values greater than 0. In addition, "indefinite" may be specified to indicate that the element should repeat indefinitely (subject to the time container semantics). At most one of repeat or repeatDur should be specified (if both are specified, the repeat duration is defined as the minimum of the specified repeatDur, and the simple duration multiplied by the repeat (count).
""

[Matthew Delaney]

Created attachment 72563 [details]
Patch

[Simon Fraser (smfr)]

Comment on attachment 72563 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=72563&action=review

> WebCore/svg/animation/SVGSMILElement.cpp:531
> -    return m_cachedRepeatDur = clockValue < 0 ? SMILTime::unresolved() : clockValue;
> +    return m_cachedRepeatDur = clockValue <= 0 ? SMILTime::unresolved() : clockValue;

The assignment in the return statement is pretty grody here. Please change to:

m_cachedRepeatDur = clockValue <= 0 ? SMILTime::unresolved() : clockValue;
return m_cachedRepeatDur;

> LayoutTests/svg/animations/repeatDur-zero.xhtml:10
> +</svg>
>  \ No newline at end of file

Please add the newline.

[Matthew Delaney]

Created attachment 72570 [details]
Patch

[Matthew Delaney]

Committed r71066: <http://trac.webkit.org/changeset/71066>

[WebKit Review Bot]

http://trac.webkit.org/changeset/71066 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
accessibility/aria-activedescendant-crash.html
accessibility/aria-checkbox-text.html
accessibility/aria-hidden-update.html
accessibility/contenteditable-hidden-div.html
accessibility/crash-with-noelement-selectbox.html
accessibility/crashing-a-tag-in-map.html
accessibility/document-attributes.html
accessibility/first-letter-text-transform-causes-crash.html
accessibility/hang-in-isignored.html
accessibility/nested-layout-crash.html
accessibility/nochildren-elements.html
accessibility/non-data-table-cell-title-ui-element.html
accessibility/non-native-image-crash.html
accessibility/radio-button-checkbox-size.html
accessibility/removed-anonymous-block-child-causes-crash.html
accessibility/removed-continuation-element-causes-crash.html
accessibility/table-modification-crash.html
accessibility/table-nofirstbody.html
accessibility/table-notbody.html
accessibility/table-with-empty-thead-causes-crash.html

[Nikolas Zimmermann]

Matthew, can you change the testcase to include
if (window.layoutTestController)
    layoutTestController.dumpAsText()

there's no need to dump render tree information there.
At the moment the expected.png/checksum files are missing, but it's useless to add them anyway.

When doing that, you might also want to remove the existing expected.txt files from platform/mac/svg/animations, and from the gtk/qt/win/chromium* platform directories (in case they have already landed their own results), and move the expected.txt file right into svg/animations/

Thanks in advance!