Bug 189063 - REGRESSION(r235408): GTK bots exiting early
Summary: REGRESSION(r235408): GTK bots exiting early
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKitGTK (show other bugs)
Version: Other
Hardware: PC Linux
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords:
: 189079 (view as bug list)
Depends on:
Blocks:
 
Reported: 2018-08-28 15:32 PDT by Michael Catanzaro
Modified: 2018-09-05 00:09 PDT (History)
8 users (show)

See Also:


Attachments
Patch (3.73 KB, patch)
2018-09-04 15:03 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Michael Catanzaro 2018-08-28 15:32:44 PDT
The GTK bots are exiting early. I see many timeouts in certain animation and compositing tests, CSS3 tests, and editing tests:

https://build.webkit.org/results/GTK%20Linux%2064-bit%20Release%20(Tests)/r235437%20(7940)/results.html

The regression was introduced between r235407-r235410, inclusive. Most likely r235408 or r235409.
Comment 1 Michael Catanzaro 2018-08-28 15:35:31 PDT
There are also ten failing webkitperl tests, which is unusual as these never fail. These may or may not be related, I don't know:

Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl .................................. Experimental keys on scalar is now forbidden at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262/Runner.pm line 929.
Compilation failed in require at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262-runner line 45.
BEGIN failed--compilation aborted at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262-runner line 45.

Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl .................................. 1/13 
#   Failed test 'test262 test failed, ignore expectations (exit code: 1)'
#   at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 136.
#          got: '255'
#     expected: '1'

#   Failed test 'test262 test failed, ignore expectations (new failures: 2)'
#   at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 142.
#          got: '0'
#     expected: '2'
Experimental keys on scalar is now forbidden at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262/Runner.pm line 929.
Compilation failed in require at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262-runner line 45.
BEGIN failed--compilation aborted at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262-runner line 45.

#   Failed test 'test262 test passed, ignore expectations (exit code: 0)'
#   at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 136.
#          got: '255'
#     expected: '0'
Experimental keys on scalar is now forbidden at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262/Runner.pm line 929.
Compilation failed in require at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262-runner line 45.
BEGIN failed--compilation aborted at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262-runner line 45.

#   Failed test 'test262 tests newly failed (exit code: 1)'
#   at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 136.
#          got: '255'
#     expected: '1'

#   Failed test 'test262 tests newly failed (new failures: 2)'
#   at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 142.
#          got: '0'
#     expected: '2'
Experimental keys on scalar is now forbidden at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262/Runner.pm line 929.
Compilation failed in require at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262-runner line 45.
BEGIN failed--compilation aborted at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262-runner line 45.

#   Failed test 'test262 tests newly passed (exit code: 0)'
#   at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 136.
#          got: '255'
#     expected: '0'
Experimental keys on scalar is now forbidden at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262/Runner.pm line 929.
Compilation failed in require at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262-runner line 45.
BEGIN failed--compilation aborted at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262-runner line 45.

#   Failed test 'test262 tests fails, expected failure (exit code: 0)'
#   at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 136.
#          got: '255'
#     expected: '0'
Experimental keys on scalar is now forbidden at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262/Runner.pm line 929.
Compilation failed in require at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262-runner line 45.
BEGIN failed--compilation aborted at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262-runner line 45.

#   Failed test 'test262 tests fails, with unexpected error string (exit code: 1)'
#   at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 136.
#          got: '255'
#     expected: '1'

#   Failed test 'test262 tests fails, with unexpected error string (new failures: 2)'
#   at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 142.
#          got: '0'
#     expected: '2'
Experimental keys on scalar is now forbidden at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262/Runner.pm line 929.
Compilation failed in require at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262-runner line 45.
BEGIN failed--compilation aborted at /home/buildbot/wpe/wpe-linux-64-release-tests/build/Tools/Scripts/test262-runner line 45.

#   Failed test 'expectations yaml file format'
#   at Tools/Scripts/webkitperl/test262_unittest/test262-runner-tests.pl line 157.
# Looks like you failed 10 tests of 13.
Comment 2 Michael Catanzaro 2018-08-28 15:35:56 PDT
(In reply to Michael Catanzaro from comment #1)
> There are also ten failing webkitperl tests, which is unusual as these never
> fail. These may or may not be related, I don't know:

Well, they started failing in the same range of commits.
Comment 3 Simon Fraser (smfr) 2018-08-28 15:46:53 PDT
You're sure it wasn't https://trac.webkit.org/changeset/235399/webkit ?
Comment 4 Simon Fraser (smfr) 2018-08-28 15:52:56 PDT
The perl tests started failing here: https://trac.webkit.org/changeset/235359/webkit
Comment 5 Michael Catanzaro 2018-08-28 16:10:39 PDT
(In reply to Simon Fraser (smfr) from comment #3)
> You're sure it wasn't https://trac.webkit.org/changeset/235399/webkit ?

Hm, that commit is mighty suspicious, but we do have successful test runs at r235402 and r235406, on two different bots. So I don't think so, not unless old versions of the test runner are somehow being used by mistake.

(In reply to Simon Fraser (smfr) from comment #4)
> The perl tests started failing here:
> https://trac.webkit.org/changeset/235359/webkit

Ooops, I misread the test results page. So that is surely unrelated. (Do you want to report the bug for it, or shall I?)
Comment 6 Michael Catanzaro 2018-08-28 20:03:43 PDT
(In reply to Michael Catanzaro from comment #5)
> (In reply to Simon Fraser (smfr) from comment #3)
> > You're sure it wasn't https://trac.webkit.org/changeset/235399/webkit ?
> 
> Hm, that commit is mighty suspicious, but we do have successful test runs at
> r235402 and r235406, on two different bots. So I don't think so, not unless
> old versions of the test runner are somehow being used by mistake.

I've tested r235406 locally and verified it's good. Tested r235408 and verified it's bad. The only change in r235407 is guarded by PLATFORM(COCOA). So we can pin this on r235408.

The tests are printing on stderr:

FAIL: TestControllerRunLoop timed out.

Looking at the commit, I don't immediately notice anything problematic, so I'll need to attempt to debug why some particular test is stalling.

I did spot one unrelated bug in the commit, in Tools/DumpRenderTree/win/DumpRenderTree.cpp:

    // DumpRenderTree does not support checking for abandonded documents.
    if (!strcmp("#CHECK FOR ABANDONED DOCUMENTS", command)) {

I noticed that "abandonded" was misspelled. Then I noticed that the command here is different than everywhere else. Should probably be:

    // DumpRenderTree does not support checking for world leaks.
    if (!strcmp("#CHECK FOR WORLD LEAKS", command)) {
Comment 7 Michael Catanzaro 2018-08-29 08:02:16 PDT
*** Bug 189079 has been marked as a duplicate of this bug. ***
Comment 8 Simon Fraser (smfr) 2018-08-29 13:00:25 PDT
Happy to help out if we can be on irc at the same time.
Comment 9 Michael Catanzaro 2018-08-31 12:53:31 PDT
(In reply to Simon Fraser (smfr) from comment #8)
> Happy to help out if we can be on irc at the same time.

Thanks... it's been a busy week and next week will be too, so probably not anytime soon unfortunately.

> I did spot one unrelated bug in the commit, in
> Tools/DumpRenderTree/win/DumpRenderTree.cpp:
> 
>     // DumpRenderTree does not support checking for abandonded documents.
>     if (!strcmp("#CHECK FOR ABANDONED DOCUMENTS", command)) {
> 
> I noticed that "abandonded" was misspelled. Then I noticed that the command
> here is different than everywhere else. Should probably be:
> 
>     // DumpRenderTree does not support checking for world leaks.
>     if (!strcmp("#CHECK FOR WORLD LEAKS", command)) {

Don't forget about this. ^
Comment 10 Simon Fraser (smfr) 2018-08-31 13:01:39 PDT
(In reply to Michael Catanzaro from comment #9)
> (In reply to Simon Fraser (smfr) from comment #8)
> > Happy to help out if we can be on irc at the same time.
> 
> Thanks... it's been a busy week and next week will be too, so probably not
> anytime soon unfortunately.
> 
> > I did spot one unrelated bug in the commit, in
> > Tools/DumpRenderTree/win/DumpRenderTree.cpp:
> > 
> >     // DumpRenderTree does not support checking for abandonded documents.
> >     if (!strcmp("#CHECK FOR ABANDONED DOCUMENTS", command)) {
> > 
> > I noticed that "abandonded" was misspelled. Then I noticed that the command
> > here is different than everywhere else. Should probably be:
> > 
> >     // DumpRenderTree does not support checking for world leaks.
> >     if (!strcmp("#CHECK FOR WORLD LEAKS", command)) {
> 
> Don't forget about this. ^

Fixed in r235408.
Comment 11 Claudio Saavedra 2018-09-04 06:30:03 PDT
I debugged this a bit today, have some findings that might shed some light.

- This only happens if you run a series of tests sharing the DumpRendererTree. If you use --run-singly, the tests pass. So it probably has to be with the remainder state of DumpRendererTree.

- I noticed that there's a 20_s in the AsyncTask that is added for the world leaks in the offending commit. I'm not familiar with this but I'm guessing that this is the timeout for the task. The timeout for the tests is 5 s, AFAIR, so it's probable that the world leaks task is timing out. I increased the tests timeout when running them locally to 20s, and then the tests pass. Similarly, if I change the 20_s to 5_s, the tests also pass.

- But what I don't understand is why this task is being called at all? Isn't it so that --world-leaks needs to be called explicitly for the check to run? What I see is that this is done unconditionally in willDestroyWebView(). Is this intended?
Comment 12 Claudio Saavedra 2018-09-04 06:47:33 PDT
(In reply to Claudio Saavedra from comment #11)

> What I see is that this is done unconditionally in willDestroyWebView(). Is
> this intended?

Apologies in advance if this is gross, but just not doing this unconditional call also allows the tests to pass without timing out.
Comment 13 Simon Fraser (smfr) 2018-09-04 08:41:12 PDT
(In reply to Claudio Saavedra from comment #12)
> (In reply to Claudio Saavedra from comment #11)
> 
> > What I see is that this is done unconditionally in willDestroyWebView(). Is
> > this intended?
> 
> Apologies in advance if this is gross, but just not doing this unconditional
> call also allows the tests to pass without timing out.

Oh, that call in DestroyWebView should not be unconditional!
Comment 14 Simon Fraser (smfr) 2018-09-04 15:03:39 PDT
Created attachment 348851 [details]
Patch
Comment 15 WebKit Commit Bot 2018-09-04 16:45:13 PDT
Comment on attachment 348851 [details]
Patch

Clearing flags on attachment: 348851

Committed r235646: <https://trac.webkit.org/changeset/235646>
Comment 16 WebKit Commit Bot 2018-09-04 16:45:15 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Claudio Saavedra 2018-09-05 00:09:42 PDT
Glad to have helped!