Bug 100686 – WebKitTestRunner flakily hangs when running pixel tests on Qt

• Status: RESOLVED
• Resolution: FIXED
• Component: Tools / Tests

[Balazs Kelemen]

Well, this is really just a try to catch the issue that makes the pixel tests timing out on the bot sometimes. My theory is that if a forced repaint times out, than it can trigger later when we expect it for a new test and it can confuse our machinery. Actually I could not prove that this is happening becase I could not reproduce the problem locally, but it cannot hurt if we add a check for this.

[Balazs Kelemen]

Created attachment 171276 [details]
Patch

[WebKit Review Bot]

Attachment 171276 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/WebKitTestRunner..." exit_code: 1
Tools/WebKitTestRunner/qt/TestInvocationQt.cpp:75:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
Total errors found: 1 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Balazs Kelemen]

(In reply to comment #2)
> Attachment 171276 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Tools/ChangeLog', u'Tools/WebKitTestRunner..." exit_code: 1
> Tools/WebKitTestRunner/qt/TestInvocationQt.cpp:75:  Weird number of spaces at line-start.  Are you using a 4-space indent?  [whitespace/indent] [3]
> Total errors found: 1 in 2 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

check-webkit-style is complaining here:

fputs("ERROR: unexpected forceRepaintDone callback triggered.\n"
      "       It is likely the result of a timeouted repaint form a previous test.\n", stderr);

So it expects the second line to be indented one space less, but I think it would be ugly.

[Balazs Kelemen]

Comment on attachment 171276 [details]
Patch

According to my testing it will not be enough to eliminate the flakiness.

[Balazs Kelemen]

Let's concentrate on the real bug! As one can see from the history of the bot (http://build.webkit.sed.hu/builders/x86-64%20Linux%20Qt%20Release%20WebKit2%20%28Pixel%20Tests%29) our pixel tests are sometimes start to fail at a given point and than every test is failing, i.e. we timeout waiting for the callback of forceRepaint. It's very bad since in this case the bot have to be stopped manually, otherwise it can run for a whole day. I don't know the exact reason, but what I can tell is that for some reason we don't get the didrenderframe from the web process although it does send it, so we will never push a flushLayerChange operation in the queue of LayerTreeRenderer and never send rendernextframe again and the web process will wait for it forever without rendering a single frame.

As I am not too sure how difficult it would be to find the reason behind not getting the message (note that you need to run all tests about 10 times in average even to reproduce it), currently I am thinking about adding yet another private API for the time being for forcing a flushLayerChange. At the first time we timeout we can use it and I believe it would move us towards for the next test. Now I am going to test whether it works.

[Jocelyn Turcotte]

It's best if we can fix this without blindly pushing fixes that add unneeded complexity until it works. If you can reproduce the problem somehow locally or on a bot without having to push changes, you could try to see if this is a result of Qt triggering a cleanup of the nodes, i.e. if ~ContentsSGNode gets called.

There could be issues around there where the web and UI processes would be waiting for each other.

[Balazs Kelemen]

(In reply to comment #6)
> It's best if we can fix this without blindly pushing fixes that add unneeded complexity until it works. If you can reproduce the problem somehow locally or on a bot without having to push changes, you could try to see if this is a result of Qt triggering a cleanup of the nodes, i.e. if ~ContentsSGNode gets called.
> 

I can reproduce it, about once a day :/
Actually the only thing I can do is to add fprintf(stderr,...)'s here and there and watch the output at of the test that timed out and some previous one.

> There could be issues around there where the web and UI processes would be waiting for each other.

As I understand, ~ContentsSGNode is handled fine, i.e. we throw away all queued update from the LayerTreeRenderer queue, and when we attached to a new node we send a rendernextframe (unconditionally). Ownerships are also seems ok. So I don't see anything that could stop the rendering and syncing loop. First we do an update upon window->show() that makes us send the first rendernextframe, and every didrenderframe triggers a flushLayerChanges in LayerTreeRenderer that also ends in rendernextframe.

Anyway, I'm going to try reproducing digging more info via printf's further.

[Balazs Kelemen]

Created attachment 172580 [details]
Patch

[Balazs Kelemen]

(In reply to comment #8)
> Created an attachment (id=172580) [details]
> Patch

This is also a workaround, but I believe a more acceptable one :)

[WebKit Review Bot]

Comment on attachment 172580 [details]
Patch

Attachment 172580 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/14761048

New failing tests:
inspector-protocol/debugger-terminate-dedicated-worker-while-paused.html

[Csaba Osztrogonác]

Comment on attachment 172580 [details]
Patch

LGTM, r=me. At least the following tests will pass if this flakiness occur.

[Balazs Kelemen]

Comment on attachment 172580 [details]
Patch

Clearing flags on attachment: 172580

Committed r133768: <http://trac.webkit.org/changeset/133768>

[Balazs Kelemen]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

Comment on attachment 172580 [details]
Patch

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

> Tools/WebKitTestRunner/TestController.cpp:697
> -        TestInvocation::dumpWebProcessUnresponsiveness("Failed to reset to consistent state before the first test");
> +        m_currentInvocation->dumpWebProcessUnresponsiveness();

This change was not correct - there is no current invocation at this point yet.

So, WebKitTestRunner just crashes when a debugger is attached to WebProcess before the first resetStateToConsistentValues().