Bug 108184 - ASSERT(time.isFinite()) in SVGSMILElement::createInstanceTimesFromSyncbase
Summary: ASSERT(time.isFinite()) in SVGSMILElement::createInstanceTimesFromSyncbase
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
: 98059 (view as bug list)
Depends on:
Blocks: 116980
  Show dependency treegraph
 
Reported: 2013-01-29 05:33 PST by Renata Hodovan
Modified: 2014-04-09 23:28 PDT (History)
12 users (show)

See Also:


Attachments
Test to reproduce (170 bytes, image/svg+xml)
2013-01-29 05:42 PST, Renata Hodovan
no flags Details
patch, 1st try (3.61 KB, patch)
2013-12-19 03:37 PST, Tamas Gergely
pdr: review-
Details | Formatted Diff | Diff
next try (1.67 KB, patch)
2014-01-10 00:57 PST, Tamas Gergely
pdr: review+
Details | Formatted Diff | Diff
patch with test (3.98 KB, patch)
2014-01-16 01:05 PST, Tamas Gergely
pdr: review-
Details | Formatted Diff | Diff
Tests with short timeout. (4.28 KB, patch)
2014-01-17 07:30 PST, Tamas Gergely
no flags Details | Formatted Diff | Diff
Corrected patch (3.98 KB, patch)
2014-01-17 07:42 PST, Tamas Gergely
no flags Details | Formatted Diff | Diff
Test (1.79 KB, patch)
2014-01-23 23:53 PST, Tamas Gergely
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Renata Hodovan 2013-01-29 05:33:21 PST
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.
Comment 1 Renata Hodovan 2013-01-29 05:42:46 PST
Created attachment 185224 [details]
Test to reproduce
Comment 2 Tamas Gergely 2013-12-19 03:37:27 PST
Created attachment 219638 [details]
patch, 1st try
Comment 3 Philip Rogers 2013-12-20 19:04:57 PST
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.
Comment 4 Tamas Gergely 2014-01-10 00:57:42 PST
Created attachment 220818 [details]
next try

Assert is replaced with a check that prevents adding indefinite times.
Comment 5 Philip Rogers 2014-01-10 10:39:35 PST
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)
Comment 6 Tamas Gergely 2014-01-13 07:05:48 PST
(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.
Comment 7 Tamas Gergely 2014-01-16 01:05:57 PST
Created attachment 221344 [details]
patch with test
Comment 8 Philip Rogers 2014-01-16 11:38:53 PST
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
Comment 9 Tamas Gergely 2014-01-17 07:27:55 PST
(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).
Comment 10 Tamas Gergely 2014-01-17 07:30:18 PST
Created attachment 221466 [details]
Tests with short timeout.

If these timings are still not small enough, then we should omit the tests.
Comment 11 Tamas Gergely 2014-01-17 07:42:22 PST
Created attachment 221467 [details]
Corrected patch

Sorry, sent only a partial diff previously.
Comment 12 Philip Rogers 2014-01-17 13:29:02 PST
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();
Comment 13 Tamas Gergely 2014-01-20 01:18:12 PST
(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).
Comment 14 WebKit Commit Bot 2014-01-21 12:12:53 PST
Comment on attachment 221467 [details]
Corrected patch

Clearing flags on attachment: 221467

Committed r162459: <http://trac.webkit.org/changeset/162459>
Comment 15 WebKit Commit Bot 2014-01-21 12:12:57 PST
All reviewed patches have been landed.  Closing bug.
Comment 16 Philip Rogers 2014-01-21 13:59:27 PST
(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.
Comment 17 Tamas Gergely 2014-01-22 06:59:51 PST
(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.
Comment 18 Alexey Proskuryakov 2014-01-22 23:04:43 PST
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?
Comment 19 Tamas Gergely 2014-01-23 01:41:39 PST
(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?
Comment 20 Alexey Proskuryakov 2014-01-23 12:08:01 PST
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>.
Comment 21 Tamas Gergely 2014-01-23 23:53:14 PST
Created attachment 222083 [details]
Test

Crash test.
Comment 22 WebKit Commit Bot 2014-01-24 19:20:59 PST
Comment on attachment 222083 [details]
Test

Clearing flags on attachment: 222083

Committed r162753: <http://trac.webkit.org/changeset/162753>
Comment 23 WebKit Commit Bot 2014-01-24 19:21:03 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Renata Hodovan 2014-04-09 23:28:38 PDT
*** Bug 98059 has been marked as a duplicate of this bug. ***