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.
Created attachment 185224 [details] Test to reproduce
Created attachment 219638 [details] patch, 1st try
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.
Created attachment 220818 [details] next try Assert is replaced with a check that prevents adding indefinite times.
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)
(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.
Created attachment 221344 [details] patch with test
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
(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).
Created attachment 221466 [details] Tests with short timeout. If these timings are still not small enough, then we should omit the tests.
Created attachment 221467 [details] Corrected patch Sorry, sent only a partial diff previously.
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();
(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 on attachment 221467 [details] Corrected patch Clearing flags on attachment: 221467 Committed r162459: <http://trac.webkit.org/changeset/162459>
All reviewed patches have been landed. Closing bug.
(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.
(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.
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?
(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?
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>.
Created attachment 222083 [details] Test Crash test.
Comment on attachment 222083 [details] Test Clearing flags on attachment: 222083 Committed r162753: <http://trac.webkit.org/changeset/162753>
*** Bug 98059 has been marked as a duplicate of this bug. ***
*** Bug 78075 has been marked as a duplicate of this bug. ***