Bug 159173

Summary: [mac] LayoutTest fast/css/ancestor-of-hovered-element-detached.html is flaky
Product: WebKit Reporter: Ryan Haddad <ryanhaddad>
Component: New BugsAssignee: Benjamin Poulain <benjamin>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, benjamin, commit-queue, simon.fraser, webkit
Priority: P2    
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=158340
Attachments:
Description Flags
Patch none

Description Ryan Haddad 2016-06-27 15:00:47 PDT
LayoutTest fast/css/ancestor-of-hovered-element-detached.html is flaky

Most recent failure:
<https://build.webkit.org/builders/Apple%20El%20Capitan%20Release%20WK2%20(Tests)/builds/7163>
Flakiness dashboard:
<https://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=fast%2Fcss%2Fancestor-of-hovered-element-detached.html>

--- /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/fast/css/ancestor-of-hovered-element-detached-expected.txt
+++ /Volumes/Data/slave/elcapitan-release-tests-wk2/build/layout-test-results/fast/css/ancestor-of-hovered-element-detached-actual.txt
@@ -8,8 +8,8 @@
 PASS elementsWithHoverStyle() is ["html", "body", "prime-ancestor", "group", "element-to-remove", "target"]
 PASS elementsMatchingHoverSelector() is ["html", "body", "prime-ancestor", "group", "element-to-remove", "target"]
 Removing the renderer of #element-to-remove
-PASS elementsWithHoverStyle() is ["html", "body", "prime-ancestor", "interceptor"]
-PASS elementsMatchingHoverSelector() is ["html", "body", "prime-ancestor", "interceptor"]
+FAIL elementsWithHoverStyle() should be html,body,prime-ancestor,interceptor. Was html,body,prime-ancestor,group,element-to-remove,target.
+FAIL elementsMatchingHoverSelector() should be html,body,prime-ancestor,interceptor. Was html,body,prime-ancestor,group,element-to-remove,target.
 PASS successfullyParsed is true
 
 TEST COMPLETE
Comment 1 Ryan Haddad 2016-06-27 15:01:59 PDT
This was added with <http://trac.webkit.org/changeset/202324>
Comment 2 Alexey Proskuryakov 2016-07-01 23:46:49 PDT
Benjamin, do you expect to take a look at this? If not, can we roll out r202324?
Comment 3 Ryan Haddad 2016-07-12 16:22:01 PDT
Marked test as flaky on mac in https://trac.webkit.org/changeset/203136
Comment 4 Benjamin Poulain 2016-07-15 20:14:14 PDT
Created attachment 283835 [details]
Patch
Comment 5 Alexey Proskuryakov 2016-07-15 20:48:52 PDT
Comment on attachment 283835 [details]
Patch

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

> LayoutTests/fast/css/ancestor-of-hovered-element-removed.html:96
> -    }, 125);
> +    }, 17);

Interesting.

I usually say that no timeout between 0ms and 5000ms makes sense in layout tests, because of how much contention for CPU resources there is. Given your explanation, maybe 17ms is right here!
Comment 6 Benjamin Poulain 2016-07-15 20:55:31 PDT
Comment on attachment 283835 [details]
Patch

(In reply to comment #5)
> Comment on attachment 283835 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=283835&action=review
> 
> > LayoutTests/fast/css/ancestor-of-hovered-element-removed.html:96
> > -    }, 125);
> > +    }, 17);
> 
> Interesting.
> 
> I usually say that no timeout between 0ms and 5000ms makes sense in layout
> tests, because of how much contention for CPU resources there is. Given your
> explanation, maybe 17ms is right here!

Thanks for the review Alexey.

In theory we should not need a timer for :hover. We should keep a dirty flag somewhere and update as needed like the other style updates.

In practice, our code for :hover and :active is in a pretty bad shape. The only guarantee right now is that we'll paint it right.

Our test coverage is also bad. It is not the right time to attempt larger changes.
Comment 7 WebKit Commit Bot 2016-07-15 21:18:12 PDT
Comment on attachment 283835 [details]
Patch

Clearing flags on attachment: 283835

Committed r203321: <http://trac.webkit.org/changeset/203321>
Comment 8 WebKit Commit Bot 2016-07-15 21:18:16 PDT
All reviewed patches have been landed.  Closing bug.