http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1296758116.1296762442.23082.gz Rev3 MacOSX Leopard 10.5.8 mozilla-central debug test mochitests-4/5 on 2011/02/03 10:35:16 s: talos-r3-leopard-039 22098 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_bug629838.html | Test timed out. Things go haywire around 991 INFO TEST-PASS | /tests/layout/base/tests/test_bug629838.html | Background color iteration 24, afterpaint count: 19, mozpaint count: 23 992 INFO TEST-PASS | /tests/layout/base/tests/test_bug629838.html | OnAfterPaint 993 INFO TEST-PASS | /tests/layout/base/tests/test_bug629838.html | Background color iteration 25, afterpaint count: 20, mozpaint count: 25 994 INFO TEST-PASS | /tests/layout/base/tests/test_bug629838.html | OnAfterPaint 995 INFO TEST-PASS | /tests/layout/base/tests/test_bug629838.html | Background color iteration 26, afterpaint count: 21, mozpaint count: 26 996 INFO TEST-PASS | /tests/layout/base/tests/test_bug629838.html | OnAfterPaint 997 INFO TEST-PASS | /tests/layout/base/tests/test_bug629838.html | Background color iteration 27, afterpaint count: 22, mozpaint count: 27 998 INFO TEST-PASS | /tests/layout/base/tests/test_bug629838.html | OnAfterPaint The problem looks to be in this code function startTest() { setTimeout(function () { afterPaintCount = 0; initialPaintCount = window.mozPaintCount; window.addEventListener("MozAfterPaint", onAfterPaint, true); doBackgroundFlicker(); }, 1000); when this runs, 0 <= window.mozPaintCount <= 4. But, the test then waits 1000ms before doing anything else. It's making the bad assumption that no painting happens after DOMContentLoaded. So, when we get to the "loop condition" here if ((afterPaintCount >= window.mozPaintCount - initialPaintCount) && (afterPaintCount > 20)) { the assumption that the test can't miss MozAfterPaint events is wrong, and the first clause in the if condition is bogus. I wonder if the best fix is to move the initialization code out of the delayed task in startTest().

Disregard comment 0, I confused myself. I'm not sure what's going on here. The test looks OK, AFAICT.

In the meantime, if we change if ((afterPaintCount >= window.mozPaintCount - initialPaintCount) && into an "assertion", the test will fail more quickly at least.

Rev3 MacOSX Snow Leopard 10.6.2 mozilla-central opt test mochitests-4/5 http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1296773931.1296775019.21434.gz&fulltext=1

Run the test until mozPaintCount has increased by 50, then check to ensure that we got at least 37 MozAfterPaint events. This should pass more reliably while still ensuring that MozAfterPaint is being fired by empty transactions. Thoughts?

Why 50/37?

Just picked an arbitrary number that's high enough to fail the test if the MozAfterPaint/empty transaction fix is missing, but low enough to pass for the example from the original comment.

Comment on attachment 509692 [details] [diff] [review] Make the test for bug 629838 less sensitive to variations in paint event count Shrug. Factor out the constants, add a comment about why they were chosen and how to change them if the test starts failing intermittently again, then r=me.

Moved the tuning values into constants and expanded the comments

Comment on attachment 509699 [details] [diff] [review] Make the test for bug 629838 less sensitive to variations in paint event count (2) Looks good. Minor point of order: usually if reviewers r+ a patch but leave comments to be addressed, then they don't need to see the fixed-up patch. For me, that means "this patch is fine to check in as-is but would be better if you fixed X,Y,Z, which I trust you to do". Reviewers vary though :).

I backed this patch as part of this pushlog <http://hg.mozilla.org/mozilla-central/pushloghtml?changeset=7e12e3e16e6c> because of the oranges it caused <http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1296852850.1296854923.2345.gz&fulltext=1>. I'm not sure which one of the bugs was at fault, so I just backed them all out. The assignee needs to investigate and make sure that his patch has not been the culprit, and then re-add checkin-needed.

Also, Kevin, please start the commit message for all of your patches with the bug number. The only bug number referenced in your commit message was bug 629838, so there is no way for anybody to reach this bug from your commit.

Fixed commit comment and rebased.