Bug 36908 – Several tests in fast/canvas/webgl/ failed randomly on Leopard Commit Bot

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

fast/canvas/webgl/index-validation.html failed on Leopard Commit Bot

Twice actually.  Somehow lightning struck twice.

https://bugs.webkit.org/show_bug.cgi?id=36876#c3

/tmp/layout-test-results/fast/canvas/webgl/index-validation-actual.txt

44 
55 PASS gl.getError() is 0
66 PASS gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0) is undefined.
7  PASS gl.getError() is 0
 7 FAIL gl.getError() should be 0. Was 1286.
88 PASS successfullyParsed is true
99 
1010 TEST COMPLETE

I suspect it may be related to the commit-bot's hardware?

[Eric Seidel (no email)]

http://trac.webkit.org/browser/trunk/LayoutTests/fast/canvas/webgl/index-validation.html

[Eric Seidel (no email)]

I'm happy to send a full hardware report to anyone who might need one.

[Eric Seidel (no email)]

Grepping the commit-queue logs, I see the commit-queue having failed this test 9 times.  We just happen to hit two failures in a row while trying to land Adam's patch https://bugs.webkit.org/show_bug.cgi?id=36876#c3

[Zhenyao Mo]

This happened once on my machine before, but not recently.

I would suggest to use "antialias:false" in this test to resolve this random failure.  However, we need to dig into the reason why this happens and find a fix for it.

[Eric Seidel (no email)]

Failed again:
https://bugs.webkit.org/show_bug.cgi?id=36932#c4

[Eric Seidel (no email)]

Something happened today to make this failure much more common than previously.

[Eric Seidel (no email)]

Looks like the first time the Leopard Commit Bot ever saw this test fail was around 3:30PM this afternoon.

[Eric Seidel (no email)]

It was shortly after http://trac.webkit.org/changeset/56872, which is probably related. :)

[Eric Seidel (no email)]

This test has failed 39 times on the Leopard Commit Bot since it first failed this afternoon!

[Adam Barth]

This failure rate is unacceptable.  Can we rollback that rev and see if it fixes the problem?

[Kenneth Russell]

*Please* don't roll back that revision. It is a large one that we've been trying to land for weeks, and the failure is ultimately caused by driver bugs and not the code.

Can we temporarily disable this test on this bot? We may have another workaround we can land in a couple of hours.

[Adam Barth]

> Can we temporarily disable this test on this bot?

We don't have a way to disable tests on the commit-queue specifically.  We can disable it more generally however.

> We may have another workaround we can land in a couple of hours.

Ok.

By way of context, this test is flaky enough that it often fails twice in a row.  We have something like 13k tests.  If an appreciable fraction of them failed this often, the entire test suite would be useless.

[Kenneth Russell]

(In reply to comment #12)
> > Can we temporarily disable this test on this bot?
> 
> We don't have a way to disable tests on the commit-queue specifically.  We can
> disable it more generally however.

Can we add it to the Skipped list for mac-leopard for the moment?

> > We may have another workaround we can land in a couple of hours.
> 
> Ok.
> 
> By way of context, this test is flaky enough that it often fails twice in a
> row.  We have something like 13k tests.  If an appreciable fraction of them
> failed this often, the entire test suite would be useless.

Understood. I apologize for the flakiness introduced by this checkin.

[Zhenyao Mo]

Created attachment 52310 [details]
patch

This patch should stabilize the test on the bot.  However, we need to dig deeper into the cause of this flaky behavior - very likely hardware/driver bugs - and find a better way to solve it.  Will create another bug to track this down.

[Zhenyao Mo]

Just created a new bug to track this issue, but the patch should solve it for now.

https://bugs.webkit.org/show_bug.cgi?id=36972

[Kenneth Russell]

Comment on attachment 52310 [details]
patch

Looks good to me.

[Eric Seidel (no email)]

Comment on attachment 52310 [details]
patch

So is this work-around we intend to roll out eventually?  Or is this a permanent change to this test? The ChangeLog doesn't really say.  I'm fine with either of course.

[Eric Seidel (no email)]

(In reply to comment #12)
> By way of context, this test is flaky enough that it often fails twice in a
> row.  We have something like 13k tests.  If an appreciable fraction of them
> failed this often, the entire test suite would be useless.

Actually, the statement is stronger.  After Adam's changes to speed up the commit-queue yesterday (which happened right after this regression landed) it now requires a flaky test to fail *three* times in a row for it to cause the commit queue to wrongly reject a patch. :)

[Kenneth Russell]

(In reply to comment #17)
> (From update of attachment 52310 [details])
> So is this work-around we intend to roll out eventually?  Or is this a
> permanent change to this test? The ChangeLog doesn't really say.  I'm fine with
> either of course.

Right now we don't know why this OpenGL error is reported on this test, so the change is basically permanent.

[Eric Seidel (no email)]

Another victim:
https://bugs.webkit.org/show_bug.cgi?id=36918#c3

[Eric Seidel (no email)]

Another victim:
https://bugs.webkit.org/show_bug.cgi?id=36932#c6

[WebKit Commit Bot]

Comment on attachment 52310 [details]
patch

Clearing flags on attachment: 52310

Committed r56925: <http://trac.webkit.org/changeset/56925>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Eric Seidel (no email)]

This test has still failed at least twice on the commit-queue since this fix went in.  I can get you the failure diff.

[Eric Seidel (no email)]

The failure:
/tmp/layout-test-results/fast/canvas/webgl/index-validation-actual.txt

44 
55 PASS gl.getError() is 0
66 PASS gl.drawElements(gl.TRIANGLES, 3, gl.UNSIGNED_SHORT, 0) is undefined.
7  PASS gl.getError() is 0
 7 FAIL gl.getError() should be 0. Was 1286.
88 PASS successfullyParsed is true
99 
1010 TEST COMPLETE

[Zhenyao Mo]

After the previous patch, the only difference in this test before and after the multisample patch is the stencil buffer is on by default (before by default we only have color and depth buffer).

I'll get another patch ready to turn off the stencil buffer for this test shortly.

[Zhenyao Mo]

Also, I am curious, after the previous patch in which I turned off antialias for this test, does the frequency of this random failure reduced on the bot or still the same?

[Eric Seidel (no email)]

It appears to be greatly reduced.  There have been 9 failure on the bot since last night.  Yesterday afternoon we had 66.  The bot may simply be doing less now... but I think the failure incidence was reduced, but not eliminated by your previous change.

[Zhenyao Mo]

Created attachment 52421 [details]
patch: gather extra information about failure

This patch is an attempt to gather further information about the cause of this failure (sorry, but seems like the bot is the only machine that we get this failure at the moment).

With this patch, the failure behavior won't be better or worse.

[Zhenyao Mo]

Eric, when this patch lands, and the bot failed this test again, could you send the output to me?  Then I will upload another patch to fix the failure problem (hopefully).

[Kenneth Russell]

Comment on attachment 52421 [details]
patch: gather extra information about failure

Looks good to me.

[Eric Seidel (no email)]

Comment on attachment 52421 [details]
patch: gather extra information about failure

OK.

[WebKit Commit Bot]

Comment on attachment 52421 [details]
patch: gather extra information about failure

Clearing flags on attachment: 52421

Committed r57015: <http://trac.webkit.org/changeset/57015>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Zhenyao Mo]

These tests are still failing on the commit-bot.

[Zhenyao Mo]

Created attachment 53444 [details]
patch

[Kenneth Russell]

The patch still has an OOPS about no new tests. Also, I think it's worth separately mentioning in the ChangeLog that this fixes an uninitialized variable bug and restores a glFinish call that used to be present.

[Zhenyao Mo]

Created attachment 53445 [details]
revised patch : responding to Ken Russell's review

[Kenneth Russell]

Looks good to me.

[Adam Barth]

Comment on attachment 53445 [details]
revised patch : responding to Ken Russell's review

Me too.

[WebKit Commit Bot]

Comment on attachment 53445 [details]
revised patch : responding to Ken Russell's review

Rejecting patch 53445 from commit-queue.

Failed to run "['WebKitTools/Scripts/build-webkit', '--debug']" exit_code: 1
Last 500 characters of output:
n/yacc
    /bin/sh -c /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Script-5DF50887116F3077005202AB.sh
** BUILD FAILED **

The following build commands failed:
WebCore:
	Distributed-CompileC /Users/eseidel/Projects/CommitQueue/WebKitBuild/WebCore.build/Debug/WebCore.build/Objects-normal/i386/GraphicsContext3DMac.o /Users/eseidel/Projects/CommitQueue/WebCore/platform/graphics/mac/GraphicsContext3DMac.cpp normal i386 c++ com.apple.compilers.gcc.4_2
(1 failure)


Full output: http://webkit-commit-queue.appspot.com/results/1711103

[Zhenyao Mo]

Created attachment 53458 [details]
revised patch: initialize members in the same order as they are declared

[Kenneth Russell]

LGTM

[Adam Barth]

Comment on attachment 53458 [details]
revised patch: initialize members in the same order as they are declared

Ah, the old "order of initialization" check.  :)

[WebKit Commit Bot]

Comment on attachment 53458 [details]
revised patch: initialize members in the same order as they are declared

Clearing flags on attachment: 53458

Committed r57664: <http://trac.webkit.org/changeset/57664>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.