Bug 48785 - Setting attr repeatDur=0 on SVG element causes hang
Summary: Setting attr repeatDur=0 on SVG element causes hang
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.6
: P2 Normal
Assignee: Matthew Delaney
URL: http://www.w3.org/1999/07/30/WD-SVG-1...
Keywords:
Depends on:
Blocks:
 
Reported: 2010-11-01 14:36 PDT by Matthew Delaney
Modified: 2010-11-03 08:38 PDT (History)
6 users (show)

See Also:


Attachments
Patch (3.24 KB, patch)
2010-11-01 14:44 PDT, Matthew Delaney
no flags Details | Formatted Diff | Diff
Patch (3.24 KB, patch)
2010-11-01 15:08 PDT, Matthew Delaney
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matthew Delaney 2010-11-01 14:36:55 PDT
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).
""
Comment 1 Matthew Delaney 2010-11-01 14:44:20 PDT
Created attachment 72563 [details]
Patch
Comment 2 Simon Fraser (smfr) 2010-11-01 14:59:36 PDT
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.
Comment 3 Matthew Delaney 2010-11-01 15:08:48 PDT
Created attachment 72570 [details]
Patch
Comment 4 Matthew Delaney 2010-11-01 15:41:03 PDT
Committed r71066: <http://trac.webkit.org/changeset/71066>
Comment 5 WebKit Review Bot 2010-11-01 16:05:05 PDT
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
Comment 6 Nikolas Zimmermann 2010-11-03 08:38:58 PDT
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!