Bug 94756 – Fix a operator ordering bug in SVGSMILElement::calculateAnimationPercentAndRepeat

• Component: New Bugs
• Status: RESOLVED FIXED

[Nico Weber]

Fix a operator ordering bug in SVGSMILElement::calculateAnimationPercentAndRepeat

[Nico Weber]

Created attachment 160032 [details]
Patch

[Eric Seidel (no email)]

Can we test this?

[Stephen Chenney]

Comment on attachment 160032 [details]
Patch

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

> Source/WebCore/svg/animation/SVGSMILElement.cpp:969
>          repeat = static_cast<unsigned>(repeatingDuration.value() / simpleDuration.value());

I would propose avoiding the fmod entirely. It apparently is being used to determine if the fractional amount of repeatingDuration.value() / simpleDuration.value() is zero. Instead:
float repeatFloat = repeatingDuration.value() / simpleDuration.value();
repeat = static_cast<unsigned>(repeatFloat);
if (!(repeatFloat - repeat))
    repeat--;

No idea how to test it.

[Nico Weber]

(In reply to comment #3)
> (From update of attachment 160032 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160032&action=review
> 
> > Source/WebCore/svg/animation/SVGSMILElement.cpp:969
> >          repeat = static_cast<unsigned>(repeatingDuration.value() / simpleDuration.value());
> 
> I would propose avoiding the fmod entirely. It apparently is being used to determine if the fractional amount of repeatingDuration.value() / simpleDuration.value() is zero. Instead:
> float repeatFloat = repeatingDuration.value() / simpleDuration.value();
> repeat = static_cast<unsigned>(repeatFloat);
> if (!(repeatFloat - repeat))
>     repeat--;

How is that simpler than what's currently there?

[Stephen Chenney]

(In reply to comment #4)
> (In reply to comment #3)
> > (From update of attachment 160032 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=160032&action=review
> > 
> > > Source/WebCore/svg/animation/SVGSMILElement.cpp:969
> > >          repeat = static_cast<unsigned>(repeatingDuration.value() / simpleDuration.value());
> > 
> > I would propose avoiding the fmod entirely. It apparently is being used to determine if the fractional amount of repeatingDuration.value() / simpleDuration.value() is zero. Instead:
> > float repeatFloat = repeatingDuration.value() / simpleDuration.value();
> > repeat = static_cast<unsigned>(repeatFloat);
> > if (!(repeatFloat - repeat))
> >     repeat--;
> 
> How is that simpler than what's currently there?

It's not really simpler, but it does less work, I think, and does not have 
numerical accuracy issues. Actually, this is probably better:

float repeatFloat = repeatingDuration.value() / simpleDuration.value();
repeat = static_cast<unsigned>(repeatFloat);
if (repeatFloat == repeat)
    repeat--;

[Nico Weber]

> > How is that simpler than what's currently there?
> 
> It's not really simpler, but it does less work, I think, and does not have 
> numerical accuracy issues. Actually, this is probably better:
> 
> float repeatFloat = repeatingDuration.value() / simpleDuration.value();
> repeat = static_cast<unsigned>(repeatFloat);
> if (repeatFloat == repeat)
>     repeat--;

I don't think this is performance-sensitive code. I would assume that fmod is at least as smart as any reimplementation we attempt to do.

[Hans Wennborg]

*** Bug 96093 has been marked as a duplicate of this bug. ***

[Nico Weber]

This is blocking our next clang roll. Can this go in? The alternative is to disable this new warning for webkit.

[Philip Rogers]

(In reply to comment #8)
> This is blocking our next clang roll. Can this go in? The alternative is to disable this new warning for webkit.

I think it's fine to go in, but you'll have to find an r+ somewhere else.

[Dirk Schulze]

Comment on attachment 160032 [details]
Patch

I would be in favor of doing it better from the beginning.

[Nico Weber]

Created attachment 162986 [details]
Patch

[Early Warning System Bot]

Comment on attachment 162986 [details]
Patch

Attachment 162986 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/13794495

[Peter Beverloo (cr-android ews)]

Comment on attachment 162986 [details]
Patch

Attachment 162986 [details] did not pass cr-android-ews (chromium-android):
Output: http://queues.webkit.org/results/13788812

[Early Warning System Bot]

Comment on attachment 162986 [details]
Patch

Attachment 162986 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13794496

[WebKit Review Bot]

Comment on attachment 162986 [details]
Patch

Attachment 162986 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13797410

[Nico Weber]

Created attachment 162988 [details]
Patch

[Dirk Schulze]

Comment on attachment 162988 [details]
Patch

Well, wait. Instead of fixing the bug you surround it with a flag? Don't think that this is a good idea.

[Nico Weber]

(In reply to comment #17)
> (From update of attachment 162988 [details])
> Well, wait. Instead of fixing the bug you surround it with a flag? Don't think that this is a good idea.

The fix was in the first patch, which was rejected as far as I understand due to lack of test (which is reasonable).

So, here's the background: We update clang in chromium every 1-2 weeks. Every now and then, clang has a new warning, and we need to make sure all our code compiles with that new warning (we build with -Werror). If the warning is noisy, we just disable it globally, but since the clang people have a very reasonable warning policy, this happens rarely.

This specific warning warns when someone writes `if (f(a, b, c > 0))` if they meant to write `if (f(a, b, c) > 0)` (see http://permalink.gmane.org/gmane.comp.compilers.clang.scm/55995 for more information if you're curious). This is exactly what happened here, see the ChangeLog entry in the first patch for more information, where I looked at the history of this file.

I had hoped that the original author of this code (anttik) would chime in. Maybe he's on vacation, or busy with other things.

I'm not familiar with the svg code, and I currently don't have time to work on a test. (Someone familiar with the svg code mentioned earlier on this bug that it's not obvious how to test this.) I do want to update clang in chromium though, so I need some way forward. I could disable this new warning for all of chrome, but then it won't catch new bugs that it could prevent. I could disable it just for webkit's svg gyp target, but then it won't prevent new bugs it could've prevented in the svg code (also, if someone who didn't use gyp used a new clang, they'd get this warning, and they might even fix this the wrong way -- see e.g. bug 96093). So suppressing the warning just locally in this file seems like the best way forward to me.

Let me know if you still disagree, then I'll disable it somewhere further up the stack.

[Dirk Schulze]

Comment on attachment 162988 [details]
Patch

(In reply to comment #18)
> Let me know if you still disagree, then I'll disable it somewhere further up the stack.

I just asked you to address the code suggestions from stephen. Adding ifdef's is not a good solution at all, especially  when you can easily fix it. r- because of ifdef instead of fixing the problem.

It is ok not to add a test, since it is build system related IMO.

[Nico Weber]

Created attachment 163260 [details]
Patch

[Nico Weber]

Back to patch set 1 then. As mentioned above, I don't think Stephen's suggestion make the code better.

I'm happy to do the changes he's asking for in a follow-up patch if he really likes to see them. In this patch, I just want to fix a compiler warning and put the code in the form the original author intended. (See the ChangeLog entry.)

[Dirk Schulze]

Our project goal is to clean up code that we touch. The second code suggestion from Stephen looks good to me.

[Nico Weber]

It looks worse to me. I'm happy to discuss this on irc (thakis).

I'm about to globally disable the warning instead: http://codereview.chromium.org/10917180/

[James Robinson]

Comment on attachment 163260 [details]
Patch

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

> Source/WebCore/svg/animation/SVGSMILElement.cpp:969
>          repeat = static_cast<unsigned>(repeatingDuration.value() / simpleDuration.value());

Due to this line it's fairly safe to assume simpleDuration.value() is not zero...

> Source/WebCore/svg/animation/SVGSMILElement.cpp:-970
> -        if (fmod(repeatingDuration.value(), !simpleDuration.value()))

so !simpleDuration.value() simplifies to false which means this can be simplified to:

if (fmodf(repeatingDuration.value(), 0))

which according to man fmodf raises a floating point exception and returns NaN

so I'm guessing this code is somehow not reachable at all in practice. perhaps we can just nuke this whole if

[Stephen Chenney]

(In reply to comment #24)
> (From update of attachment 163260 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=163260&action=review
> 
> > Source/WebCore/svg/animation/SVGSMILElement.cpp:969
> >          repeat = static_cast<unsigned>(repeatingDuration.value() / simpleDuration.value());
> 
> Due to this line it's fairly safe to assume simpleDuration.value() is not zero...

simpleDuration.value() is not zero. That is checked for at the beginning of the method.

> > Source/WebCore/svg/animation/SVGSMILElement.cpp:-970
> > -        if (fmod(repeatingDuration.value(), !simpleDuration.value()))
> 
> so !simpleDuration.value() simplifies to false which means this can be simplified to:
> 
> if (fmodf(repeatingDuration.value(), 0))
> 
> which according to man fmodf raises a floating point exception and returns NaN
> 
> so I'm guessing this code is somehow not reachable at all in practice. perhaps we can just nuke this whole if

I believe that the code is intended to catch the case where the repeatingDuration is an exact multiple of the simpleDuration, in which case we need to add one to the repeat count. It might be possible to hit it with a test - I can try in my quiet moments today.

[Nico Weber]

Created attachment 171036 [details]
Patch

[James Robinson]

Comment on attachment 171036 [details]
Patch

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

> Source/WebCore/svg/animation/SVGSMILElement.cpp:1006
>          repeat = static_cast<unsigned>(repeatingDuration.value() / simpleDuration.value());
> -        if (fmod(repeatingDuration.value(), !simpleDuration.value()))
> -            repeat--;
> +        repeat--;

fold into the line above?

[Nico Weber]

(In reply to comment #27)
> (From update of attachment 171036 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=171036&action=review
> 
> > Source/WebCore/svg/animation/SVGSMILElement.cpp:1006
> >          repeat = static_cast<unsigned>(repeatingDuration.value() / simpleDuration.value());
> > -        if (fmod(repeatingDuration.value(), !simpleDuration.value()))
> > -            repeat--;
> > +        repeat--;
> 
> fold into the line above?

Done. (Didn't do it for somewhat nicer blame output, but meh)

[Nico Weber]

Created attachment 171037 [details]
Patch

[Dirk Schulze]

Comment on attachment 171037 [details]
Patch

r=me

[Nico Weber]

Comment on attachment 171037 [details]
Patch

Thanks!

[WebKit Review Bot]

Comment on attachment 171037 [details]
Patch

Clearing flags on attachment: 171037

Committed r132715: <http://trac.webkit.org/changeset/132715>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.