Bug 108184 – ASSERT(time.isFinite()) in SVGSMILElement::createInstanceTimesFromSyncbase

• Component: SVG
• Status: RESOLVED FIXED

[Renata Hodovan]

During SVG fuzzing I've found the following assert:

Program received signal SIGSEGV, Segmentation fault.
0x00007ffff5993f4f in WebCore::SVGSMILElement::createInstanceTimesFromSyncbase (this=0xae07a0, syncbase=0xae07a0)
    at /home/reni/repos/webkit2/Source/WebCore/svg/animation/SVGSMILElement.cpp:1156
1156	            ASSERT(time.isFinite());


Test case attached.

The problem occures if the begin parameter depends on the set object itself.

[Renata Hodovan]

Created attachment 185224 [details]
Test to reproduce

[Tamas Gergely]

Created attachment 219638 [details]
patch, 1st try

[Philip Rogers]

Comment on attachment 219638 [details]
patch, 1st try

I don't think this is the right approach because it will break valid self references. For example:
<rect width="100" height="100" fill="green">
  <animate id="fri" attributeName="x" begin="0s; fri.end + 3s" from="0" to="400" dur="5s"/>
</rect>

I think we probably need to remove the assertion and not call addBeginTime/addEndTime if the time is not finite.

[Tamas Gergely]

Created attachment 220818 [details]
next try

Assert is replaced with a check that prevents adding indefinite times.

[Philip Rogers]

Comment on attachment 220818 [details]
next try

Thanks for updating this. I think your new approach looks good.

Not required, but it would be nice to add this testcase so we don't accidentally break this in the future:
<rect width="100" height="100" fill="green">
  <animate id="fri" attributeName="x" begin="0s; fri.end + 3s" from="0" to="400" dur="5s"/>
</rect>
(you would need to clean this up and format it like the other animation tests)

[Tamas Gergely]

(In reply to comment #5)
> (From update of attachment 220818 [details])
> Thanks for updating this. I think your new approach looks good.
> 
> Not required, but it would be nice to add this testcase so we don't accidentally break this in the future:
> <rect width="100" height="100" fill="green">
>   <animate id="fri" attributeName="x" begin="0s; fri.end + 3s" from="0" to="400" dur="5s"/>
> </rect>
> (you would need to clean this up and format it like the other animation tests)

Thanks. I'll update the patch with the test case.

[Tamas Gergely]

Created attachment 221344 [details]
patch with test

[Philip Rogers]

Comment on attachment 221344 [details]
patch with test

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

Almost there! Just a few minor issues with the test.

> LayoutTests/svg/animations/smil-syncbase-self-dependency.svg:13
> +        if (window.testRunner) {

I'm confused... this will fire and stop the test before the settimeouts below even trigger.

> LayoutTests/svg/animations/smil-syncbase-self-dependency.svg:18
> +        setTimeout('displayMessage()', 500);

This timeout is much too large. Can you try either 0 or, worst case, 30-40? You can use document.documentElement.setCurrentTime to set the current time so you don't have to wait for the animation to advance.

We have 20,000 tests and we try very hard to keep them as fast as possible. Because this test will exist on the tree for years to come, it's important to ensure it's as fast as we can make it.

> LayoutTests/svg/animations/smil-syncbase-self-dependency.svg:21
> +            setTimeout('testRunner.notifyDone()', 600);

Same here

[Tamas Gergely]

(In reply to comment #8)
> (From update of attachment 221344 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221344&action=review
> 
> > LayoutTests/svg/animations/smil-syncbase-self-dependency.svg:13
> > +        if (window.testRunner) {
> 
> I'm confused... this will fire and stop the test before the settimeouts below even trigger.

I cannot see the difference to other tests. Most of them starts with this check. And it works for me only this way, if I leave this block it would fail. Or, am I wrong?

> > LayoutTests/svg/animations/smil-syncbase-self-dependency.svg:18
> > +        setTimeout('displayMessage()', 500);
> 
> This timeout is much too large. Can you try either 0 or, worst case, 30-40? You can use document.documentElement.setCurrentTime to set the current time so you don't have to wait for the animation to advance.
> 
> We have 20,000 tests and we try very hard to keep them as fast as possible. Because this test will exist on the tree for years to come, it's important to ensure it's as fast as we can make it.
> 
> > LayoutTests/svg/animations/smil-syncbase-self-dependency.svg:21
> > +            setTimeout('testRunner.notifyDone()', 600);
> 
> Same here

Less than 0.02s as animation duration and less than 30ms as timeout did not work for me. Using setCurrentTime in this situation also fails (see bug 127174).

[Tamas Gergely]

Created attachment 221466 [details]
Tests with short timeout.

If these timings are still not small enough, then we should omit the tests.

[Tamas Gergely]

Created attachment 221467 [details]
Corrected patch

Sorry, sent only a partial diff previously.

[Philip Rogers]

Comment on attachment 221467 [details]
Corrected patch

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

This looks good to me. One minor issue with the test.

> LayoutTests/svg/animations/smil-syncbase-self-dependency.svg:14
> +            document.getElementById("label").textContent = "SMILElement animation time self-dependency is" +

Why not end the test here?

You can do:
document.write("SMILElement animation ... etc");
if (testRunner)
    testRunner.notifyDone();

[Tamas Gergely]

(In reply to comment #12)
> (From update of attachment 221467 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=221467&action=review
> 
> This looks good to me. One minor issue with the test.
> 
> > LayoutTests/svg/animations/smil-syncbase-self-dependency.svg:14
> > +            document.getElementById("label").textContent = "SMILElement animation time self-dependency is" +
> 
> Why not end the test here?
> 
> You can do:
> document.write("SMILElement animation ... etc");
> if (testRunner)
>     testRunner.notifyDone();

Here we want to test whether self-dependency is correctly handled. If you just write out a fixed string, it will test whether WK crashes in the case of self-dependency, but it will not find situations where the self-dependency is silently dropped (e.g. my first try).

[WebKit Commit Bot]

Comment on attachment 221467 [details]
Corrected patch

Clearing flags on attachment: 221467

Committed r162459: <http://trac.webkit.org/changeset/162459>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Philip Rogers]

(In reply to comment #15)
> All reviewed patches have been landed.  Closing bug.

I'm very sorry but I made a mistake when reviewing this. We should have landed a test that crashes before this patch and does not crash after this patch. I created this testcase when merging this patch to Chrome and it's available at: https://codereview.chromium.org/144333002

(LayoutTests/svg/animations/self-dependency-crash.html and LayoutTests/svg/animations/self-dependency-crash-expected.html).

Would you be willing to land this test in another patch? If not, I'd be happy to do it for you.

[Tamas Gergely]

(In reply to comment #16)
> (In reply to comment #15)
> > All reviewed patches have been landed.  Closing bug.
> 
> I'm very sorry but I made a mistake when reviewing this. We should have landed a test that crashes before this patch and does not crash after this patch. I created this testcase when merging this patch to Chrome and it's available at: https://codereview.chromium.org/144333002
> 
> (LayoutTests/svg/animations/self-dependency-crash.html and LayoutTests/svg/animations/self-dependency-crash-expected.html).
> 
> Would you be willing to land this test in another patch? If not, I'd be happy to do it for you.

I'll do it.

[Alexey Proskuryakov]

The test added here is very flaky on all bots: <http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=svg%2Fanimations%2Fsmil-syncbase-self-dependency.svg>

What would be the right way to handle this? Should we roll the fix out?

[Tamas Gergely]

(In reply to comment #18)
> The test added here is very flaky on all bots: <http://webkit-test-results.appspot.com/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=svg%2Fanimations%2Fsmil-syncbase-self-dependency.svg>
> 
> What would be the right way to handle this? Should we roll the fix out?

This is probably caused by the short time intervals the test uses. (For me these were the minimal time settings that worked correctly.) I'll add a test case that only checks the original crash and contains no such timing issues. Should I also remove these flaky tests in that patch? Or the policy is to roll out the fix and land it again with the other test case?

[Alexey Proskuryakov]

The important thing is to address flaky and failing tests quickly (possibly by rolling out or disabling the tests), and then to get to the bottom of what went wrong.

Since your plan is to remove this test, I went ahead and deleted the flaky one in <http://trac.webkit.org/r162632>.

[Tamas Gergely]

Created attachment 222083 [details]
Test

Crash test.

[WebKit Commit Bot]

Comment on attachment 222083 [details]
Test

Clearing flags on attachment: 222083

Committed r162753: <http://trac.webkit.org/changeset/162753>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Renata Hodovan]

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

[Ahmad Saleem]

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