Bug 173178

Summary: svg/animations/svglength-element-removed-crash.svg is flaky
Product: WebKit Reporter: Matt Lewis <jlewis3>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED DUPLICATE    
Severity: Normal CC: ap, fpizlo, ryanhaddad, sabouhallawa
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=191047
https://bugs.webkit.org/show_bug.cgi?id=191664

Description Matt Lewis 2017-06-09 12:53:36 PDT
svg/animations/svglength-element-removed-crash.svg is a flaky failure but is becoming more consistent in failing.

Dashboard shows the failures happening on iOS Simulator Release:
https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=svg%2Fanimations%2Fsvglength-element-removed-crash.svg

https://build.webkit.org/results/Apple%20iOS%2010%20Simulator%20Release%20WK2%20(Tests)/r217993%20(2116)/results.html
https://build.webkit.org/builders/Apple%20iOS%2010%20Simulator%20Release%20WK2%20(Tests)/builds/2085


I was able to reproduce the issue locally with:
run-webkit-tests svg/animations/svglength-element-removed-crash.svg --iterations=100 --no-retry-failure --ios-simulator

I was also able to narrow down the failure to Revision r217944 using same testing method
https://trac.webkit.org/changeset/217944/webkit

Diff:
--- /Volumes/Data/slave/ios-simulator-10-release-tests-wk2/build/layout-test-results/svg/animations/svglength-element-removed-crash-expected.txt
+++ /Volumes/Data/slave/ios-simulator-10-release-tests-wk2/build/layout-test-results/svg/animations/svglength-element-removed-crash-actual.txt
@@ -1 +1 @@
-PASS x = 10
+FAIL: -20 extra live node(s) x = 10
Comment 1 Ryan Haddad 2017-06-09 14:02:34 PDT
This is affecting EWS results, so we should take care of this quickly or roll out the offending change.
Comment 2 Alexey Proskuryakov 2017-06-09 14:10:45 PDT
Ryan, can you disable the test for now? That fix was definitely addressing a JSC bug and test flakiness, while this may be just a bad test.

But it is somewhat surprising.
Comment 3 Filip Pizlo 2017-06-09 14:25:27 PDT
(In reply to Alexey Proskuryakov from comment #2)
> Ryan, can you disable the test for now? That fix was definitely addressing a
> JSC bug and test flakiness, while this may be just a bad test.
> 
> But it is somewhat surprising.

Maybe this SVG test was inadvertently taking advantage of LLInt's old wrong behavior.  It would flake, because the wrong behavior was in a LLInt optimization, so sometimes you wouldn't get the wrong behavior.

Specifically, what happens if you have both a custom property and an element called "foo".  What does document.foo do?  Some what the property, some want the element.  Correct behavior is to give the element, LLInt would sometimes give the property.

I agree that it's worth skipping the SVG test.  It's probably not too hard to fix, if I'm right about what's going on.
Comment 4 Matt Lewis 2017-06-09 14:27:13 PDT
Skipped test:
https://trac.webkit.org/changeset/218018/webkit/
Comment 5 Said Abou-Hallawa 2018-11-07 13:49:52 PST
This should be fixed by removing the SVG tear-off object. https://bugs.webkit.org/show_bug.cgi?id=191237

*** This bug has been marked as a duplicate of bug 191237 ***