Bug 167538 - [css-grid] LayoutTest fast/css-grid-layout/grid-simplified-layout-positioned.html is a flaky failure
Summary: [css-grid] LayoutTest fast/css-grid-layout/grid-simplified-layout-positioned....
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: Other
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Manuel Rego Casasnovas
URL:
Keywords:
Depends on:
Blocks: 60731
  Show dependency treegraph
 
Reported: 2017-01-27 17:32 PST by Ryan Haddad
Modified: 2017-04-11 02:20 PDT (History)
8 users (show)

See Also:


Attachments
Patch (3.28 KB, patch)
2017-02-10 03:41 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Patch for landing (3.42 KB, patch)
2017-02-13 02:17 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (930.15 KB, application/zip)
2017-02-13 03:18 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (877.43 KB, application/zip)
2017-02-13 03:23 PST, Build Bot
no flags Details
Archive of layout-test-results from ews117 for mac-elcapitan (1.67 MB, application/zip)
2017-02-13 03:31 PST, Build Bot
no flags Details
New version with timeout (3.52 KB, patch)
2017-02-13 06:16 PST, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews101 for mac-elcapitan (1.18 MB, application/zip)
2017-02-13 07:05 PST, Build Bot
no flags Details
Archive of layout-test-results from ews106 for mac-elcapitan-wk2 (911.46 KB, application/zip)
2017-02-13 07:21 PST, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-elcapitan (1.67 MB, application/zip)
2017-02-13 07:27 PST, Build Bot
no flags Details
Patch for landing (3.77 KB, patch)
2017-04-11 00:57 PDT, Manuel Rego Casasnovas
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Ryan Haddad 2017-01-27 17:32:43 PST
The images show that the test is expecting the cursor to be present in the text field, but it is missing.
Comment 2 Ryan Haddad 2017-02-09 15:20:35 PST
Marked test as flaky in http://trac.webkit.org/projects/webkit/changeset/211994
Comment 3 Ryan Haddad 2017-02-09 15:21:05 PST
This does not appear to be a recent regression. CC'ing test author.
Comment 4 Manuel Rego Casasnovas 2017-02-10 02:30:25 PST
It seems that the only issue is that the caret appears or not,
the test is not checking anything regarding the caret.
The only important thing was that with "autofocus" we were performing a simplified layout
that was crashing before the patch that added the test (bug #163450).

I'll modify the tests trying to get rid of this issue with the caret.
Comment 5 Manuel Rego Casasnovas 2017-02-10 03:41:43 PST
Created attachment 301146 [details]
Patch

Try to fix the test, let's see what EWS has to say.
Comment 6 Michael Catanzaro 2017-02-11 19:55:18 PST
Hm, I wonder if there is a more idiomatic way to force the cursor to not appear? Is the blur a pattern used by many tests to avoid this? When reading the test, I would be confused as to why the blur is present, since there's no comment.

I'm kinda surprised that our test infrastructure doesn't have the ability to add two different expectations and allow the test to match either. Blinky cursors are a very common problem in automated tests and most test environments have a standard mechanism for dealing with them.
Comment 7 Darin Adler 2017-02-12 01:34:47 PST
Comment on attachment 301146 [details]
Patch

I am tempted to mark this as reviewed, but I am wondering: Is this still an effective test? Would the call to blur have side-stepped the crash even before we fixed the bug?
Comment 8 Darin Adler 2017-02-12 01:35:49 PST
Another possibility would be to enhance the test runners so that they understand the flashing caret and always take snapshots with the caret visible.
Comment 9 Darin Adler 2017-02-12 01:36:12 PST
Or always with the caret invisible, or any other sensible rule.
Comment 10 Darin Adler 2017-02-12 01:37:16 PST
Comment on attachment 301146 [details]
Patch

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

> LayoutTests/fast/css-grid-layout/grid-simplified-layout-positioned-expected.html:15
>  <input autofocus>
> +<script>
> +  document.querySelector("input").blur();
> +</script>

Here in the expected file, maybe we can just omit autofocus instead of calling blur?
Comment 11 Michael Catanzaro 2017-02-12 05:00:01 PST
Comment on attachment 301146 [details]
Patch

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

> LayoutTests/ChangeLog:13
> +        "autofocus" was needed to get the crash, so this change modifies
> +        the test to blur the element, so the caret is not painted anymore.

autofocus was needed to get the crash.
Comment 12 Darin Adler 2017-02-12 10:49:13 PST
(In reply to comment #11)
> autofocus was needed to get the crash.

The expected file doesn’t need to crash. That’s why I suggested omitting it *in the expected file*, not in the test file.
Comment 13 Manuel Rego Casasnovas 2017-02-13 02:17:51 PST
Created attachment 301338 [details]
Patch for landing
Comment 14 Manuel Rego Casasnovas 2017-02-13 02:19:42 PST
Thanks for the reviews.

Uploaded new version for landing adding a comment to explain the "blur()"
and also removing "autofocus" on the "-expected" file.

(In reply to comment #7)
> I am tempted to mark this as reviewed, but I am wondering: Is this still an
> effective test? Would the call to blur have side-stepped the crash even
> before we fixed the bug?

Yeah I've verified that the test is still effective with the blur() part,
as the crash was triggered before.
Comment 15 Build Bot 2017-02-13 03:18:36 PST
Comment on attachment 301338 [details]
Patch for landing

Attachment 301338 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3077149

New failing tests:
fast/css-grid-layout/grid-simplified-layout-positioned.html
Comment 16 Build Bot 2017-02-13 03:18:40 PST
Created attachment 301341 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 17 Build Bot 2017-02-13 03:23:36 PST
Comment on attachment 301338 [details]
Patch for landing

Attachment 301338 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3077154

New failing tests:
fast/css-grid-layout/grid-simplified-layout-positioned.html
Comment 18 Build Bot 2017-02-13 03:23:39 PST
Created attachment 301342 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 19 Build Bot 2017-02-13 03:31:29 PST
Comment on attachment 301338 [details]
Patch for landing

Attachment 301338 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3077158

New failing tests:
fast/css-grid-layout/grid-simplified-layout-positioned.html
Comment 20 Build Bot 2017-02-13 03:31:33 PST
Created attachment 301343 [details]
Archive of layout-test-results from ews117 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews117  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 21 Manuel Rego Casasnovas 2017-02-13 06:16:13 PST
Created attachment 301349 [details]
New version with timeout
Comment 22 Manuel Rego Casasnovas 2017-02-13 06:18:52 PST
Oops, so in my previous version the "blur()" was run before "autofocus",
so it was not useful at all.
Both versions the test and the expected file were showing the caret
(so probably it would be still flacky on Mac.

New version uses setTimeout() on the test file to ensure that blur()
is run after "autofocus". Would you mind to take a new look? Thanks.
Comment 23 Build Bot 2017-02-13 07:05:19 PST
Comment on attachment 301349 [details]
New version with timeout

Attachment 301349 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/3077880

New failing tests:
fast/css-grid-layout/grid-simplified-layout-positioned.html
Comment 24 Build Bot 2017-02-13 07:05:23 PST
Created attachment 301350 [details]
Archive of layout-test-results from ews101 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews101  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 25 Build Bot 2017-02-13 07:21:29 PST
Comment on attachment 301349 [details]
New version with timeout

Attachment 301349 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.webkit.org/results/3077922

New failing tests:
fast/css-grid-layout/grid-simplified-layout-positioned.html
Comment 26 Build Bot 2017-02-13 07:21:34 PST
Created attachment 301354 [details]
Archive of layout-test-results from ews106 for mac-elcapitan-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-elcapitan-wk2  Platform: Mac OS X 10.11.6
Comment 27 Build Bot 2017-02-13 07:27:28 PST
Comment on attachment 301349 [details]
New version with timeout

Attachment 301349 [details] did not pass mac-debug-ews (mac):
Output: http://webkit-queues.webkit.org/results/3077921

New failing tests:
fast/css-grid-layout/grid-simplified-layout-positioned.html
Comment 28 Build Bot 2017-02-13 07:27:32 PST
Created attachment 301355 [details]
Archive of layout-test-results from ews116 for mac-elcapitan

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews116  Port: mac-elcapitan  Platform: Mac OS X 10.11.6
Comment 29 Manuel Rego Casasnovas 2017-02-14 08:05:38 PST
Comment on attachment 301349 [details]
New version with timeout

Oops so this is still not working on Mac.
I'd need to take a deeper look when I find some time, removing from review queue at this point.
Comment 30 Manuel Rego Casasnovas 2017-04-11 00:57:08 PDT
Created attachment 306784 [details]
Patch for landing

Fix test so it waits until the blur() happens, this makets it passes on Mac.
Comment 31 WebKit Commit Bot 2017-04-11 02:20:20 PDT
Comment on attachment 306784 [details]
Patch for landing

Clearing flags on attachment: 306784

Committed r215222: <http://trac.webkit.org/changeset/215222>
Comment 32 WebKit Commit Bot 2017-04-11 02:20:22 PDT
All reviewed patches have been landed.  Closing bug.