Closed Bug 852850 Opened 8 years ago Closed 8 years ago

Intermittent test_image_layers.html | Test timed out


(Core :: Graphics, defect)

Not set





(Reporter: philor, Assigned: mattwoodrow)



(Keywords: intermittent-failure, Whiteboard: [leave open])


(4 files, 2 obsolete files)

I've seen this twice now, once on one of my backouts, and first on That's and, or with the proper script, `backout cd08011c06f7:3ec52a8caf73`.
Ubuntu 12.04 x64 mozilla-inbound opt test mochitest-4 on 2013-03-19 19:55:42 PDT for push cd08011c06f7
slave: tst-linux64-ec2-309

20:00:27     INFO -  15188 INFO TEST-START | /tests/layout/base/tests/test_image_layers.html
20:05:36     INFO -  15189 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_image_layers.html | Test timed out.
(screenshot of the iframe half-red, half-vile-lime-green, on a putrid desktop)
20:05:38     INFO -  15190 INFO TEST-END | /tests/layout/base/tests/test_image_layers.html | finished in 309650ms
Ubuntu 12.04 x64 mozilla-inbound opt test mochitest-4 on 2013-03-19 22:45:12 PDT for push 26653529ea8b
slave: tst-linux64-ec2-036

22:49:57     INFO -  15190 INFO TEST-START | /tests/layout/base/tests/test_image_layers.html
22:55:15     INFO -  15191 ERROR TEST-UNEXPECTED-FAIL | /tests/layout/base/tests/test_image_layers.html | Test timed out.
(identical screenshot)
22:55:17     INFO -  15192 INFO TEST-END | /tests/layout/base/tests/test_image_layers.html | finished in 319281ms
Bug 851755 probably doesn't have anything to do with this as that only deals with XUL <image> and not HTML <img> used in this test. Although philor could prove me wrong...
Just blaming the whole push.

You know, the whole push and all the dependencies on it that have landed since, which will get backed out if this turns into some crazybad intermittent that fails 25 times a day.
This is from bug 852413 - Matt created a version of this that uses the reftest framework instead, so we don't have to keep trying to reverse-engineer the correct way of finding draws.
(In reply to Joe Drew (:JOEDREW! \o/) from comment #41)
> Created attachment 727713 [details] [diff] [review]
> matt's alternate version of this, as a reftest
> This is from bug 852413 - Matt created a version of this that uses the
> reftest framework instead, so we don't have to keep trying to
> reverse-engineer the correct way of finding draws.

Did you mean to request review on this? :-)
I haven't even run it yet, and I don't know whether Matt has either. :) Will look at it over the next handful of days.
Thank you :-)
(In reply to Joe Drew (:JOEDREW! \o/) (Vacation 26 March–11 April) from comment #61)
> I haven't even run it yet, and I don't know whether Matt has either. :) Will
> look at it over the next handful of days.

Do you know if you'll have a chance to look at this before you go on vacation? No worries if not, but we may as well disable it now if we know it will be failing at least until mid-April. Thanks :-)
I'm actually looking at this right now. I'll either get a working reftest or disable the test by the end of the day.
Thank you Joe, much appreciated :-D
This removes the old mochitest, since it's faily. r=me since Matt actually wrote this.
Attachment #727713 - Attachment is obsolete: true
Attachment #729052 - Flags: review+
This (again, written by Matt) adds the ability for the reftest framework to test whether an element is painted in an invalidation test, and fail if it is.
Attachment #729054 - Flags: review?(dbaron)
This patch takes the old mochitest and converts it into a reftest using reftest-no-paint. r=me since Matt wrote it.
Attachment #729056 - Flags: review+
I pushed the mochitest removal now, with the idea that the followup patches can be landed once David reviews the reftest-no-paint patch (while I'm on vacation, most likely).
Whiteboard: [leave open]
Assignee: joe → matt.woodrow
Attachment #729052 - Flags: checkin+
Hopefully fixed the crashtest error:
Fixed the crashtest failures.
Attachment #729054 - Attachment is obsolete: true
Attachment #729054 - Flags: review?(dbaron)
Attachment #729410 - Flags: review?(dbaron)
Can't we use nsIDOMWindowUtils::checkAndClearPaintedState for this? We already have mochitests using that to check that an element is not painted.
That's what this uses internally.

Writing mochitests is hard, since you have to wait for a stable painting state. That's what joe had initially, and had a lot of trouble with timeouts and intermittent failures.

The reftest framework already has code for this, and it's been extensively tested. Taking advantage of this makes it a lot easier to write a deterministic invalidation test.
Comment on attachment 729410 [details] [diff] [review]
part 2: add support for a reftest-no-paint class v2

Review of attachment 729410 [details] [diff] [review]:

ok, makes sense.
Attachment #729410 - Flags: review?(dbaron) → review+
Closed: 8 years ago
Resolution: --- → FIXED
So I see multiple problems in part 2 here:
 (1) the error message doesn't indicate which test failed, which is bad for TBPL starring
 (2) the failure case calls a LogError function that doesn't actually exist (not sure whether the exception here will cause the suite to fail)
 (3) otherwise, the failure case doesn't actually do anything to cause the test suite to fail
 (4) the failure handling also doesn't allow a test to be marked as known to fail.  It should handle that.

I suspect the solution to all of these (or maybe just 2-4?) may involve sending a message to the parent that there was a no-paint failure, and letting the parent handle the rest of the known-fail and reporting logic.

Matt, any chance I could get you to follow up here?

(I also wonder if these issues are responsible for bug 864686, though I don't yet see how they would be.)
Flags: needinfo?(matt.woodrow)
Also, I think there's a bit of a failure here in testing the new code.  While I'm not sure it should be a 100%-of-the-time expectation that people test that their tests fail without the patch (or with the feature being tested intentionally broken, if it's a test for an older feature) in addition to checking that they pass with the patch, with new test harness code like this, I would expect that sort of testing to check that the harness code actually works.
Attachment #756259 - Flags: review?(dbaron)
Flags: needinfo?(matt.woodrow)
Comment on attachment 756259 [details] [diff] [review]
Fixed error reporting, and added a test to make sure it fails correctly

>             // whether the comparison result matches what is in the manifest
>-            var test_passed = (equal == (gURLs[0].type == TYPE_REFTEST_EQUAL));
>+            var test_passed = (equal == (gURLs[0].type == TYPE_REFTEST_EQUAL)) && !gFailedNoPaint;

This isn't great, for two reasons:

 (1) when a reftest fails for no-paint, there's no indication that was the reason
 (2) it dumps screenshots unnecessarily (which also adds to the confusion in (1)) that won't match the failure description

Instead, could you make RecvFailedNoPaint do something like this:

                var test = gURLs[0];
                if (test.expected == EXPECTED_FAIL) {
                    gDumpLog("REFTEST TEST-KNOWN-FAIL | " + gCurrentURL +
                             " | failed reftest-no-paint\n");
                } else {
                    gDumpLog("REFTEST TEST-UNEXPECTED-FAIL | " + gCurrentURL +
                             " | failed reftest-no-paint\n");

and then, in the code quoted above, have two variables:  test_passed_equality = (what's there now) and test_passed = test_passed_equality && !gFailedNoPaint.

Then make the bit that dumps images, i.e.:

            if (!test_passed && expected == EXPECTED_PASS ||
                !test_passed && expected == EXPECTED_FUZZY ||
                test_passed && expected == EXPECTED_FAIL) {
                if (!equal) {
                    result += ", max difference: " + maxDifference.value + ", number of differing pixels: " + differences + "\n";
                    result += "REFTEST   IMAGE 1 (TEST): " + gCanvas1.toDataURL() + "\n";
                    result += "REFTEST   IMAGE 2 (REFERENCE): " + gCanvas2.toDataURL() + "\n";
                } else {
                    result += "\n";
                    gDumpLog("REFTEST   IMAGE: " + gCanvas1.toDataURL() + "\n");
            } else {
                result += "\n";

check test_passed_equality rather than test_passed.

r=dbaron with that or something like it
Attachment #756259 - Flags: review?(dbaron) → review+
You need to log in before you can comment on or make changes to this bug.